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

Notify named subscribers for membership changes #5823

Merged

Conversation

mantas-sidlauskas
Copy link
Contributor

What changed?
Notify "named" membership change subscribers

Why?
There are two levels of membership change notification mechanisms. One is per-service (frontend, history, etc), another one is for services to use in their internal components (like shard controller in history service).

For service-level subscribers, there was a missing piece of code to send notification on registered channel. As this is a bug, automatic 10s interval refresh helped to mitigate this bug. Now service-level subscribers will be informed immediately.

How did you test it?
Added unit test to check if notifications are sent when receiving message from "peer provider" (ringpop for example)

Potential risks

Release notes

Documentation Changes

@coveralls
Copy link

coveralls commented Mar 29, 2024

Pull Request Test Coverage Report for Build 018ebfa2-6b0a-49b7-84c6-36513803f096

Details

  • 27 of 30 (90.0%) changed or added relevant lines in 1 file are covered.
  • 42 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.003%) to 67.356%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/membership/hashring.go 27 30 90.0%
Files with Coverage Reduction New Missed Lines %
service/history/task/transfer_standby_task_executor.go 2 86.6%
service/frontend/api/handler.go 2 62.05%
common/archiver/filestore/historyArchiver.go 4 80.95%
common/task/fifo_task_scheduler.go 5 84.54%
service/frontend/wrappers/metered/metered.go 9 63.18%
service/history/task/task_util.go 20 70.57%
Totals Coverage Status
Change from base Build 018ebfa1-6acc-4043-824b-1582ae3eb76d: 0.003%
Covered Lines: 98180
Relevant Lines: 145763

💛 - Coveralls

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Merging #5823 (3f42064) into master (604b55c) will increase coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 82.75%.

❗ Current head 3f42064 differs from pull request most recent head e975bc3. Consider uploading reports for the commit e975bc3 to get more accurate results

Additional details and impacted files
Files Coverage Δ
common/membership/hashring.go 83.04% <82.75%> (+1.79%) ⬆️

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56624b6...e975bc3. Read the comment docs.

Copy link
Contributor

@Groxx Groxx left a comment

Choose a reason for hiding this comment

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

blocking until core issues are cleared up

Copy link
Contributor

@Groxx Groxx left a comment

Choose a reason for hiding this comment

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

  • Q on a surprising change (just not sure how the changes lead to it, if there's a cause/effect I'm missing then it's totally fine and it looks correct to me)
  • new test is rather highly racy and not reliable

Non-test-code changes look correct though 👍 I'll double check that next time, but I didn't see any issues in this read through.

Copy link
Contributor

@Groxx Groxx left a comment

Choose a reason for hiding this comment

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

Yea, looks correct and I also can't get any test failures by stressing it*. Will merge as soon as the merge-and-retest finishes 👍

It'd be nice to shift all this to the mock clock so we can reduce some sleeps / actually ensure that only the test-triggered refresh runs (and not the ticker, though that'd take absurd machine load to hit), but that's well outside the scope of this PR.

* unless it runs too long / -count 1000 but that appears to be due to not shutting down hashrings in any of these tests.

@Groxx Groxx enabled auto-merge (squash) April 8, 2024 21:29
@Groxx Groxx merged commit 3e05f2c into cadence-workflow:master Apr 8, 2024
18 checks passed
@mantas-sidlauskas mantas-sidlauskas deleted the subscriber_notification branch April 10, 2024 06: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.

4 participants