-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
release-22.1: kvserver: disable merge queue until kvsubscriber has updated #78190
release-22.1: kvserver: disable merge queue until kvsubscriber has updated #78190
Conversation
If we don't have any span configs available, enabling range merges would be extremely dangerous -- we could collapse everything into a single range. We've observed this happen when the kvsubscriber's initial scan overflows its bounded buffer, preventing it from ever getting a snapshot. A future commit will fix the bounded memory issue, but the side-effect pointed out the need for this important safe guard. Informs #77687. Release justification: bug fix Release note: None
0d9961c
to
5c99e1f
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
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. Do you think we need similar protection in any of the other queues?
The split queue will implicitly not be able to do anything, if the kvsubscriber doesn’t have data, the split queue doesn’t have any split points it can induce. Only other queue I can think of is maybe the MVCC GC queue and it not having protected timestamps installed by SQL. I’m not sure if holding up GC is the right solution there. @arulajmani, what do you think? |
I think it's okay to not hold up GC if protected timestamps haven't been reconciled. All our claims are for when protected timestamps are reconciled to KV, not when they're written in SQL, so we should be good. |
But this happens after reconciliation |
Ah right -- I'll send out a patch that doesn't allow GC-ing if PTS information isn't available. |
Backport 1/1 commits from #78122 on behalf of @irfansharif.
/cc @cockroachdb/release
If we don't have any span configs available, enabling range merges would
be extremely dangerous -- we could collapse everything into a single
range. We've observed this happen when the kvsubscriber's initial scan
overflows its bounded buffer, preventing it from ever getting a
snapshot. A future commit will fix the bounded memory issue, but the
side-effect pointed out the need for this important safe guard.
Informs #77687.
Release justification: bug fix
Release note: None
Release justification: