rangefeed: create catchup iterators eagerly #111045

aliher1911 · 2023-09-21T14:27:59Z

Previously, catchup iterators were created in the main rangefeed processor work loop. This is negatively affecting scheduler based processors as this operation could be slow.
This commit makes iterator creation eager, simplifying error handling and making rangefeed times delays lower.

Epic: CRDB-26372

Fixes: #111060
Fixes: #111040

Release note: None

blathers-crl · 2023-09-21T14:28:03Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-09-21T14:28:09Z

This change is

miretskiy

This is actually quite nice, small and focused. I just have few nits, but @erikgrinaker should have final approval.

pkg/kv/kvserver/rangefeed/processor.go

miretskiy · 2023-09-21T14:47:53Z

pkg/kv/kvserver/replica_rangefeed.go

+		iter, err := rangefeed.NewCatchUpIterator(r.store.TODOEngine(), rSpan.AsRawSpanWithNoLocals(),
+			args.Timestamp, iterSemRelease, pacer)
+		if err != nil {
+			r.raftMu.Unlock()


do we want to keep AssertHeld?

I don't think we want. The sole reason for that was because we passed closure to method that executes it within its worker. But all that work must be done while we are still holding the lock (in this method above). Now everything is done within the method body under the lock, so no point in having the assert.

miretskiy · 2023-09-21T15:09:55Z

pkg/kv/kvserver/rangefeed/processor.go

+// out of container, it is safe to close it regardless of startup success or
+// failure.
+type CatchupIteratorContainer struct {
+	syncutil.Mutex


i'm not sure who we are trying to serialize with.... presumably registration/filter stuff running and racing with some other error encountered elsewhere...

(totally optional/): maybe type CatchupIteratorContainer atomic.Pointer[CatchUpIterator.

Get method is simply swap.

I think if context is cancelled while processor is handling registration request. So I'll be uneasy mutating it from two places. I also think that linter will eventually flake on data race even if that's safe to do.

I love the idea of atomic pointer + swap. I'll change that.

miretskiy · 2023-09-21T15:10:36Z

pkg/kv/kvserver/rangefeed/processor.go

+
+// Get moves iterator out of container. Calling Close on container won't close
+// the iterator after that. Safe to call on empty container.
+func (c *CatchupIteratorContainer) Get() (iter *CatchUpIterator) {


nit: Maybe rename to Detach() or Move() or Release()?

aliher1911

Let me address the nits real quick!

aliher1911 · 2023-09-21T15:22:50Z

pkg/kv/kvserver/rangefeed/processor.go

+// out of container, it is safe to close it regardless of startup success or
+// failure.
+type CatchupIteratorContainer struct {
+	syncutil.Mutex


I think if context is cancelled while processor is handling registration request. So I'll be uneasy mutating it from two places. I also think that linter will eventually flake on data race even if that's safe to do.

aliher1911 · 2023-09-21T15:24:33Z

pkg/kv/kvserver/replica_rangefeed.go

+		iter, err := rangefeed.NewCatchUpIterator(r.store.TODOEngine(), rSpan.AsRawSpanWithNoLocals(),
+			args.Timestamp, iterSemRelease, pacer)
+		if err != nil {
+			r.raftMu.Unlock()


I don't think we want. The sole reason for that was because we passed closure to method that executes it within its worker. But all that work must be done while we are still holding the lock (in this method above). Now everything is done within the method body under the lock, so no point in having the assert.

aliher1911 · 2023-09-21T15:25:42Z

pkg/kv/kvserver/rangefeed/processor.go

+// out of container, it is safe to close it regardless of startup success or
+// failure.
+type CatchupIteratorContainer struct {
+	syncutil.Mutex


I love the idea of atomic pointer + swap. I'll change that.

miretskiy

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @erikgrinaker)

miretskiy · 2023-09-21T19:08:58Z

do we have an epic for this?

…

On Thu, Sep 21, 2023 at 2:28 PM Yevgeniy Miretskiy ***@***.***> wrote: ***@***.**** approved this pull request. *Reviewable <https://reviewable.io/reviews/cockroachdb/cockroach/111045>* status: [image:

] complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 <https://github.com/aliher1911> and @erikgrinaker <https://github.com/erikgrinaker>) — Reply to this email directly, view it on GitHub <#111045 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANA4FVCPREW4QLE2O2T2N7TX3SBOTANCNFSM6AAAAAA5BUXQVQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

erikgrinaker

Did you look at the original motivation for plumbing through the constructor in #69613? I think this is fine, because all the lifecycle events appear to synchronize with raftMu, but I wonder if we're missing something.

pkg/kv/kvserver/rangefeed/scheduled_processor.go

pkg/kv/kvserver/rangefeed/processor.go

aliher1911 · 2023-09-22T12:15:46Z

The original PR you mentioned tried to move iterator creation from registration work loop which is guaranteed to run after current raft lock was released and some data might get through. But it was passing a function instead of constructing iterator upfront. Since all possible locks are held while we are waiting for registration I don't see why we can't create iterator upfront other than trying to avoid its creation on failure path.

Container was trying to do the same, but just handling it on error paths might be better. The only difference that it was handled implicitly by defer and someone didn't start using the iterator.

Moving contruction to registerWithRangefeedRaftMuLocked() seems ugly as we will have to pull limiter callbacks down the stack because they are attached to the interator. So passing iterator down and letting registerWithRangefeedRaftMuLocked() handle closing seem not uglier than passing callback for the sake of iterator creation to me.

erikgrinaker

LGTM, thanks for the simplification.

Moving contruction to registerWithRangefeedRaftMuLocked() seems ugly as we will have to pull limiter callbacks down the stack because they are attached to the interator. So passing iterator down and letting registerWithRangefeedRaftMuLocked() handle closing seem not uglier than passing callback for the sake of iterator creation to me.

Yeah, this is better, thanks.

pkg/kv/kvserver/replica_rangefeed.go

pkg/kv/kvserver/rangefeed/registry.go

pkg/kv/kvserver/rangefeed/registry_test.go

Previously, catchup iterators were created in the main rangefeed processor work loop. This is negatively affecting scheduler based processors as this operation could be slow. This commit makes iterator creation eager, simplifying error handling and making rangefeed times delays lower. Epic: none Release note: None

aliher1911 · 2023-09-22T17:36:41Z

bors r=erikgrinaker

craig · 2023-09-22T18:28:46Z

Build succeeded:

Bazel Essential CI (Cockroach)

miretskiy reviewed Sep 21, 2023

View reviewed changes

aliher1911 force-pushed the rangefeed_eager_iterator branch from 136c17d to d0bad64 Compare September 21, 2023 15:19

aliher1911 commented Sep 21, 2023

View reviewed changes

aliher1911 marked this pull request as ready for review September 21, 2023 15:26

aliher1911 requested a review from a team September 21, 2023 15:26

aliher1911 force-pushed the rangefeed_eager_iterator branch from d0bad64 to 483b6aa Compare September 21, 2023 15:48

aliher1911 self-assigned this Sep 21, 2023

aliher1911 force-pushed the rangefeed_eager_iterator branch 2 times, most recently from 420ad31 to a5d7730 Compare September 21, 2023 16:56

aliher1911 requested review from miretskiy and erikgrinaker September 21, 2023 17:38

miretskiy approved these changes Sep 21, 2023

View reviewed changes

aliher1911 force-pushed the rangefeed_eager_iterator branch 2 times, most recently from f0cfab1 to 3257904 Compare September 22, 2023 09:21

erikgrinaker reviewed Sep 22, 2023

View reviewed changes

pkg/kv/kvserver/rangefeed/scheduled_processor.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/rangefeed/processor.go Outdated Show resolved Hide resolved

aliher1911 force-pushed the rangefeed_eager_iterator branch from 3257904 to cf35678 Compare September 22, 2023 12:06

aliher1911 requested review from erikgrinaker and miretskiy September 22, 2023 12:18

erikgrinaker approved these changes Sep 22, 2023

View reviewed changes

pkg/kv/kvserver/replica_rangefeed.go Show resolved Hide resolved

pkg/kv/kvserver/replica_rangefeed.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/rangefeed/registry.go Show resolved Hide resolved

pkg/kv/kvserver/rangefeed/registry_test.go Outdated Show resolved Hide resolved

aliher1911 force-pushed the rangefeed_eager_iterator branch from cf35678 to b50a6f0 Compare September 22, 2023 14:34

erikgrinaker mentioned this pull request Sep 22, 2023

rangefeed: measure impact of scheduler rangefeeds on system ranges #110344

Closed

aliher1911 force-pushed the rangefeed_eager_iterator branch from b50a6f0 to 54b8a73 Compare September 22, 2023 15:30

craig bot merged commit 235babd into cockroachdb:master Sep 22, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rangefeed: create catchup iterators eagerly #111045

rangefeed: create catchup iterators eagerly #111045

aliher1911 commented Sep 21, 2023 •

edited

Loading

blathers-crl bot commented Sep 21, 2023

cockroach-teamcity commented Sep 21, 2023

miretskiy left a comment

miretskiy Sep 21, 2023

aliher1911 Sep 21, 2023

miretskiy Sep 21, 2023

aliher1911 Sep 21, 2023

aliher1911 Sep 21, 2023

miretskiy Sep 21, 2023

aliher1911 left a comment

aliher1911 Sep 21, 2023

aliher1911 Sep 21, 2023

aliher1911 Sep 21, 2023

miretskiy left a comment

miretskiy commented Sep 21, 2023 via email

erikgrinaker left a comment

aliher1911 commented Sep 22, 2023

erikgrinaker left a comment

aliher1911 commented Sep 22, 2023

craig bot commented Sep 22, 2023

rangefeed: create catchup iterators eagerly #111045

rangefeed: create catchup iterators eagerly #111045

Conversation

aliher1911 commented Sep 21, 2023 • edited Loading

blathers-crl bot commented Sep 21, 2023

cockroach-teamcity commented Sep 21, 2023

miretskiy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliher1911 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

miretskiy left a comment

Choose a reason for hiding this comment

miretskiy commented Sep 21, 2023 via email

erikgrinaker left a comment

Choose a reason for hiding this comment

aliher1911 commented Sep 22, 2023

erikgrinaker left a comment

Choose a reason for hiding this comment

aliher1911 commented Sep 22, 2023

craig bot commented Sep 22, 2023

aliher1911 commented Sep 21, 2023 •

edited

Loading