Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feat] Change to unix domain socket #52

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

[Feat] Change to unix domain socket #52

wants to merge 10 commits into from

Conversation

RandyLambert
Copy link
Contributor

@RandyLambert RandyLambert commented Jul 8, 2022 •
edited
Loading

Signed-off-by: RandyLambert [email protected]

@RandyLambert @ti-chi-bot
@YangKeao @Hexilee
[Feat] Change to unix domain socket
ccc5cfc 
* Use use tokio::net::UnixListener instead of tokio::net::Unix for unix socket communication

Signed-off-by: shouxunsun <[email protected]>

Co-authored-by: Ti Chi Robot <[email protected]>
Co-authored-by: Yang Keao <[email protected]>
Co-authored-by: xixi <[email protected]>
Signed-off-by: RandyLambert <[email protected]>
@RandyLambert
[Fix] Delete serve_interactive return error
4f5d9bb 
* Delete serve_interactive return error

Signed-off-by: RandyLambert <[email protected]>
@RandyLambert RandyLambert mentioned this pull request Jul 9, 2022
Open
RandyLambert and others added 4 commits August 1, 2022 02:20
@RandyLambert
[Feat] Use unix domain socket with path
7d4795f 
* use unix domain socket with path

Signed-off-by: RandyLambert <[email protected]>
@RandyLambert @ti-chi-bot
[Feat] Use unix domain socket with path
26df9e2 
* use unix domain socket with path

Signed-off-by: RandyLambert <[email protected]>

Co-authored-by: Ti Chi Robot <[email protected]>
@RandyLambert @ti-chi-bot
[Feat] Use unix domain socket with path
f0c5434 
* use unix domain socket with path

Signed-off-by: RandyLambert <[email protected]>

Co-authored-by: Ti Chi Robot <[email protected]>
@RandyLambert @ti-chi-bot
[Feat] Use unix domain socket with path
492d386 
* use unix domain socket with path

Signed-off-by: RandyLambert <[email protected]>

Co-authored-by: Ti Chi Robot <[email protected]>
Andrewmatilde
Andrewmatilde approved these changes Aug 21, 2022
View reviewed changes
Copy link
Member

@Andrewmatilde Andrewmatilde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

@RandyLambert
[Feat] Use unix domain socket with path
6cde853 
* use unix domain socket with path

Signed-off-by: RandyLambert <[email protected]>
Andrewmatilde
Andrewmatilde approved these changes Aug 31, 2022
View reviewed changes
Copy link
Member

@Andrewmatilde Andrewmatilde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Andrewmatilde
Copy link
Member

PTAL @Hexilee

Hexilee
Hexilee approved these changes Sep 4, 2022
View reviewed changes
Copy link
Member

@Hexilee Hexilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

chaos-tproxy-controller/src/main.rs Outdated Show resolved Hide resolved
chaos-tproxy-controller/src/main.rs Outdated Show resolved Hide resolved
chaos-tproxy-controller/src/main.rs Outdated Show resolved Hide resolved
@Hexilee Hexilee mentioned this pull request Sep 9, 2022
Closed
11 tasks
@RandyLambert
[Feat] Use unix domain socket with path
8321a2e 
* use unix domain socket with path

Signed-off-by: RandyLambert <[email protected]>
@RandyLambert
[Feat] Use unix domain socket with path
22bc2ce 
* use unix domain socket with path

Signed-off-by: RandyLambert <[email protected]>
@Hexilee
Copy link
Member

Hexilee commented Sep 19, 2022

@RandyLambert Please fix the conflicts, then we can merge this PR.

@RandyLambert
Copy link
Contributor Author

@RandyLambert Please fix the conflicts, then we can merge this PR.

@Hexilee fixed

@STRRL
Copy link
Member

STRRL commented Oct 14, 2022

Could we merge this PR?

ping @Hexilee @Andrewmatilde

@Hexilee
Copy link
Member

Hexilee commented Oct 14, 2022 •
edited
Loading

Could we merge this PR?

Yes, I think we can.

@Andrewmatilde
Copy link
Member

I guess not , this PR will block master branch release before this PR merged. chaos-mesh/chaos-mesh#3553

@STRRL
Copy link
Member

STRRL commented Oct 14, 2022

I guess not , this PR will block master branch release before this PR merged. chaos-mesh/chaos-mesh#3553

hmmm, it seems this PR brings breaking changes with chaos-tproxy and it requires to merge different PRs at the same time across different repos.

And that is nearly IMPOSSIBLE.

Please consider keeping the old flags and functionality, keep both stdio and uds.

After chaos-mesh/chaos-mesh#3553 get merged, remove stdio part.

@STRRL
Copy link
Member

STRRL commented Oct 14, 2022

I guess not , this PR will block master branch release before this PR merged. chaos-mesh/chaos-mesh#3553

hmmm, it seems this PR brings breaking changes with chaos-tproxy and it requires to merge different PRs at the same time across different repos.

And that is nearly IMPOSSIBLE.

Please consider keeping the old flags and functionality, keep both stdio and uds.

After chaos-mesh/chaos-mesh#3553 get merged, remove stdio part.

PTAL @RandyLambert , also cc @Hexilee @Andrewmatilde

@RandyLambert
Copy link
Contributor Author

I guess not , this PR will block master branch release before this PR merged. chaos-mesh/chaos-mesh#3553

hmmm, it seems this PR brings breaking changes with chaos-tproxy and it requires to merge different PRs at the same time across different repos.
And that is nearly IMPOSSIBLE.
Please consider keeping the old flags and functionality, keep both stdio and uds.
After chaos-mesh/chaos-mesh#3553 get merged, remove stdio part.

PTAL @RandyLambert , also cc @Hexilee @Andrewmatilde

When chaos-mesh is used, it will be based on the release versions of tdoa and tproxy. I think this pr can be merged. Before the release version of toda and tproxy pulled before the dockfile of choas-mesh is not modified, this merge will not affect the use of chaos- mesh.

@STRRL
Copy link
Member

STRRL commented Oct 17, 2022

I guess not , this PR will block master branch release before this PR merged. chaos-mesh/chaos-mesh#3553

hmmm, it seems this PR brings breaking changes with chaos-tproxy and it requires to merge different PRs at the same time across different repos.
And that is nearly IMPOSSIBLE.
Please consider keeping the old flags and functionality, keep both stdio and uds.
After chaos-mesh/chaos-mesh#3553 get merged, remove stdio part.

PTAL @RandyLambert , also cc @Hexilee @Andrewmatilde

When chaos-mesh is used, it will be based on the release versions of tdoa and tproxy. I think this pr can be merged. Before the release version of toda and tproxy pulled before the dockfile of choas-mesh is not modified, this merge will not affect the use of chaos- mesh.

cc @Andrewmatilde

@Andrewmatilde
Copy link
Member

I guess not , this PR will block master branch release before this PR merged. chaos-mesh/chaos-mesh#3553

hmmm, it seems this PR brings breaking changes with chaos-tproxy and it requires to merge different PRs at the same time across different repos.
And that is nearly IMPOSSIBLE.
Please consider keeping the old flags and functionality, keep both stdio and uds.
After chaos-mesh/chaos-mesh#3553 get merged, remove stdio part.

PTAL @RandyLambert , also cc @Hexilee @Andrewmatilde

When chaos-mesh is used, it will be based on the release versions of tdoa and tproxy. I think this pr can be merged. Before the release version of toda and tproxy pulled before the dockfile of choas-mesh is not modified, this merge will not affect the use of chaos- mesh.

But when will this PR merge?

@STRRL
Copy link
Member

STRRL commented Oct 17, 2022

But when will this chaos-mesh/chaos-mesh#3553 merge?

I would push the review progress about this PR and chaos-mesh/chaos-mesh#3553, are there some kind of dependencies between these PRs? eg, one PR should be merged before another PR.

@Andrewmatilde
Copy link
Member

I would push the review progress about this PR and chaos-mesh/chaos-mesh#3553, are there some kind of dependencies between these PRs? eg, one PR should be merged before another PR.

This PR have break changes on the parts of commucation between chaos-mesh & chaos-tproxy. chaos-mesh/chaos-mesh#3553 rely on a special version of this PR. But if this PR merge we will rollback it every time when release before chaos-mesh/chaos-mesh#3553 would merged.

@STRRL
Copy link
Member

STRRL commented Oct 17, 2022

I would push the review progress about this PR and chaos-mesh/chaos-mesh#3553, are there some kind of dependencies between these PRs? eg, one PR should be merged before another PR.

This PR have break changes on the parts of commucation between chaos-mesh & chaos-tproxy. chaos-mesh/chaos-mesh#3553 rely on a special version of this PR. But if this PR merge we will rollback it every time when release before chaos-mesh/chaos-mesh#3553 would merged.

Actually, I do not think so. As @RandyLambert said, Chaos Mesh uses a versioned release of chaos-tproxy, merging this PR into master should NOT effect anything. 🤔

@Andrewmatilde
Copy link
Member

Actually, I do not think so. As @RandyLambert said, Chaos Mesh uses a versioned release of chaos-tproxy, merging this PR into master should NOT effect anything. 🤔

There may have a problem in chaos-tproxy. Every versioned release of chaos-tproxy now rely on the master branch of chaos-tproxy. If this PR merge in master , it will affect the PR merged after it.

@Andrewmatilde
Copy link
Member

If this PR merged , & then I merged #44 , & then I want to use it in chaos-mesh. I need to rollback #52 ,then create a released version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andrewmatilde Andrewmatilde Andrewmatilde approved these changes

@Hexilee Hexilee Hexilee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RandyLambert @Andrewmatilde @Hexilee @STRRL