Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachtest/cdc: add buffered sender cdc_bench #130021

Closed
wants to merge 17 commits into from

Conversation

wenyihu6
Copy link
Contributor

@wenyihu6 wenyihu6 commented Sep 3, 2024

to discuss: we can't use cluster settings to change things without involving buffered registration at the beginning

epic: none

Copy link

blathers-crl bot commented Sep 3, 2024

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@wenyihu6 wenyihu6 force-pushed the cdcbench branch 3 times, most recently from b7b3faf to 00a3cd0 Compare September 3, 2024 17:31
This patch adds RegisterRangefeedCleanUp to BufferedStream interface. Rangefeed
level can register a cleanup callback that will be invoked after
BufferedStream.Disconnect is called. Note that it might not be invoked
immediately during SendBufferedError.

Buffered registration has a dedicated goroutine to do cleanup. Unbuffered
registration does not have a dedicated goroutine, so it relies on BufferedSender
to invoke the callback when it disconnects.

Part of: cockroachdb#129814
Release note: none
This patch moves helper functions to registry_helpers_test.

Part of: cockroachdb#129814
Release note: none
…re tests

This patch refactors helper functions in registry_helpers_test.

Part of: cockroachdb#129814
Release note: none
This patch adds unbufferedRegistration.

UnbufferedRegistration is like BufferedRangefeed but uses BufferedStream to
buffer live raft updates instead of a using buf channel and having a
dedicated per-range per-registration goroutine to volley events to underlying
grpc stream. Instead, there is only one BufferedStream for each incoming
node.MuxRangefeed rpc call. BufferedStream is responsible for buffering and
sending its updates to the underlying grpc stream in a dedicated goroutine
O(node).

Note that BufferedStreamSender is still left unimplemented. This commit doesn't
add any tests for it yet. More tests will be added in future commits of this PR.

Part of: cockroachdb#129814
Release note: none
@wenyihu6 wenyihu6 force-pushed the cdcbench branch 3 times, most recently from a9e0ad3 to cc3a302 Compare September 5, 2024 14:02
This patch adds more test cases for unbufferedRegistration.

Closed: cockroachdb#126560
Release note: none
This patch refactors the error for kvpb.RangeFeedRetryError_REASON_SLOW_CONSUMER
into newRetryErrBufferCapacityExceeded.

Part of: cockroachdb#126560
Release note: none
This patch adds a new data structure eventQueue which is like a queue but uses a
fixed size for the chunked linked list. Each chunk has a fixed size of 4096
elements. This implementation uses sync.Pool to reduce the number of
allocations.

pushBack, popFront, len run in constant time. removeAll runs in linear time with
respect to the number of elements in the queue. This structure is not safe for
concurrent use.

This is for future commits to include the queue in the BufferedSender to buffer
events at the node level.

Part of: cockroachdb#129813
Release note: none
This patch is the last step for reducing long-running O(ranges) goroutines in
kvserver/rangefeed. It changes the BufferedSender to use a queue to buffer
events before forwarding them to underlying grpc stream.

Closed: cockroachdb#129813
Release note: A new cluster setting
`kv.rangefeed.buffered_stream_sender.enabled` can now be used to allow rangefeed
to use buffered sender for all rangefeed feeds instead of buffering events
separately per client per range.
This patch adds capacity to node level buffered sender which will shut down all
registrations if the node level buffer had overflowed.

Part of: cockroachdb#129813
Release note: none

TODO: add a larger test at the kvclient side to make sure error returned here is treated as a restart signal
to discuss: we can't use cluster settings to change things without involving buffered registration at the beginning
@wenyihu6
Copy link
Contributor Author

Closing - as we are splitting things out based on our new integration branch: #125872.

@wenyihu6 wenyihu6 closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants