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

[FIXED] Don't decrement pending count twice after ack #6343

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

MauriceVanVeen
Copy link
Member

Reverts the change made in #6297. The change did (somewhat) improve the reliability of the drifting tests if the o.npc-- was done as a result of contention, but resulted in a regression if o.npc-- was done twice when a message was acked that did not move the ack floor up for WorkQueue/Interest retention.

This PR fixes what would otherwise have been a regression.

We really should try to fix the race condition itself though (outside of this PR). Without fixing it the pending count can be incorrect with no way to be resolved unless all messages are consumed, or we'd need to manually recalculate.

Signed-off-by: Maurice van Veen [email protected]

@MauriceVanVeen MauriceVanVeen requested a review from a team as a code owner January 8, 2025 13:32
@MauriceVanVeen MauriceVanVeen force-pushed the maurice/dont-decrement-pending-twice branch from 405c4dd to dd27b48 Compare January 8, 2025 13:46
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit 3fefcf3 into main Jan 8, 2025
5 checks passed
@derekcollison derekcollison deleted the maurice/dont-decrement-pending-twice branch January 8, 2025 14:32
wallyqs added a commit that referenced this pull request Jan 9, 2025
#### Dependencies
- #6323
- #6324

####  Leafnode
- #6291

#### JetStream
- #6226
- #6235
- #6277
- #6279
- #6283
- #6289
- #6316
- #6317
- #6325
- #6326
- #6335
- #6338
- #6341
- #6344
- #6150
- #6351
- #6355

#### Tests
- #6278
- #6297
- #6300
- #6343
- #6329
- #6330
- #6331
- #6332
- #6334
- #6356
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.

2 participants