-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
NRG: Lockless Leaderless
and HadPreviousLeader
#6438
Conversation
Signed-off-by: Neil Twigg <[email protected]>
…hreshold` Signed-off-by: Neil Twigg <[email protected]>
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.
LGTM - Just question on other changes.
@@ -4436,7 +4436,11 @@ func (js *jetStream) processClusterCreateConsumer(ca *consumerAssignment, state | |||
if isConfigUpdate = !reflect.DeepEqual(&cfg, ca.Config); isConfigUpdate { | |||
// Call into update, ignore consumer exists error here since this means an old deliver subject is bound | |||
// which can happen on restart etc. | |||
if err := o.updateConfig(ca.Config); err != nil && err != NewJSConsumerNameExistError() { | |||
// JS lock needed as this can mutate the consumer assignments and race with updateInactivityThreshold. | |||
js.mu.Lock() |
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.
These suppose to be part of this PR?
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.
Something about the changing in the locking caused this race condition to hit far more often, to the point that it was taking several attempts of kicking CI.
I've kept it isolated in a second commit though, as that change won't be relevant for a 2.10.x backport.
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.
LGTM
This removes lock contention around JS API requests figuring out if the meta group is leaderless or not by instead tracking that state atomically.
Also fixes a race condition in inactivity thresholds/consumer pause that this seemed to uncover.
Signed-off-by: Neil Twigg [email protected]