rangefeed: remove non-muxrangefeed code #125610
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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. |
|
This change is |
wenyihu6
force-pushed
the
removenonmux
branch
12 times, most recently
from
9eeb56f to
db3d932
Compare
|
The first three commits are from #125665. |
|
Correct me if I'm wrong - we shouldn't need to worry about mixed version tests here since they were covered during the removal process in v24.1 cockroach/pkg/cmd/roachtest/tests/mixed_version_cdc.go Lines 422 to 431 in d8073b5 |
wenyihu6
requested review from
srosenberg and
DarrylWong
and removed request for
a team
wenyihu6
requested review from
dt,
nvanbenschoten,
a team,
srosenberg and
renatolabs
and removed request for
a team,
srosenberg,
DarrylWong and
renatolabs
wenyihu6
force-pushed
the
removenonmux
branch
3 times, most recently
from
66bbd9e to
9bbe559
Compare
In 22.2, mux rangefeeds were introduced to reduce the number of rpc streams from one per replica to one per client for all rangefeeds on a node. In 24.1, to simplify the implementation of cockroachdb#110432, we removed any cluster setting for enabling non-mux rangefeeds, making them always enabled. But we retained the non-mux rangefeed code there for test coverage in mixed-version clusters. Given that we are going to 24.2, we are now more comfortable with removing the tests. This patch completely removes client and server code for non-mux rangefeed. Resolves: cockroachdb#125666 Release note: none
|
@nvanbenschoten (Didn't mean to rush the review) Friendly ping here to ensure this PR isn’t overlooked since it was out of order from the list of PRs I made. |
wenyihu6
requested review from
nvanbenschoten
and removed request for
nvanbenschoten
|
Friendly ping here : ) Lmk if we prefer to bake for a while before merging this pr. |
nvanbenschoten
approved these changes
Jun 18, 2024
There was a problem hiding this comment.
Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @dt)
|
TFTR! bors r=nvanbenschoten |
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this pull request
Jul 1, 2024
Previously, we removed non-mux rangefeed code in cockroachdb#125610. However, that patch forgot to remove non-mux rangefeed metrics. This patch removes these metrics as they are no longer needed. Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this pull request
Jul 2, 2024
Previously, we removed non-mux rangefeed code in cockroachdb#125610. However, that patch forgot to remove non-mux rangefeed metrics. This patch removes these metrics as they are no longer needed. Epic: none Release note: none
wenyihu6
added a commit
to wenyihu6/cockroach
that referenced
this pull request
Jul 2, 2024
Previously, we removed non-mux rangefeed code in cockroachdb#125610. However, that patch forgot to remove non-mux rangefeed metrics. This patch removes these metrics as they are no longer needed. Epic: none Release note: none
craig bot
pushed a commit
that referenced
this pull request
Jul 2, 2024
126352: cli: add fallback query support for debug zip r=xinhaoz a=dhartunian Previously, when SQL queries for dumping tables to debug zip would fail, we would have no follow-up. Engineers can now define "fallback" queries for tables in debug zip in order to make a second attempt with a simpler query. Often we want to run a more complex query to gather more debug data but these queries can fail when the cluster is experiencing problems. This change gives us a chance to define a simpler approach that can be attempted when necessary. In order to define a fallback, there are two new optional fields in the `TableRegistryConfig` struct for redacted and unredacted queries respectively. Debug zip output will still include the failed attempts at the original query along with the error message file as before. If a fallback query is defined, that query will produce its own output (and error) file with an additional `.fallback` suffix added to the base table name to identify it. Resolves: #123964 Epic: CRDB-35278 Release note: None 126354: ui: alter role events render correctly r=xinhaoz a=dhartunian Previously, ALTER ROLE events without role options would render with an "undefined" option in the event log on the DB Console. This change amends the rendering logic to correctly render events without any options. Resolves #124871 Epic: None Release note (bug fix,ui change): ALTER ROLE events in the DB Console event log now render correctly when the event does not contain any role options. 126486: kvserver/rangefeed: remove lockedRangefeedStream r=nvanbenschoten a=wenyihu6 **kvserver: wrap kvpb.RangeFeedEventSink in Stream** Previously, we declared the same interface signature twice: once in kvpb.RangeFeedEventSink and again in rangefeed.Stream. This patch embeds kvpb.RangeFeedEventSink inside rangefeed.Stream, making rangefeed.Stream a superset of kvpb.RangeFeedEventSink. This approach makes sense, as each rangefeed server stream should be a rangefeed event sink, capable of making thread-safe rangefeed event sends. Epic: none Release note: none --- **kvserver/rangefeed: remove lockedRangefeedStream** Previously, we created separate locked rangefeed streams for each individual rangefeed stream to ensure Send can be called concurrently as the underlying grpc stream is not thread safe. However, since the introduction of the mux rangefeed support, we already have a dedicated lock for the underlying mux stream, making the Send method on each rangefeed stream thread safe already. This patch removes the redundant locks from each individual rangefeed stream. Epic: none Release note: none 126487: kvserver/rangefeed: remove non-mux rangefeed metrics r=nvanbenschoten a=wenyihu6 Previously, we removed non-mux rangefeed code in #125610. However, that patch forgot to remove non-mux rangefeed metrics. This patch removes these metrics as they are no longer needed. Epic: none Release note: none 126498: status: fix TestTenantStatusAPI test r=xinhaoz a=dhartunian Previously, this test would use a single connection, cancel it, and then use the connection to verify the cancellation. The test is adjusted here to use two separate sessions, one to cancel for testing, and another to observe the cancellation. Resolves: #125404 Epic: None Release note: None 126524: sql: unskip Insights test r=dhartunian a=dhartunian This test has been flaky for a while because of the async tagging of the TransactionID to the insight that somtimes takes too long to complete. This change removes that check and unskips the test so that we can catch regressions for this feature. In the future we may want to write a separate test to verify the async transactionID tagging separately. Resolves: #125771 Resolves: #121986 Epic: None Release note: None 126533: kv: hook Raft StoreLiveness into storeliveness package r=nvanbenschoten a=nvanbenschoten Fixes #125242. This commit adds a `replicaRLockedStoreLiveness` adapter type to hook the raft store liveness into the storeliveness package. This is currently unused. Release note: None 126536: roachpb: add Leader lease type definition r=nvanbenschoten a=nvanbenschoten Fixes #125225. This commit adds a new `Term` field to the Lease struct. This field defines the term of the raft leader that a leader lease is associated with. The lease is valid for as long as the raft leader has a guarantee from store liveness that it remains the leader under this term. The lease is invalid if the raft leader loses leadership (i.e. changes its term). The field is not yet used. Release note: None Co-authored-by: David Hartunian <[email protected]> Co-authored-by: Wenyi Hu <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 22.2, mux rangefeeds were introduced to reduce the number of rpc streams from
one per replica to one per client for all rangefeeds on a node.
In 24.1, to simplify the implementation of #110432, we removed any cluster
setting for enabling non-mux rangefeeds, making them always enabled. But we
retained the non-mux rangefeed code there for test coverage in mixed-version
clusters. Given that we are going to 24.2, we are now more comfortable with removing
the tests. This patch completely removes client and server code for non-mux
rangefeed.
Resolves: #125666
Release note: none