kvserver/rangefeed: wrap muxer inside setRangeIDEventSink #126489
Conversation
|
This change is |
wenyihu6
changed the title
This patch removes the unused field cancel from setRangeIDEventSink. Epic: none Release note: none
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/server/node.go line 1841 at r2 (raw file):
Send(event *kvpb.MuxRangeFeedEvent) error SendIsThreadSafe() }
I added an interface to abstract the methods of rangefeed.StreamMuxer. We could also just store the actual *rangefeed.StreamMuxer here, but I thought using an interface here makes the purpose of stream muxer in this package clearer. In the future, we will start adding methods like StreamMuxer.DisconnectStreamWithError to remove future package, but it should never need other methods like StreamMuxer.Run. Do you think this makes sense?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:
pkg/server/node.go line 1841 at r2 (raw file):
Previously, wenyihu6 (Wenyi Hu) wrote…
I added an interface to abstract the methods of
rangefeed.StreamMuxer. We could also just store the actual*rangefeed.StreamMuxerhere, but I thought using an interface here makes the purpose of stream muxer in this package clearer. In the future, we will start adding methods likeStreamMuxer.DisconnectStreamWithErrorto remove future package, but it should never need other methods likeStreamMuxer.Run. Do you think this makes sense?
Will there be multiple implementations of this interface? If not, then I don't think the interface is buying us much beyond making the code a little harder to understand and breaking jump-to-definition.
pkg/server/node.go line 1845 at r2 (raw file):
var _ muxer = &rangefeed.StreamMuxer{} // setRangeIDEventSink is an implementation of rangefeed.Stream which annotates
Unrelated to this change, but what do you think of this name? I keep thinking it's a function that "sets a range ID on an event sink". Any appetite to switching to something like rangeBoundEventSink or perRangeEventSink which makes it more clear how the type relates to the other streams?
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
This patch renames `setRangeIDEventSink` to `perRangeEventSink` for clarity. Epic: none Release note: none
There was a problem hiding this comment.
Reviewable status:
pkg/server/node.go line 1841 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Will there be multiple implementations of this interface? If not, then I don't think the interface is buying us much beyond making the code a little harder to understand and breaking jump-to-definition.
No, there will only be one implementation of this interface. Agree, I've removed the interface and only stored the actual struct.
pkg/server/node.go line 1845 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Unrelated to this change, but what do you think of this name? I keep thinking it's a function that "sets a range ID on an event sink". Any appetite to switching to something like
rangeBoundEventSinkorperRangeEventSinkwhich makes it more clear how the type relates to the other streams?
I like perRangeEventSink! I added another commit for this.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status:
|
TFTR! bors r=nvanbenschoten |
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
setRangeIDEventSinktoperRangeEventSinkfor clarity.Epic: none
Release note: none