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

feat: peer management on pubsub via callbacks #1220

Merged
merged 11 commits into from
Jul 3, 2024
Merged

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Jul 2, 2024

Which problem is this PR solving?

  • Implement the current peers system using a pubsub model for peers
  • Uses the current pubsub system that uses callbacks

Short description of the changes

  • Add start/stop to pubsub so it can become injectable
  • Fix the mock and existing implementations
  • Implement pubsub peers using the new pubsub system and generic.SetWithTTL
  • Add a few tests
  • Update config metadata
  • delete legacy redis peering
  • make it more idiomatically injectable
  • do all the things to make injection work everywhere

closes #1201

@kentquirk kentquirk self-assigned this Jul 2, 2024
@kentquirk kentquirk requested a review from a team as a code owner July 2, 2024 22:32
Copy link
Contributor

@VinozzZ VinozzZ left a comment

Choose a reason for hiding this comment

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

I'm more inclined to remove the existing redis peer management logic. Couple reasons that comes to mind:

  • since the pub/sub implementation is still using redis, having it as a separate config option feels a bit confusing for me
  • does that mean stress_relief also needs a type option so that customers can choose to stay with the existing stress relief mechanism?
  • I can't think of a scenario that customers will want to stay with the existing redis implementation if pub/sub system ultimately enables scaling without loosing data.
  • it would be nice that we can just have one redis package in refinery instead of maintaining two implementation

internal/peer/pubsub.go Outdated Show resolved Hide resolved
internal/peer/pubsub.go Outdated Show resolved Hide resolved
})
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

How about a test with multiple peers that can get correct number of peers and the callback functions are called when a peer joins or leaves the cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been trying to write that test, and it's not real easy to run multiple peers on the same machine. I think it's possible, but it might need some gymnastics to make it happen.

Because I'm going on vacation now, I'm going to merge this so it doesn't get stale while I'm gone. We can look at that test another time.

@kentquirk kentquirk merged commit f8d7c17 into main Jul 3, 2024
4 checks passed
@kentquirk kentquirk deleted the kent.pubsub_on_cb branch July 3, 2024 23:25
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.

Implement Gossip protocol
2 participants