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.

Sign up for GitHub

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

Jump to bottom

kvserver: use separate type for uninitialized replicas #72374

Closed
tbg opened this issue Nov 3, 2021 · 3 comments
Closed

kvserver: use separate type for uninitialized replicas #72374

tbg opened this issue Nov 3, 2021 · 3 comments
Labels
A-kv-replication Relating to Raft, consensus, and coordination. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) no-issue-activity X-stale

Comments

@tbg
Copy link
Member

tbg commented Nov 3, 2021 •
edited by cockroach-jira-scripts
Loading

Is your feature request related to a problem? Please describe.
Replicas can be uninitialized, meaning that they are essentially a pure Raft group waiting to receive a snapshot. We currently represent uninitialized replicas as Replicas. This litters the code with extra checks and is a major driver of complexity along with making the lower replication layers fairly impermeable. If we can make uninitialized into a separate type and clarify when and using which code transitions between the states are made, much will be gained.

Jira issue: CRDB-11122
@tbg tbg self-assigned this Nov 3, 2021
@blathers-crl

This comment has been minimized.

Sign in to view
@blathers-crl blathers-crl bot added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Nov 3, 2021
@tbg tbg added the A-kv-replication Relating to Raft, consensus, and coordination. label Nov 3, 2021
tbg added a commit to tbg/cockroach that referenced this issue Nov 3, 2021
@tbg
kvserver: use AdminSplit in splitTestRange
593c30b 
`splitTestRange` was previously reaching into the store to finagle a
split. It is used by around a dozen tests. Prototyping around cockroachdb#72374
has shown that these tests frequently need patching up whenever we
adjust (improve) the store's replica handling.

This is a time suck and besides, we also want to be able to test
the Store from within the `kvserver` (not `kvserver_test`) package.
So if we can make that happen, and can use AdminSplit, that would
be preferrable.

AdminSplit requires them to run a (somewhat) distributed multi-range
transaction. A first split would hit a single range, but after that the
split batch hits at least two ranges (meta2 and splitKey), and so we
need nontrivial DistSender-like functionality. Splits are also
nontrivial distributed transactions and so we need a TxnCoordSender.
Experience suggests that it's better to use the "real thing" and to
make sure it's configurable enough to fit the use case, rather than
whipping up half-baked replacements.

Luckily, it turned out that DistSender and TxnCoordSender are already
up to the task, and this commit adopts them in
`createTestStoreWithoutStart`, and changes `splitTestRange` to use
`AdminSplit`.

Release note: None
@tbg tbg mentioned this issue Nov 3, 2021
Merged
tbg added a commit to tbg/cockroach that referenced this issue Nov 4, 2021
@tbg
kvserver: use wrapper type for Store.mu.replicas
6256dd8 
This simplifies lots of callers and it will also make it easier to work
on cockroachdb#72374, where this map will start containing more than one type as
value.

Release note: None
@tbg tbg mentioned this issue Nov 4, 2021
Merged
tbg added a commit to tbg/cockroach that referenced this issue Nov 4, 2021
@tbg
kvserver: use AdminSplit in splitTestRange
cd258bc 
`splitTestRange` was previously reaching into the store to finagle a
split. It is used by around a dozen tests. Prototyping around cockroachdb#72374
has shown that these tests frequently need patching up whenever we
adjust (improve) the store's replica handling.

This is a time suck and besides, we also want to be able to test
the Store from within the `kvserver` (not `kvserver_test`) package.
So if we can make that happen, and can use AdminSplit, that would
be preferrable.

AdminSplit requires them to run a (somewhat) distributed multi-range
transaction. A first split would hit a single range, but after that the
split batch hits at least two ranges (meta2 and splitKey), and so we
need nontrivial DistSender-like functionality. Splits are also
nontrivial distributed transactions and so we need a TxnCoordSender.
Experience suggests that it's better to use the "real thing" and to
make sure it's configurable enough to fit the use case, rather than
whipping up half-baked replacements.

Luckily, it turned out that DistSender and TxnCoordSender are already
up to the task, and this commit adopts them in
`createTestStoreWithoutStart`, and changes `splitTestRange` to use
`AdminSplit`.

Release note: None
tbg added a commit to tbg/cockroach that referenced this issue Nov 4, 2021
@tbg
kvserver: use wrapper type for Store.mu.replicas
d9087af 
This simplifies lots of callers and it will also make it easier to work
on cockroachdb#72374, where this map will start containing more than one type as
value.

Release note: None
craig bot pushed a commit that referenced this issue Nov 4, 2021
@rauchenstein @knz
@rafiss @RichardJCai @tbg
Merge #70330 #72353 #72417 #72430 #72432
ed35bbe 
70330: util/log: add buffer sink decorator r=knz a=rauchenstein

Previously, only the file sink had buffering, and in that case it is
built into the sink.  It's important to add buffering to network sinks
for various reasons -- reducing network chatter, and making the
networking call itself asynchronous so the log call returns with very
low latency.

This change adds a buffering decorator so that buffering can be added to
any log sink with little or no development effort, and allowing
buffering to be configured in a uniform way.

Release note (cli change): Add buffering to log sinks. This can be
configured with the new "buffering" field on any log sink provided via
the "--log" or "--log-config-file" flags.

Release justification: This change is safe because it is a no-op without
a configuration change specifically enabling it.

72353: *: fix improperly wrapped errors r=otan,RaduBerinde,stevendanna a=rafiss

refs #42510

I'm working on a linter that detects errors that are not wrapped
correctly, and it discovered these.

Release note: None

72417: sql: add unit tests for creating default privileges r=ajwerner a=RichardJCai

Adding some unit test coverage so we don't hit bugs like this again.
#72322

Release note: None

72430: kvserver: use wrapper type for Store.mu.replicas r=erikgrinaker a=tbg

This simplifies lots of callers and it will also make it easier to work
on #72374, where this map will start containing more than one type as
value.

Release note: None


72432: roachprod: fix `roachprod start` ignoring --binary flag r=[rail,tbg] a=healthy-pod

Merging #71660 introduced a bug where roachprod ignores --binary
flag when running `roachprod start`.

This patch reverts to the old way of setting config.Binary as a quick solution to the bug.

Release note: None

Fixes #72425 #72420 #72373 #72372

Co-authored-by: Jay Rauchenstein <rauchenstein@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: richardjcai <caioftherichard@gmail.com>
Co-authored-by: Richard Cai <RichardJCai@users.noreply.github.com>
Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
Co-authored-by: Ahmad Abedalqader <ahmad.abedalqader@cockroachlabs.com>
@tbg tbg mentioned this issue Nov 4, 2021
Merged
craig bot pushed a commit that referenced this issue Nov 8, 2021
@tbg
Merge #72383
f29118b 
72383: kvserver: use AdminSplit in splitTestRange r=erikgrinaker a=tbg

`splitTestRange` was previously reaching into the store to finagle a
split. It is used by around a dozen tests. Prototyping around #72374
has shown that these tests frequently need patching up whenever we
adjust (improve) the store's replica handling.

This is a time suck and besides, we also want to be able to test
the Store from within the `kvserver` (not `kvserver_test`) package.
So if we can make that happen, and can use AdminSplit, that would
be preferrable.

AdminSplit requires them to run a (somewhat) distributed multi-range
transaction. A first split would hit a single range, but after that the
split batch hits at least two ranges (meta2 and splitKey), and so we
need nontrivial DistSender-like functionality. Splits are also
nontrivial distributed transactions and so we need a TxnCoordSender.
Experience suggests that it's better to use the "real thing" and to
make sure it's configurable enough to fit the use case, rather than
whipping up half-baked replacements.

Luckily, it turned out that DistSender and TxnCoordSender are already
up to the task, and this commit adopts them in
`createTestStoreWithoutStart`, and changes `splitTestRange` to use
`AdminSplit`.

Release note: None


Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
@tbg
Copy link
Member Author

tbg commented Nov 10, 2021

Have discussed this a bit (slack (internal)):

[...] at this point it's better not to think of "making a separate type for uninited replicas" but rather thinking about disassociating the in-memory state and access to data (Replica) from the underlying raft state machine further (replicaStateMachine ) to the point where we can do things like have uninitialized replicas (i.e. no Replica for the given state) or catch-up on start (where we similarly don't want a Replica yet but just move the disk state forward).

So in a sense replicaStateMachine is our separate type already, and the task at hand is to make it such that we're not carrying around a surrounding *Replica while in uninitialized state (or at least tightly containing in which code paths one can observe uninitialized replicas).

@tbg tbg mentioned this issue Nov 15, 2021
Merged
tbg added a commit to tbg/cockroach that referenced this issue Nov 15, 2021
@tbg
kvserver: document replica lifecycle
25182d3 
Part of the research for cockroachdb#72374, which in turn was inspired by cockroachdb#38322.

Release note: None
tbg added a commit to tbg/cockroach that referenced this issue Nov 16, 2021
@tbg
kvserver: document replica lifecycle
984ef2a 
Part of the research for cockroachdb#72374, which in turn was inspired by cockroachdb#38322.

Release note: None
tbg added a commit to tbg/cockroach that referenced this issue Nov 17, 2021
@tbg
kvserver: document replica lifecycle
89162cb 
Part of the research for cockroachdb#72374, which in turn was inspired by cockroachdb#38322.

Release note: None
craig bot pushed a commit that referenced this issue Nov 17, 2021
@tbg
Merge #72745
6282767 
72745: kvserver: document replica lifecycle r=sumeerbhola a=tbg

Part of the research for #72374, which in turn was inspired by #38322.

Release note: None


Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
craig bot pushed a commit that referenced this issue Nov 18, 2021
@tbg @knz
Merge #72471 #72836
e1c7d6a 
72471: kvserver: fix bugs in & fortify tenant refcounting r=ajwerner a=tbg

This PR fixes a sandwich of two bugs around refcounting the tenant rate limiters and metrics that I found while prototyping around #72374.

We had an accidental early return in `postDestroyRaftMuLocked` that meant that tenant metrics and rate limiters were essentially never released.

We were also continuing to use at least the tenant metrics object after the call to `postDestroyRaftMuLocked` had returned (but note that the above bug meant that we hadn't actually released the ref).

This PR fixes both and adds precautions against regressions of such bugs.

Despite having fixed bugs, I would be cautious about backporting this to 21.2 and 21.1; the bugs here never seem to have caused any problems, and since our day-to-day testing isn't heavy on tenants, it's unclear how reliably we'd be shaking out problems that were previously masked by the bug.

72836: server,sql,kv: various context improvements r=miretskiy,tbg a=knz

Informs #58938.

Connects more async goroutines to the tracer.

Also fixes various defects I introduced in #72638 and #72605.


Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
Co-authored-by: Tobias Grieger <tobias.schottdorf@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@tbg tbg removed their assignment May 31, 2022
@github-actions GitHub Actions
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-kv-replication Relating to Raft, consensus, and coordination. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) no-issue-activity X-stale
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@erikgrinaker @tbg