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

add keepalive #14

Merged
merged 12 commits into from
Aug 22, 2023
Merged

add keepalive #14

merged 12 commits into from
Aug 22, 2023

Conversation

day253
Copy link
Contributor

@day253 day253 commented Oct 24, 2022 •
edited
Loading

fix
#10

@CLAassistant
Copy link

CLAassistant commented Oct 24, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@day253
Copy link
Contributor Author

day253 commented Oct 24, 2022

/assign @horizonzy

@day253 day253 marked this pull request as draft October 24, 2022 14:12
@bytedance-oss-robot bytedance-oss-robot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2022
@day253 day253 marked this pull request as ready for review October 24, 2022 14:14
@bytedance-oss-robot bytedance-oss-robot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 24, 2022
@horizonzy
Copy link
Collaborator

horizonzy commented Oct 25, 2022 •
edited
Loading

Could we use a watcher to listen to connection events? It's more graceful.

@horizonzy
Copy link
Collaborator

Could we use a watcher to listen to connection events? It's more graceful.
Ignore it, the zk lib didn't support connection listener

horizonzy
horizonzy reviewed Oct 25, 2022
View reviewed changes
registry/registry.go Outdated Show resolved Hide resolved
horizonzy
horizonzy reviewed Oct 25, 2022
View reviewed changes
registry/registry.go Outdated Show resolved Hide resolved
@day253 day253 requested a review from horizonzy October 25, 2022 10:54
horizonzy
horizonzy reviewed Oct 26, 2022
View reviewed changes
registry/registry.go Outdated Show resolved Hide resolved
@day253 day253 requested a review from horizonzy October 26, 2022 03:51
horizonzy
horizonzy reviewed Oct 26, 2022
View reviewed changes
registry/registry.go Outdated Show resolved Hide resolved
@horizonzy
Copy link
Collaborator

Nicely done, could you add a test for cover this case?

@day253
Copy link
Contributor Author

day253 commented Oct 27, 2022

Nicely done, could you add a test for cover this case?

Ok.

@GuangmingLuo
Copy link
Member

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

@day253 sign CLA please.

@GuangmingLuo
Copy link
Member

@horizonzy please confirm and handle this PR.

@day253
Copy link
Contributor Author

day253 commented Feb 1, 2023 •
edited
Loading

We deployed this to our production environment. But the difference is when Deregister is called we will cancel the keepalive goroutine. @horizonzy Should we add this function?

@day253
Copy link
Contributor Author

day253 commented Feb 1, 2023

Nicely done, could you add a test for cover this case?

If we have the test to test the keepalive function. We should have two method. The first one is to delete the registered node. The other one is delete the registered node and cancel the keepalive function. But now the Deregister is almost the same as deleteNode.

@GuangmingLuo GuangmingLuo assigned bodhisatan and unassigned horizonzy Feb 10, 2023
bodhisatan
bodhisatan reviewed Feb 17, 2023
View reviewed changes
registry/registry.go Outdated
}
}

func (z *zookeeperRegistry) ensureName(path string, data []byte, flags int32) error {
Copy link
Member

Choose a reason for hiding this comment

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

The function's name doesn't express what it wants to ensure. Maybe a different name would be better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@horizonzy
Copy link
Collaborator

Sorry for the delay reply, I'm watching now.

@horizonzy
Copy link
Collaborator

We deployed this to our production environment. But the difference is when Deregister is called we will cancel the keepalive goroutine. @horizonzy Should we add this function?

Yes, if the service already be deregister, we shouldn't make it keep-alive anymore.

@horizonzy
Copy link
Collaborator

If we have the test to test the keepalive function. We should have two method. The first one is to delete the registered node. The other one is delete the registered node and cancel the keepalive function. But now the Deregister is almost the same as deleteNode.

We can delete the node path using zk directly, not using the registry.go function.

@day253
Copy link
Contributor Author

day253 commented Jun 26, 2023

@horizonzy I have fixed all the comments.

@day253 day253 requested a review from horizonzy June 26, 2023 09:05
@day253
Copy link
Contributor Author

day253 commented Jun 26, 2023

cc @bodhisatan

@bodhisatan
Copy link
Member

cc @bodhisatan

get, i'm reviewing it

@horizonzy
Copy link
Collaborator

Sorry, I haven't used Go for over a year, I am worried that I might not have the ability to review. Please help to review it, thanks! @bodhisatan

@day253
Copy link
Contributor Author

day253 commented Aug 20, 2023

@bodhisatan any idea?

@bytedance-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bodhisatan, day253
To complete the pull request process, please assign horizonzy after the PR has been reviewed.
You can assign the PR to them by writing /assign @horizonzy in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@bodhisatan
Copy link
Member

@bodhisatan any idea?

LGTM

@day253
Copy link
Contributor Author

day253 commented Aug 22, 2023

/assign @horizonzy

@day253
Copy link
Contributor Author

day253 commented Aug 22, 2023

@bodhisatan any idea?

LGTM

thx

@bodhisatan bodhisatan merged commit bddfec4 into kitex-contrib:main Aug 22, 2023
3 checks passed
@day253 day253 deleted the keepalive branch August 22, 2023 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bodhisatan bodhisatan bodhisatan approved these changes

@horizonzy horizonzy Awaiting requested review from horizonzy

Assignees

@horizonzy horizonzy

@bodhisatan bodhisatan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@day253 @CLAassistant @horizonzy @GuangmingLuo @bodhisatan