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

On shutdown, remove ourself from the peers list #569

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

kentquirk
Copy link
Contributor

Which problem is this PR solving?

  • In a redis-managed pool of peers, the peer group is updated only after a timeout. This adds explicit unregistration so that in the case of a deliberately terminated instance, redis is notified immediately that the peer has dropped.
  • Doesn't change the behavior of refreshing the peer list, which is still a pull-based timeout.
  • Fixes Refinery nodes should remove themselves from peer management on shutdown #393

Short description of the changes

  • Add an Unregister method to redimem
  • Add a done channel to the goroutines that keep redis updated
  • Unregister when the done channel is closed
  • Propagate the done channel through the app initialization
  • Close the done channel on shutdown
  • Update tests
  • Add a new test to make sure that unregistration happens
  • Also adds a new test case to rules; I wrote it to verify we didn't have a bug and thought it should probably stay in.

@kentquirk kentquirk requested a review from a team November 23, 2022 14:48
@@ -1,5 +1,3 @@
//go:build all || race
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need these build flags any more so I removed them.

@kentquirk kentquirk self-assigned this Nov 23, 2022
@kentquirk kentquirk added type: enhancement New feature or request status: review needed Changes need review. version: bump minor A PR that adds behavior, but is backwards-compatible. labels Nov 23, 2022
cmd/refinery/main.go Show resolved Hide resolved
internal/peer/redis.go Show resolved Hide resolved
@kentquirk kentquirk merged commit 854613c into main Nov 28, 2022
@kentquirk kentquirk deleted the kent.unregister_on_exit branch November 28, 2022 22:04
ghost pushed a commit to opsramp/tracing-proxy that referenced this pull request Jul 5, 2024
## Which problem is this PR solving?

- In a redis-managed pool of peers, the peer group is updated only after
a timeout. This adds explicit unregistration so that in the case of a
deliberately terminated instance, redis is notified immediately that the
peer has dropped.
- Doesn't change the behavior of refreshing the peer list, which is
still a pull-based timeout.
- Fixes honeycombio#393 

## Short description of the changes

- Add an Unregister method to redimem
- Add a done channel to the goroutines that keep redis updated
- Unregister when the done channel is closed
- Propagate the done channel through the app initialization
- Close the done channel on shutdown
- Update tests
- Add a new test to make sure that unregistration happens
- Also adds a new test case to rules; I wrote it to verify we didn't
have a bug and thought it should probably stay in.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: review needed Changes need review. type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refinery nodes should remove themselves from peer management on shutdown
2 participants