kvserver/rangefeed: remove future package and move rangefeed cleanup to stream muxer #126561
Assignees
Labels
A-cdc
Change Data Capture
A-kv-rangefeed
Rangefeed infrastructure, server+client
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
T-cdc
Comments
wenyihu6
added
the
C-enhancement
wenyihu6
changed the title
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 2, 2024
This patch introduces a new StreamMuxer struct to manage server rangefeed streams. Currently, it just takes over the responsibilities as the existing rangefeed completion watcher without changing any existing behaviour. In a future commit, we will move the rangefeed cleanup process to StreamMuxer as well. Part of: cockroachdb#126561 Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 2, 2024
This patch moves the existing active stream management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 2, 2024
This patch moves the existing rangefeed metrics management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 8, 2024
This patch introduces a new StreamMuxer struct to manage server rangefeed streams. Currently, it just takes over the responsibilities as the existing rangefeed completion watcher without changing any existing behaviour. In a future commit, we will move the rangefeed cleanup process to StreamMuxer as well. Part of: cockroachdb#126561 Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 8, 2024
This patch moves the existing active stream management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 8, 2024
This patch moves the existing rangefeed metrics management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 9, 2024
This patch moves the existing active stream management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 9, 2024
This patch moves the existing rangefeed metrics management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 9, 2024
Previously, nodeMetrics methods used value receivers, causing unnecessary copies on each call. This patch changes these methods to use pointer receivers. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 9, 2024
This patch introduces a new StreamMuxer struct to manage server rangefeed streams. Currently, it just takes over the responsibilities as the existing rangefeed completion watcher without changing any existing behaviour. In a future commit, we will move the rangefeed cleanup process to StreamMuxer as well. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 9, 2024
This patch moves the existing active stream management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 10, 2024
Previously, the mux rangefeed completion watcher ignored stream.Send errors and simply returned, allowing the main loop to continue receiving rangefeed requests even after completion watcher completes. This meant that grpc stream continues taking rangefeed requests even after the rangefeed completion watcher shut down. While not currently problematic, this issue gets worse as the stream muxer begins handling additional tasks, such as invoking rangefeed cleanup callbacks. Additionally, the main loop did not watch for context cancellation or the stopper. This patch addresses these issues by ensuring proper error handling in the main loop. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 10, 2024
This patch moves the existing active stream management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 10, 2024
This patch moves the existing rangefeed metrics management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 10, 2024
This patch introduces a new StreamMuxer struct to manage server rangefeed streams. Currently, it just takes over the responsibilities as the existing rangefeed completion watcher without changing any existing behaviour. In a future commit, we will move the rangefeed cleanup process to StreamMuxer as well. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 10, 2024
Previously, the mux rangefeed completion watcher ignored stream.Send errors and simply returned, allowing the main loop to continue receiving rangefeed requests even after completion watcher completes. This meant that grpc stream continues taking rangefeed requests even after the rangefeed completion watcher shut down. While not currently problematic, this issue gets worse as the stream muxer begins handling additional tasks, such as invoking rangefeed cleanup callbacks. Additionally, the main loop did not watch for context cancellation or the stopper. This patch addresses these issues by ensuring proper error handling in the main loop. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 10, 2024
This patch moves the existing active stream management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 10, 2024
This patch moves the existing rangefeed metrics management to StreamMuxer without changing any existing behaviour. The main purpose is to make future commits cleaner. Part of: cockroachdb#126561 Release note: none
craig bot
pushed a commit
that referenced
this issue
Jul 10, 2024
126488: kvserver/rangefeed: introduce stream muxer r=wenyihu6 a=wenyihu6 **server: change nodeMetrics methods to pointer receivers** Previously, nodeMetrics methods used value receivers, causing unnecessary copies on each call. This patch changes these methods to use pointer receivers. Epic: none Release note: none --- **kvserver/rangefeed: handle StreamMuxer.run errors** Previously, the mux rangefeed completion watcher ignored stream.Send errors and simply returned, allowing the main loop to continue receiving rangefeed requests even after completion watcher completes. This meant that grpc stream continues taking rangefeed requests even after the rangefeed completion watcher shut down. While not currently problematic, this issue gets worse as the stream muxer begins handling additional tasks, such as invoking rangefeed cleanup callbacks. Additionally, the main loop did not watch for context cancellation or the stopper. This patch addresses these issues by ensuring proper error handling in the main loop. Part of: #126561 Release note: none --- **kvserver/rangefeed: introduce stream muxer** This patch introduces a new stream muxer struct to manage server rangefeed streams. Currently, it delegates the existing rangefeed completion watcher to stream muxer without changing any existing behavior. The main purpose is to make future commits cleaner. Part of: #126561 Release note: none --- **kvserver/rangefeed: move active streams to stream muxer** This patch delegates the existing active stream management to stream muxer without changing any existing behavior. The main purpose is to make future commits cleaner. Part of: #126561 Release note: none --- **kvserver/rangefeed: move metrics updates to stream muxer** This patch delegates the existing rangefeed metrics management to stream muxer without changing any existing behavior. The main purpose is to make future commits cleaner. Part of: #126561 Release note: none Co-authored-by: Wenyi Hu <[email protected]>
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 11, 2024
Previously, each rangefeed stream setRangeIDEventSink wrapped the underlying muxstream to send to grpc stream. This patch changes the stream to wrap the stream muxer struct instead which itself wraps the underlying muxstream. Note that this patch doesn’t change any existing behavior but only to make future commits cleaner when we change muxer to send data to a buffered stream instead of directly to grpc. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 11, 2024
Previously, each rangefeed stream setRangeIDEventSink wrapped the underlying muxstream to send to grpc stream. This patch changes the stream to wrap the stream muxer struct instead which itself wraps the underlying muxstream. Note that this patch doesn’t change any existing behavior but only to make future commits cleaner when we change muxer to send data to a buffered stream instead of directly to grpc. Part of: cockroachdb#126561 Release note: none
craig bot
pushed a commit
that referenced
this issue
Jul 11, 2024
126489: kvserver/rangefeed: wrap muxer inside setRangeIDEventSink r=nvanbenschoten a=wenyihu6 **kvserver/rangefeed: remove unused cancel** This patch removes the unused field cancel from setRangeIDEventSink. Epic: none Release note: none --- **kvserver/rangefeed: wrap muxer inside setRangeIDEventSink** Previously, each rangefeed stream setRangeIDEventSink wrapped the underlying muxstream to send to grpc stream. This patch changes the stream to wrap the stream muxer struct instead which itself wraps the underlying muxstream. Note that this patch doesn’t change any existing behavior but only to make future commits cleaner when we change muxer to send data to a buffered stream instead of directly to grpc. Part of: #126561 Release note: none --- **kvserver/rangefeed: rename setRangeIDEventSink to perRangeEventSink** This patch renames `setRangeIDEventSink` to `perRangeEventSink` for clarity. Epic: none Release note: none 127011: workload/schemachange: version gate pgvector in more places r=rafiss a=rafiss fixes #126980 Release note: None Co-authored-by: Wenyi Hu <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 11, 2024
Previously, to reduce O(ranges) goroutines and avoid blocking on rangefeed completion, we introduced the future package to manage rangefeed completion by invoking a future callback when an error occurred. This patch replaces that strategy with a new approach. Each registration now uses the embedded `StreamMuxer.DisconnectStreamWithError` method to signal rangefeed completion. StreamMuxer will handle the shutdown logic and additional stream cleanup. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 11, 2024
Previously, to reduce O(ranges) goroutines and avoid blocking on rangefeed completion, we introduced the future package to manage rangefeed completion by invoking a future callback when an error occurred. The future package makes it difficult to justify which goroutine the callback runs on and which locks are being held. This patch introduces a new approach, replacing the future usage. Each registration now uses the embedded StreamMuxer.DisconnectStreamWithError method to signal rangefeed completion. StreamMuxer will manage the shutdown logic and additional stream cleanup. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 17, 2024
Previously, to reduce O(ranges) goroutines and avoid blocking on rangefeed completion, we introduced the future package to manage rangefeed completion by invoking a future callback when an error occurred. The future package makes it difficult to justify which goroutine the callback runs on and which locks are being held. This patch introduces a new approach, replacing the future usage. Each registration now uses the embedded StreamMuxer.DisconnectStreamWithError method to signal rangefeed completion. StreamMuxer will manage the shutdown logic and additional stream cleanup. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 17, 2024
Previously, to reduce O(ranges) goroutines and avoid blocking on rangefeed completion, we introduced the future package to manage rangefeed completion by invoking a future callback when an error occurred. The future package makes it difficult to justify which goroutine the callback runs on and which locks are being held. This patch introduces a new approach, replacing the future usage. Each registration now uses the embedded StreamMuxer.DisconnectStreamWithError method to signal rangefeed completion. StreamMuxer will manage the shutdown logic and additional stream cleanup. Part of: cockroachdb#126561 Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this issue
Jul 17, 2024
Previously, to reduce O(ranges) goroutines and avoid blocking on rangefeed completion, we introduced the future package to manage rangefeed completion by invoking a future callback when an error occurred. The future package makes it difficult to justify which goroutine the callback runs on and which locks are being held. This patch introduces a new approach, replacing the future usage. Each registration now uses the embedded StreamMuxer.DisconnectStreamWithError method to signal rangefeed completion. StreamMuxer will manage the shutdown logic and additional stream cleanup. Part of: cockroachdb#126561 Release note: none
craig bot
pushed a commit
that referenced
this issue
Jul 17, 2024
126490: kvserver/rangefeed: remove future package r=nvanbenschoten a=wenyihu6 Previously, to reduce O(ranges) goroutines and avoid blocking on rangefeed completion, we introduced the future package to manage rangefeed completion by invoking a future callback when an error occurred. The future package makes it difficult to justify which goroutine the callback runs on and which locks are being held. This patch introduces a new approach, replacing the future usage. Each registration now uses the embedded StreamMuxer.DisconnectStreamWithError method to signal rangefeed completion. StreamMuxer will manage the shutdown logic and additional stream cleanup. Part of: #126561 Release note: none Co-authored-by: Wenyi Hu <[email protected]>
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
cc @cockroachdb/cdc |
|
Closing - I will just track everything in #126560. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem? Please describe.
As part of #126560, we should remove the future package usage in kvserver/rangefeed and move the registration disconnect clean up responsibility of each registration to stream muxer to help us get rid of the O(ranges) goroutines in the future.
Jira issue: CRDB-39979
Epic CRDB-37519
The text was updated successfully, but these errors were encountered: