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.

Sign up for GitHub

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

Jump to bottom

xCluster Replication: Make setting up replication synchronous #8757

Closed
rahuldesirazu opened this issue Jun 3, 2021 · 2 comments
Closed

xCluster Replication: Make setting up replication synchronous #8757

rahuldesirazu opened this issue Jun 3, 2021 · 2 comments
Assignees
@hulien22
Labels
area/cdc Change Data Capture area/docdb YugabyteDB core features

Comments

@rahuldesirazu
Copy link
Contributor

Right now, the API for setting up universe replication is asynchronous (i.e. the API returns success before replication is fully set up). This results in situations where the user thinks setup has succeeded, but has failed under the hood. Once we make this synchronous, the API will cleanly bubble up errors to the caller, which would make both yb-admin and platform (in the future) more usable.
@rahuldesirazu rahuldesirazu added area/docdb YugabyteDB core features area/cdc Change Data Capture labels Jun 3, 2021
@rahuldesirazu rahuldesirazu self-assigned this Jun 3, 2021
@nspiegelberg
Copy link
Contributor

  1. See CatalogManager's CreateTable + IsCreateTableDone functions to understand how to use the existing SetupUniverseReplication async API to add IsSetupUniverseReplicationDone.
  2. Probably want to add something like table->GetCreateTableErrorStatus() to UniverseReplicationInfo so we can return it from IsSetupUniverseReplicationDone on setup failure and display to the ClusterAdminClient.

@hulien22 hulien22 mentioned this issue Jun 24, 2021
Closed
@hulien22 hulien22 assigned hulien22 and unassigned rahuldesirazu Jun 24, 2021
@CharlotteRose CharlotteRose mentioned this issue Jun 29, 2021
Closed
hulien22 added a commit that referenced this issue Jul 12, 2021
@hulien22
[#8757] xCluster Replication: Make setting up replication synchronous
a9bb6e0 
Summary:
Changing `yb-admin setup_universe_replication / alter_universe_replication` to only return once the setup has actually completed or failed.
Adding in `IsSetupUniverseReplicationDone` which gets the universe state and returns that to yb-admin which does the waiting / looping in `WaitForSetupUniverseReplicationToFinish`. Uses `CatalogManager::GetUniverseReplication` to get the state, and determines completeness by either the state becoming `Active` or the universe being marked as failed.
Also adding in a status stored in `UniverseReplicationInfo` which stores the last error that occurred during setup. This status is set in `MarkUniverseReplicationFailed`, and is used to return a better error message to yb-admin.

Test Plan:
```
ybd --cxx-test yb-admin-test_ent --gtest_filter AdminCliTest.TestSetupUniverseReplication
ybd --cxx-test yb-admin-test_ent --gtest_filter AdminCliTest.TestSetupUniverseReplicationFailsWithInvalidSchema
ybd --cxx-test yb-admin-test_ent --gtest_filter AdminCliTest.TestSetupUniverseReplicationFailsWithInvalidBootstrapId
```

Reviewers: rahuldesirazu, bogdan, nicolas

Reviewed By: nicolas

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D12116
@hulien22
Copy link
Contributor

Closed by a9bb6e0

@rahuldesirazu rahuldesirazu mentioned this issue Jul 21, 2021
Closed
hulien22 added a commit that referenced this issue Jul 30, 2021
@hulien22
[#9420] XCluster: Add tests for synchronous alter_universe_replication
d7092ef 
Summary: Synchronous alter_universe_replication was implemented in #8757 with setup_universe_replication but not tested.  Only the `alter_universe_replication add_table` is async, so the original implementation would use an extra RPC for the other commands.  This diff fixes that unnecessary step and adds unit testing.

Test Plan:
```
ybd --cxx-test yb-admin-test_ent --gtest_filter XClusterAlterUniverseAdminCliTest.TestAlterUniverseReplication
```

Reviewers: rahuldesirazu, nicolas

Reviewed By: nicolas

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D12336
hulien22 added a commit that referenced this issue Aug 9, 2021
@hulien22
[Backport 2.6] [#8757] xCluster Replication: Make setting up replicat…
b7bb227 
…ion synchronous

Summary:
Changing `yb-admin setup_universe_replication / alter_universe_replication` to only return once the setup has actually completed or failed.
Adding in `IsSetupUniverseReplicationDone` which gets the universe state and returns that to yb-admin which does the waiting / looping in `WaitForSetupUniverseReplicationToFinish`. Uses `CatalogManager::GetUniverseReplication` to get the state, and determines completeness by either the state becoming `Active` or the universe being marked as failed.
Also adding in a status stored in `UniverseReplicationInfo` which stores the last error that occurred during setup. This status is set in `MarkUniverseReplicationFailed`, and is used to return a better error message to yb-admin.

Original commit: a9bb6e0 / D12226

Test Plan:
Jenkins: rebase: 2.6
```
ybd --cxx-test yb-admin-test_ent --gtest_filter AdminCliTest.TestSetupUniverseReplication
ybd --cxx-test yb-admin-test_ent --gtest_filter AdminCliTest.TestSetupUniverseReplicationFailsWithInvalidSchema
ybd --cxx-test yb-admin-test_ent --gtest_filter AdminCliTest.TestSetupUniverseReplicationFailsWithInvalidBootstrapId
```

Reviewers: rahuldesirazu, bogdan, nicolas

Reviewed By: nicolas

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D12489
hulien22 added a commit that referenced this issue Aug 10, 2021
@hulien22
[Backport 2.6] [#9420] XCluster: Add tests for synchronous alter_univ…
6ea1528 
…erse_replication

Summary:
Synchronous alter_universe_replication was implemented in #8757 with setup_universe_replication but not tested.  Only the `alter_universe_replication add_table` is async, so the original implementation would use an extra RPC for the other commands.  This diff fixes that unnecessary step and adds unit testing.

Original commit d7092ef / D12336

Test Plan:
Jenkins: rebase: 2.6
```
ybd --cxx-test yb-admin-test_ent --gtest_filter XClusterAlterUniverseAdminCliTest.TestAlterUniverseReplication
```

Reviewers: rahuldesirazu, nicolas

Reviewed By: nicolas

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D12526
@bmatican bmatican mentioned this issue Aug 12, 2021
Closed
@hulien22 hulien22 mentioned this issue Sep 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hulien22 hulien22

Labels
area/cdc Change Data Capture area/docdb YugabyteDB core features
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rahuldesirazu @hulien22 @nspiegelberg