crosscluster/logical: enable automatic bidi replication #138297
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}
|
It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
msbutler
force-pushed
the
butler-bidi-3
branch
3 times, most recently
from
62474e0 to
84d69ee
Compare
msbutler
added a commit
to msbutler/cockroach
that referenced
this pull request
Jan 8, 2025
…ine tables This patch is required for cockroachdb#138297, where an LDR stream is created from a source that is offline. The subsequent PR will add test coverage to this. Epic: none Release note: none
msbutler
added a commit
to msbutler/cockroach
that referenced
this pull request
Jan 8, 2025
…ine tables This patch is required for cockroachdb#138297, where an LDR stream is created from a source that is offline. The subsequent PR will add test coverage to this. Epic: none Release note: none
msbutler
force-pushed
the
butler-bidi-3
branch
2 times, most recently
from
3eee766 to
a254d02
Compare
msbutler
added a commit
to msbutler/cockroach
that referenced
this pull request
Jan 8, 2025
…ine tables This patch is required for cockroachdb#138297, where an LDR stream is created from a source that is offline. The subsequent PR will add test coverage to this. Epic: none Release note: none
msbutler
changed the title
|
First commit getting reviewed in #138680 |
|
The stress race failure is likely caused by admission control. In the logs, i see a lot of: |
msbutler
force-pushed
the
butler-bidi-3
branch
from
a254d02 to
1e13926
Compare
msbutler
added a commit
to msbutler/cockroach
that referenced
this pull request
Jan 9, 2025
…ine tables This patch is required for cockroachdb#138297, where an LDR stream is created from a source that is offline. The subsequent PR will add test coverage to this. Epic: none Release note: none
msbutler
force-pushed
the
butler-bidi-3
branch
from
1e13926 to
9b5a8e7
Compare
msbutler
requested review from
dt and
jeffswenson
and removed request for
a team
msbutler
added a commit
to msbutler/cockroach
that referenced
this pull request
Jan 10, 2025
…tables This patch is required for cockroachdb#138297, where an LDR stream is created from a source that is offline. The subsequent PR will add test coverage to this. Epic: none Release note: none
jeffswenson
reviewed
Jan 10, 2025
| @@ -406,6 +406,26 @@ func (p *partitionedStreamClient) CreateForTables( | |||
| return spec, nil | |||
| } | |||
|
|
|||
| func (p *partitionedStreamClient) SetupReverseStream( | |||
There was a problem hiding this comment.
I feel like SetupReverseStream is a bit of an odd man out compared to the other methods on the client. The other methods could be conceivably implemented over something like GRPC. SetupReverseStream on the other hand is accepting a sql command and is basically conn.Exec with a traced name.
I think this would match the pattern better if it took a struct that described the desired stream and internally constructed the statement.
Alternatively, I think this should be named something that makes it clear what it really is: like ExecStatement or even expose the underlying pg connection object and run the statement on that.
There was a problem hiding this comment.
This is a good callout. I'll refactor this to be ExecStatement.
I prefer that over your first suggestion as it's much easier to construct the reverse stream statement during the planning of the original ldr stmt, where we access to the OG parsed statement. Then, since this needs to be called during job execution, i merely need to persist the cmd instead of all the components of command.
| jobID := p.ExecCfg().JobRegistry.MakeJobID() | ||
| var reverseStreamCmd string | ||
| if stmt.CreateTable && options.BidirectionalURI() != "" { | ||
| // TODO: validate URI. |
There was a problem hiding this comment.
Validating the URI is a bit tricky. We really need the peer cluster to validate it.
There was a problem hiding this comment.
We really need the peer cluster to validate it.
Good point. I can plan to pass it as an extra arg to CreateForTables, so the og source can validate it. Sounds reasonable?
craig bot
pushed a commit
that referenced
this pull request
Jan 10, 2025
138765: rowexec: add simple benchmark for index backfills r=rafiss a=rafiss This benchmark is not super realistic, since it runs in memory, but it still should be useful for tracking changes over time. Epic: CRDB-42901 Release note: None 138786: sql: add gauge for object count r=annrpom a=annrpom ### sql: remove redundant metrics Epic: none Release note (backwards-incompatible change): Several metrics are redundant and have been removed. Below is a mapping from removed metrics to an existing, identical metric: ``` [REMOVED] sql.schema_changer.running ---- jobs.schema_change.currently_running [REMOVED] sql.schema_changer.successes ---- jobs.schema_change.resume_completed [REMOVED] sql.schema_changer.retry_errors ---- jobs.schema_change.resume_retry_error [REMOVED] sql.schema_changer.permanent_errors ---- jobs.schema_change.resume_failed ``` --- ### sql: add gauge for object count This patch introduces a metric, `sql.schema_changer.object_count`, that keeps track of the count of descriptors in the cluster. Epic: none Fixes: #134740 Release note (ops change): This patch introduces a metric, `sql.schema_changer.object_count`, that keeps track of the count of objects in the cluster. 138823: sql/externalcatalog: allow ExtractInternalCatalog to include offline tables r=fqazi a=msbutler This patch is required for #138297, where an LDR stream is created from a source that is offline. The subsequent PR will add test coverage to this. Epic: none Release note: none Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Annie Pompa <[email protected]> Co-authored-by: Michael Butler <[email protected]>
This patch introduces two options to CREATE LOGICALLY REPLICATED TABLES:
- UNIDIRECTIONAL: to specify a unidectional ldr stream with a fast offline scan
- BIDIRECTIONAL ON {uri}: which customers will use to set up a bidirectional
stream with one command. The uri provided by the customer connects to the
source of the reverse stream, aka the destination of the main stream.
The PARENT option will be used internally to set up the reverse stream.
Specifically, when the destination sets up the reverse stream, it will pass its
jobID as the parent for the reverse stream, which the reverse stream will use
to prevent creating duplicate reverse streams and to configure a stream that
replicates from an offline source. Customers should not directly use it.
Epic: none
Release note: none
This destination will use this api to set up a reverse stream from a cursor. Epic: none Release note: none
For mysterious resons, the resolver cannot fetch offline descriptors unless the mutable flag is passed. Epic: none Release note: none
This patch allows a user to create an LDR stream that replicates from an offline table. Epic: none Release note: none
This patch enables a user to create a bidirectional replication stream with one
command. When the user runs `CREATE LOGICALLY REPLICATED TABLE tabB FROM tabA
ON uriA WITH BIDIRECTIONAL ON uriB`, the following will happen:
- on cluster B, during job planning, construct the reverse stream cmd, while we
still have access to original LDR cmd. This command will have the following
form: `CREATE LOGICAL REPLICATION STREAM FROM TABLE tabB ON 'uriB' INTO TABLE
tabA WITH CURSOR = $1, PARENT = '{og job id}';`
- on cluster B, create table tabB and begin an offline scan, as normal
- on cluster B, once the offline scan completes, but before tabB is published,
set up the reverse stream from B to A at a cursor time after the initial scan
completes but before the tabB has come online. This cursor time prevents data
looping and ensures the replication of all future foreground writes.
- on cluster B, steady state replication begins.
Epic: none
Release note (sql change): when a user runs CREATE LOGICALLY REPLICATED TABLE,
they must specify one of the following options:
- UNIDIRECTIONAL: setups a unidirectional stream with fast initial scan
- BIDIRECTIONAL ON {dest uri}: sets up a bidirectional stream from the original
dest to the original source.
msbutler
force-pushed
the
butler-bidi-3
branch
from
9b5a8e7 to
15876c2
Compare
|
TFTR! bors r=jeffswenson |
This was referenced Jan 17, 2025
Merged
Merged
Merged
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.
This patch enables a user to create a bidirectional replication stream with one
command. When the user runs
CREATE LOGICALLY REPLICATED TABLE tabB FROM tabA ON uriA WITH BIDIRECTIONAL ON uriB, the following will happen:still have access to original LDR cmd. This command will have the following
form:
CREATE LOGICAL REPLICATION STREAM FROM TABLE tabB ON 'uriB' INTO TABLE tabA WITH CURSOR = $1, PARENT = '{og job id}';set up the reverse stream from B to A at a cursor time after the initial scan
completes but before the tabB has come online. This cursor time prevents data
looping and ensures the replication of all future foreground writes.
Epic: none
Release note (sql change): when a user runs CREATE LOGICALLY REPLICATED TABLE,
they must specify one of the following options:
dest to the original source.