storage: delete ReplicatedEvalResult.BlockReads #43048
Conversation
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/removeBlockReads
branch
from
5a02b2d
to
c46d016
Compare
There was a problem hiding this comment.
Everything LGTM except the remove of readOnlyCmdMu, where I share your concerns and also your suspicion that today's code isn't correct.
Could the latch manager help? If before destruction we "poison" the latch mgr (i.e. let it "refuse" new latches) and atomically push through a latch that covers everything (i.e. flush out any pending requests), we should be good to go, right? Maybe this is a decent stand-in for decent replica lifecycle management.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 4 of 4 files at r3, 4 of 4 files at r4, 4 of 4 files at r5.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @irfansharif, and @nvanbenschoten)
pkg/storage/replica_application_state_machine.go, line 984 at r4 (raw file):
defer unlock() } if cmd.replicatedResult().BlockReads {
Add something to the commit message on why it's safe to remove this without a migration: even though replicated, it really only ever mattered on the proposer node (i.e. it never needed to be replicated in the first place).
nvanbenschoten
force-pushed
the
nvanbenschoten/removeBlockReads
branch
from
c46d016
to
b863ce0
Compare
nvanbenschoten
changed the title
There was a problem hiding this comment.
where I share your concerns and also your suspicion that today's code isn't correct.
Yeah, this is pretty troubling. I don't think it will be too hard to confirm or disprove. I'll take a shot at doing so.
Could the latch manager help? If before destruction we "poison" the latch mgr (i.e. let it "refuse" new latches) and atomically push through a latch that covers everything (i.e. flush out any pending requests), we should be good to go, right? Maybe this is a decent stand-in for decent replica lifecycle management.
I think you're on to something here. I would love it if we could use the latch mgr as the single synchronization primitive for a replica and not have other random locking. My concern would just be that it's not re-entrant and there might be cases where we would need it to be.
I'll continue trying to kill off the readOnlyCmdMu, but I'm going to pull that commit for now so that I can push the rest of this through since it's blocking some other cleanup.
bors r+
Reviewable status:
pkg/storage/replica_application_state_machine.go, line 984 at r4 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Add something to the commit message on why it's safe to remove this without a migration: even though replicated, it really only ever mattered on the proposer node (i.e. it never needed to be replicated in the first place).
Done.
Merge conflict |
The decision to block reads on lease transfers has a long history. All lease changes began blocking reads in 2896b8c. This was later reduced to just lease transfers (not extensions) in 231089c. The reason for blocking reads on lease transfers was that "if we didn't block concurrent reads, there'd be a chance that reads could sneak in on a new lease holder between setting the lease and updating the low water mark [of the timestamp cache]." As the corresponding TODO mentions, it's not clear that this would actually cause any issues. It's actually much more likely that it would cause issues for writes, which actually consult the timestamp cache. However, none of this is a concern in practice because `Replica.leasePostApply` updates the timestamp cache before updating the Replica's lease, so such a race is not possible. This has been the case since c54d9b0. This commit addresses the TODO and stops blocking reads on lease transfers. Release note: None
This commit stops using the BlockReads flag to block reads during the application of a MergeTrigger. There was no need to block reads during this time. Reads to the RHS of the range will be rejected up until the call to Replica.setDesc in Store.MergeRange, at which point they will be accepted without issue. The use of BlockReads here dates back to 2896b8c, where it was added in with a reference to cockroachdb#3148 (fixed by 34b2037). The referenced issue has to do with splits and not merges. However, the call to BlockReads was carried over to both the SplitTrigger and MergeTrigger accidentally when the single EndTransaction CommitTrigger handler was split in 2896b8c. Release note: None
Fixes cockroachdb#32583. This commit uses non-MVCC latches introduced in cockroachdb#39765 to properly synchronize reads to the RHS of a split and the split itself. Specifically, this addresses the concern cockroachdb#32583 about reads below the split timestamp being able to read from the the RHS of the split even after the RHS is governed by a new timestamp cache (cockroachdb#3148) and even after it has allowed that data to be rebalanced away. These concerns had previously been addressed by using the BlockReads flag and synchronizing with reads through the readOnlyCmdMu. Now that latching gives us what we need, we can stop setting the BlockReads flag on the SplitTrigger. Release note (performance improvement): Range splits are now less disruptive to foreground reads.
Now that it's not used for lease transfers, merges, or splits, it's no longer used at all. The field is safe to get rid of (ignore) without a migration, even though it is replicated, because it only ever mattered on the proposer node. In other words, it never needed to be replicated in the first place. Release note: None
We can't quite remove the readOnlyCmdMu because of concerns with replica destruction (see PR discussion). However, the prior cleanup allows us to remove the awkward requirements around how the readOnlyCmdMu interacted with the timestamp cache and needed to interleave its locking with the latch manager. This enabled further cleanup. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/removeBlockReads
branch
from
b863ce0
to
645982e
Compare
|
bors r+ |
43048: storage: delete ReplicatedEvalResult.BlockReads r=nvanbenschoten a=nvanbenschoten Closes #32583. This series of commits phases out and deleted `ReplicatedEvalResult.BlockReads`. In doing so, it addresses the remainder of #32583. Most of the effort here was in digging through the history of BlockReads and convincing myself that these changes were sufficient to address the synchronization concerns that led to its creation. The individual commit messages do their best to explore this archaeology. Once that was complete, the rest of the change was straightforward. This is about four cleanups deep in a chain of refactoring that's clearing the way for the new `concurrency` package, so I'm hoping to push this through over the next few days. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
Closes #32583.
This series of commits phases out and deleted
ReplicatedEvalResult.BlockReads. In doing so, it addresses the remainder of #32583.Most of the effort here was in digging through the history of BlockReads and convincing myself that these changes were sufficient to address the synchronization concerns that led to its creation. The individual commit messages do their best to explore this archaeology. Once that was complete, the rest of the change was straightforward.
This is about four cleanups deep in a chain of refactoring that's clearing the way for the new
concurrencypackage, so I'm hoping to push this through over the next few days.