kv: teach DistSender about RangeFeeds, use for changefeeds#28912
kv: teach DistSender about RangeFeeds, use for changefeeds#28912craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
This change is |
danhhz
left a comment
There was a problem hiding this comment.
changefeedccl changes LGTM
Reviewable status:
complete! 0 of 0 LGTMs obtained
9229220 to
4e1c025
Compare
|
@tschottdorf the DistSender changes here are ready to review (commits 1 and 2). The unit testing isn't really sufficient, but I wasn't planning on spending a ton of time on that yet. I've done a decent amount of manual testing and the end-to-end testing that we'll get once #28974 goes in and we can hook this up to the changefeed unit tests and roachtests should be solid. @danhhz I adjusted the poller to perform a starting scan as we discussed. It uses an |
tbg
left a comment
There was a problem hiding this comment.
The DistSender changes LGTM, though if you wanted to split that out into its own file I would be for it.
Reviewed 4 of 4 files at r1, 6 of 6 files at r2.
Reviewable status:
pkg/kv/dist_sender.go, line 1056 at r2 (raw file):
descKey = rs.Key } // TODO(nvanbenschoten): shouldn't we be passing an eviction token here?
You mean from the last iteration?
pkg/kv/dist_sender.go, line 1536 at r2 (raw file):
desc *roachpb.RangeDescriptor, eventCh chan<- *roachpb.RangeFeedEvent, ) (hlc.Timestamp, *roachpb.Error) {
Explain the timestamp returned.
The previous contract for `rangefeed.Stream.Send` was that its events were unsafe for use after it returned, mandating a clone if they were stored somewhere. This added extra complication and will force local RangeFeed rpcs to perform clones even when they're not necessary. Remove this "optimization" and strengthen the contract. Release note: None
This commit adds a RangeFeed method to DistSender and teaches it how to split rangefeeds across ranges and how to manage their individual lifecycle events. Release note: None
This commit adjusts the changefeed poller to use RangeFeed requests when the corresponding cluster setting is enabled. Release note: None
4e1c025 to
c817605
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTR!
though if you wanted to split that out into its own file I would be for it.
Done.
Reviewable status:
pkg/kv/dist_sender.go, line 1056 at r2 (raw file):
Previously, tschottdorf (Tobias Schottdorf) wrote…
You mean from the last iteration?
Yes, exactly. I filed an issue about this (#28967). Linked in the comment.
pkg/kv/dist_sender.go, line 1536 at r2 (raw file):
Previously, tschottdorf (Tobias Schottdorf) wrote…
Explain the timestamp returned.
Done.
danhhz
left a comment
There was a problem hiding this comment.
Reviewable status:
|
bors r=tschottdorf,danhhz |
Build failed (retrying...) |
Build failed (retrying...) |
|
Flake in bors r- |
|
bors r+ |
Build failed (retrying...) |
28912: kv: teach DistSender about RangeFeeds, use for changefeeds r=nvanbenschoten a=nvanbenschoten This change adds a RangeFeed method to DistSender and teaches it how to split rangefeeds across ranges and how to manage their individual lifecycle events. It then adjusts the changefeed poller to use RangeFeed requests when the corresponding cluster setting is enabled. 28975: storage: Track renewable expiration-based leases in map instead of chan r=a-robinson a=a-robinson As discussed on #28931 Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com>
Build succeeded |
28974: storage: hook closed timestamps into rangefeed r=nvanbenschoten a=nvanbenschoten Closes #28912. This change hooks up closed timestamps into rangefeed. It does so by creating a new closed timestamp subscription and notifying all ranges with active rangefeed when new closed timestamp entries arrive. This triggers each rangefeed to check for the maximum closed timestamp on its range and use this to attempt to move its resolved timestamp forward. Eventually this will need some testing tied in with the closedts package. That will have to wait until the closedts package is set up to be more easily used in tests from the storage package (right now `NoopContainer`s are used in tests everywhere). For now, we can wait for integration testing with cdc. With this change and the changes it depends on, rangefeeds are sufficiently feature-complete to be used for changefeeds. For instance, with a few small tweaks, we pass 6 changefeed unit tests in `./pkg/ccl/changefeedccl` with `kv.rangefeed.enabled` set to true. Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
29500: release-2.1: backport 7 rangefeed PRs r=nvanbenschoten a=nvanbenschoten Backport: * 5/5 commits from "kv: give Transport a haircut" (#28855) * 1/1 commits from "engine: use Txn.Timestamp instead of OrigTimestamp for LogLogicalOp" (#28970) * 1/1 commits from "rangefeed: add release valve to logical op consumption" (#29076) * 3/3 commits from "kv: teach DistSender about RangeFeeds, use for changefeeds" (#28912) * 1/1 commits from "rangefeed: small perf-related changes" (#29134) * 2/2 commits from "kv: truncate RangeFeed span to range descriptor " (#29219) * 2/2 commits from "storage: hook closed timestamps into rangefeed" (#28974) Please see individual PRs for details. All cherry-picks were clean. /cc @cockroachdb/release Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
This change adds a RangeFeed method to DistSender and teaches it how
to split rangefeeds across ranges and how to manage their individual
lifecycle events. It then adjusts the changefeed poller to use RangeFeed
requests when the corresponding cluster setting is enabled.