changefeedccl: enable frontier quantization #138328

wenyihu6 · 2025-01-06T17:18:55Z

Previously, all checkpoint timestamps were forwarded precisely to the resolved
timestamp. To improve merging of adjacent frontier spans, this patch introduces
a frontier quantization mode. In this mode, resolved timestamps from rangefeeds
are rounded down to the nearest multiple of the quantization granularity.
Rounding down ensures no progress is missed and that we consistently track the
lowest possible resolved timestamp.

Resolves: #125723
Release note: none

cockroach-teamcity · 2025-01-06T17:19:05Z

This change is

asg0451 · 2025-01-06T20:19:03Z

pkg/ccl/changefeedccl/kvfeed/physical_kv_feed.go

@@ -128,7 +130,12 @@ func (p *rangefeed) addEventsToBuffer(ctx context.Context) error {
 				}
 				stop()
 			case *kvpb.RangeFeedCheckpoint:
-				if !t.ResolvedTS.IsEmpty() && t.ResolvedTS.Less(p.cfg.Frontier) {
+				ev := e.RangeFeedEvent


while this is a pretty simple implementation, would you mind investigating how much work it would be to switch to using the higher-level rangefeed client implementation that LDR uses? it already includes an implementation of quantization and more things that may prove useful. see https://github.com/asg0451/cockroach/blob/ed5ed37ea91b04fba061084ba73fe9225c018288/pkg/crosscluster/producer/event_stream.go#L147 &/ talk to me about it

if it's too much work / too incompatible an api, then maybe dont. however i think it would be beneficial if it's low to moderate effort

I will look into the refactoring. Can we try merging this PR anyways since this is pretty straightforward and already up? I will look into the refactoring as part of the follow up work.

I put up #138346 and assigned myself.

thanks. lmk how it goes

I would also be in favor of using the already-built frontier quantization rangefeed client code if it's not too complicated. Maybe you could try prototyping it and seeing how involved it would be?

Ack, I'm exploring this option now and see how much work this turns out to be. I would still prefer to drive this PR forward given this is already up, but I'm actively prototyping for this refactoring.

asg0451

looks good but i have some questions about the tests.

pkg/ccl/changefeedccl/kvfeed/kv_feed_test.go

asg0451 · 2025-01-06T21:42:30Z

pkg/ccl/changefeedccl/kvfeed/kv_feed_test.go

+				require.NoError(t, err)
+			}
+			frontier.Entries(func(sp roachpb.Span, ts hlc.Timestamp) span.OpResult {
+				t.Logf("span: %v, ts: %v\n", sp, ts)


I thought it might be worth leaving it in since it was helpful for debugging test failures wdyt?

🤷 if you think it will be helpful and it's not too spammy we can leave it in

asg0451 · 2025-01-06T21:55:14Z

pkg/ccl/changefeedccl/kvfeed/physical_kv_feed.go

@@ -128,7 +130,12 @@ func (p *rangefeed) addEventsToBuffer(ctx context.Context) error {
 				}
 				stop()
 			case *kvpb.RangeFeedCheckpoint:
-				if !t.ResolvedTS.IsEmpty() && t.ResolvedTS.Less(p.cfg.Frontier) {
+				ev := e.RangeFeedEvent


thanks. lmk how it goes

pkg/ccl/changefeedccl/kvfeed/kv_feed_test.go

pkg/ccl/changefeedccl/kvfeed/physical_kv_feed.go

andyyang890

Nice work! Left some comments.

pkg/ccl/changefeedccl/kvfeed/physical_kv_feed.go

andyyang890 · 2025-01-06T22:47:41Z

pkg/ccl/changefeedccl/kvfeed/physical_kv_feed.go

@@ -128,7 +130,12 @@ func (p *rangefeed) addEventsToBuffer(ctx context.Context) error {
 				}
 				stop()
 			case *kvpb.RangeFeedCheckpoint:
-				if !t.ResolvedTS.IsEmpty() && t.ResolvedTS.Less(p.cfg.Frontier) {
+				ev := e.RangeFeedEvent


I would also be in favor of using the already-built frontier quantization rangefeed client code if it's not too complicated. Maybe you could try prototyping it and seeing how involved it would be?

pkg/ccl/changefeedccl/kvfeed/kv_feed.go

pkg/ccl/changefeedccl/changefeedbase/settings.go

pkg/ccl/changefeedccl/kvfeed/kv_feed_test.go

pkg/ccl/changefeedccl/kvfeed/physical_kv_feed.go

pkg/ccl/changefeedccl/kvfeed/kv_feed_test.go

andyyang890

LGTM modulo one comment

pkg/ccl/changefeedccl/changefeedbase/settings.go

wenyihu6

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @asg0451, @dt, and @rharding6373)

pkg/ccl/changefeedccl/changefeedbase/settings.go

Previously, all checkpoint timestamps were forwarded precisely to the resolved timestamp. To improve merging of adjacent frontier spans, this patch introduces a frontier quantization mode. In this mode, resolved timestamps from rangefeeds are rounded down to the nearest multiple of the quantization granularity. Rounding down ensures no progress is missed and that we consistently track the lowest possible resolved timestamp. Resolves: cockroachdb#125723 Release note: none

wenyihu6 · 2025-01-10T22:25:45Z

TFTRs!

bors r=asg0451,andyyang890

craig · 2025-01-10T22:56:41Z

Build succeeded:

wenyihu6 marked this pull request as ready for review January 6, 2025 17:20

wenyihu6 requested a review from a team as a code owner January 6, 2025 17:20

wenyihu6 requested review from asg0451, rharding6373 and andyyang890 and removed request for a team January 6, 2025 17:20

wenyihu6 force-pushed the frontierquantize branch 2 times, most recently from ecc531b to 8eae6cf Compare January 6, 2025 18:07

asg0451 reviewed Jan 6, 2025

View reviewed changes

wenyihu6 force-pushed the frontierquantize branch from 8eae6cf to d165a8c Compare January 6, 2025 20:37

asg0451 reviewed Jan 6, 2025

View reviewed changes

wenyihu6 force-pushed the frontierquantize branch 2 times, most recently from cd1cba8 to 8bcc47f Compare January 6, 2025 22:16

dt reviewed Jan 6, 2025

View reviewed changes

pkg/ccl/changefeedccl/kvfeed/physical_kv_feed.go Outdated Show resolved Hide resolved

wenyihu6 force-pushed the frontierquantize branch 2 times, most recently from 64820b0 to 3f5a7ef Compare January 6, 2025 22:24

andyyang890 reviewed Jan 6, 2025

View reviewed changes

wenyihu6 force-pushed the frontierquantize branch from 3f5a7ef to bfe3ed5 Compare January 7, 2025 16:32

wenyihu6 requested review from asg0451 and andyyang890 January 7, 2025 16:34

wenyihu6 force-pushed the frontierquantize branch 2 times, most recently from 30a63fc to 592c7b6 Compare January 7, 2025 21:27

asg0451 approved these changes Jan 8, 2025

View reviewed changes

pkg/ccl/changefeedccl/kvfeed/physical_kv_feed.go Show resolved Hide resolved

pkg/ccl/changefeedccl/kvfeed/kv_feed_test.go Outdated Show resolved Hide resolved

wenyihu6 force-pushed the frontierquantize branch from 592c7b6 to 177c499 Compare January 8, 2025 20:20

andyyang890 approved these changes Jan 8, 2025

View reviewed changes

pkg/ccl/changefeedccl/changefeedbase/settings.go Show resolved Hide resolved

wenyihu6 force-pushed the frontierquantize branch 2 times, most recently from 488184b to 69f3484 Compare January 8, 2025 21:37

wenyihu6 commented Jan 8, 2025

View reviewed changes

pkg/ccl/changefeedccl/changefeedbase/settings.go Show resolved Hide resolved

wenyihu6 force-pushed the frontierquantize branch from 69f3484 to b3983bf Compare January 10, 2025 20:59

wenyihu6 mentioned this pull request Jan 10, 2025

kvclient/rangefeed: data race between receiveEventsFromNode and kvfeed #138687

Closed

craig bot merged commit 3cc42e6 into cockroachdb:master Jan 10, 2025
22 checks passed

andyyang890 mentioned this pull request Jan 17, 2025

changefeedccl: quantize timestamps again before making span-level checkpoint #139355

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changefeedccl: enable frontier quantization #138328

changefeedccl: enable frontier quantization #138328

wenyihu6 commented Jan 6, 2025 •

edited

Loading

cockroach-teamcity commented Jan 6, 2025

asg0451 Jan 6, 2025

asg0451 Jan 6, 2025

wenyihu6 Jan 6, 2025

wenyihu6 Jan 6, 2025

asg0451 Jan 6, 2025

andyyang890 Jan 6, 2025

wenyihu6 Jan 7, 2025

asg0451 left a comment

asg0451 Jan 6, 2025

wenyihu6 Jan 6, 2025

asg0451 Jan 8, 2025

asg0451 Jan 6, 2025

andyyang890 left a comment

andyyang890 Jan 6, 2025

andyyang890 left a comment

wenyihu6 left a comment

wenyihu6 commented Jan 10, 2025

craig bot commented Jan 10, 2025

changefeedccl: enable frontier quantization #138328

changefeedccl: enable frontier quantization #138328

Conversation

wenyihu6 commented Jan 6, 2025 • edited Loading

cockroach-teamcity commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asg0451 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyyang890 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyyang890 left a comment

Choose a reason for hiding this comment

wenyihu6 left a comment

Choose a reason for hiding this comment

wenyihu6 commented Jan 10, 2025

craig bot commented Jan 10, 2025

wenyihu6 commented Jan 6, 2025 •

edited

Loading