kvserver: disconnect rangefeeds on AddSSTable
#72724
Conversation
|
This change is |
erikgrinaker
force-pushed
the
addsstable-rangefeed
branch
from
439a6aa to
2428b09
Compare
There was a problem hiding this comment.
Only looked at last commit.
Reviewed 2 of 4 files at r1, 3 of 6 files at r2, 6 of 10 files at r3, 24 of 28 files at r4, 11 of 11 files at r5, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @aliher1911)
erikgrinaker
force-pushed
the
addsstable-rangefeed
branch
from
2428b09 to
435ad17
Compare
|
One thing that bugs me about this is the potential for thrashing. During imports and such we'll often fire off lots of I suppose what saves us is that there shouldn't be any rangefeed consumers over these spans as the table is offline, at least not until we implement streaming cluster-to-cluster replication. |
This patch disconnects rangefeeds on `AddSSTable` application with `REASON_ADDSSTABLE`. This allows consumers to reconnect and run a catchup scan to see the new writes. The rangefeed is only disconnected when `WriteAtRequestTimestamp` is enabled for the `AddSSTable` request. Otherwise the SSTable may write below the closed timestamp, and so a catchup scan may not see the changes anyway. Since we have to disconnect the rangefeed even for `IngestAsWrites`, which does not produce a replicated `AddSSTable` result (there is no SST to link), a new field `ReplicatedEvalResult.DisconnectRangeFeed` is added as a general mechanism to disconnect the rangefeed. This is not used elsewhere, to retain backwards compatibility with 21.2 followers, but `WriteAtRequestTimestamp` requires a 22.1 `MVCCAddSSTable` version gate and can therefore safely rely on this field. Release note: None
erikgrinaker
force-pushed
the
addsstable-rangefeed
branch
from
435ad17 to
6f2891d
Compare
|
I'm going to let this sit for a bit and see how much work it'd be to publish SSTs to the range feed instead, since that's the preferred long-term solution anyway. My hunch is that it won't take much. |
|
See #73487 for proper handling of |
73436: kvserver: add timeouts for rangefeed test cases r=tbg a=erikgrinaker This splits out some test tweaks from #72724, since we're exploring alternative approaches for that PR and may abandon it. Release note: None Co-authored-by: Erik Grinaker <[email protected]>
|
Closing in favor of #73487. |
This patch disconnects rangefeeds on
AddSSTableapplication withREASON_ADDSSTABLE. This allows consumers to reconnect and run acatchup scan to see the new writes.
The rangefeed is only disconnected when
WriteAtRequestTimestampisenabled for the
AddSSTablerequest. Otherwise the SSTable may writebelow the closed timestamp, and so a catchup scan may not see the
changes anyway.
Since we have to disconnect the rangefeed even for
IngestAsWrites,which does not produce a replicated
AddSSTableresult (there is no SSTto link), a new field
ReplicatedEvalResult.DisconnectRangeFeedisadded as a general mechanism to disconnect the rangefeed. This is not
used elsewhere, to retain backwards compatibility with 21.2 followers,
but
WriteAtRequestTimestamprequires a 22.1MVCCAddSSTableversiongate and can therefore safely rely on this field.
Resolves #70434.
Release note: None