-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add more addTemporaryStateListener utils
#125648
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
Add more addTemporaryStateListener utils
#125648
Conversation
We often call `addTemporaryStateListener` with the `ClusterService` of a random node, or the currently elected master. This commit adds utilities for this common pattern.
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| * belongs to the chosen node's {@link ClusterService}. | ||
| */ | ||
| public static SubscribableListener<Void> addTemporaryStateListener(Predicate<ClusterState> predicate) { | ||
| return addTemporaryStateListener(ESIntegTestCase.internalCluster().clusterService(), predicate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, for some reason I assumed ESIntegTestCase#internalCluster() was only available in ESIntegTestCase and descendants. Realizing it's publicly available, I definitely agree we should try to keep utility methods outside of ESIntegTestCase as that class is already huge. I'll move the data stream utility methods I recently added in ESIntegTestCase to a different class sometime.
| * {@link ESTestCase#SAFE_AWAIT_TIMEOUT} then the listener is completed exceptionally on the scheduler thread that belongs to | ||
| * the elected master node's {@link ClusterService}. | ||
| */ | ||
| public static SubscribableListener<Void> addMasterTemporaryStateListener(Predicate<ClusterState> predicate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ on explicitly mentioning master in the method name. Should we mention randomNode (or something similar) in the other method? You're updating those usages anyway, so it won't result in a lot more changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conventionally we don't say randomNode in these things (or else we'd say it almost everywhere). I'm going to stick with that, but if we changed it everywhere then that'd be ok too :)
|
Thanks, David! |
💔 Backport failed
You can use sqren/backport to manually backport by running |
We often call `addTemporaryStateListener` with the `ClusterService` of a random node, or the currently elected master. This commit adds utilities for this common pattern.
We often call
addTemporaryStateListenerwith theClusterServiceof arandom node, or the currently elected master. This commit adds utilities
for this common pattern.