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.

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

make use of ringpop LookN method #5102

Merged
merged 2 commits into from
Nov 27, 2023
Merged

make use of ringpop LookN method #5102

merged 2 commits into from
Nov 27, 2023

Conversation

ast2023
Copy link
Contributor

@ast2023 ast2023 commented Nov 10, 2023

What changed?
To find number of per namespace workers we now use ringpop LookupN method instead of sequentially calling Lookup. The lookup key also changed: worker id was dropped from the hashed string.

Added util.SliceMap function and renamed util.SliceReduce to util.SliceFold

Why?
more evenly distribute work across ring

How did you test it?
local testing only

Potential risks
not sure

Is hotfix candidate?
no

@ast2023 ast2023 requested a review from a team as a code owner November 10, 2023 01:11
@CLAassistant
Copy link

CLAassistant commented Nov 10, 2023

CLA assistant check
All committers have signed the CLA.

@ast2023 ast2023 force-pushed the sasha/oss-1763 branch 2 times, most recently from 067f5c8 to 4fc6b21 Compare November 10, 2023 02:26
common/membership/ringpop/service_resolver.go Show resolved Hide resolved
common/membership/ringpop/service_resolver.go Outdated Show resolved Hide resolved
common/util/util.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Show resolved Hide resolved
service/worker/pernamespaceworker.go Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
common/membership/interfaces.go Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker_test.go Outdated Show resolved Hide resolved
@ast2023
Copy link
Contributor Author

ast2023 commented Nov 14, 2023

Hey @dnr, I've uploaded a new version. I addressed most of the suggestions you pointed out. Additionally, I did some refactoring to ensure that the code better aligns with the information you provided. Let me know your thoughts!

common/dynamicconfig/collection.go Outdated Show resolved Hide resolved
common/membership/interfaces.go Outdated Show resolved Hide resolved
common/membership/ringpop/service_resolver.go Outdated Show resolved Hide resolved
common/util/util.go Show resolved Hide resolved
common/util/util.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
@ast2023 ast2023 force-pushed the sasha/oss-1763 branch 3 times, most recently from fba2f63 to 92bb47a Compare November 16, 2023 22:16
common/membership/ringpop/service_resolver.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker_test.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker_test.go Outdated Show resolved Hide resolved
@ast2023 ast2023 force-pushed the sasha/oss-1763 branch 2 times, most recently from f581fe6 to 3e26e79 Compare November 17, 2023 20:38
common/dynamicconfig/collection.go Outdated Show resolved Hide resolved
common/util/util_test.go Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker_test.go Outdated Show resolved Hide resolved
service/worker/pernamespaceworker_test.go Outdated Show resolved Hide resolved
Copy link
Member

@dnr dnr left a comment

Choose a reason for hiding this comment

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

Please fix the typo in the PR title

@ast2023 ast2023 merged commit b2c9944 into main Nov 27, 2023
10 checks passed
@ast2023 ast2023 deleted the sasha/oss-1763 branch November 27, 2023 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants