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

New yb-admin command to set the wal retention time of a table #1931

Closed
hectorgcr opened this issue Jul 29, 2019 · 5 comments
Closed

New yb-admin command to set the wal retention time of a table #1931

hectorgcr opened this issue Jul 29, 2019 · 5 comments
Assignees
@SteveXiSong @nspiegelberg
Labels
area/cdc Change Data Capture

Comments

@hectorgcr
Copy link
Contributor

We need a way to manually set the wal retention time of a specific table through yb-admin
@hectorgcr hectorgcr self-assigned this Jul 30, 2019
@hectorgcr hectorgcr added the area/cdc Change Data Capture label Jul 30, 2019
hectorgcr added a commit that referenced this issue Aug 5, 2019
@hectorgcr
#1524: [CDC] Set WAL retention time for all tablets in a table
9c5616e 
Summary:
This diff provides a way to specify the minimum WAL retention time.
A new field wal_retention_secs has been added to AlterTableRequestPB to use the AlterTable mechanism to modify a table's retention time. By doing this, we get the same guarantees that we get when we do other types of modification like table rename, or change of schema.
Once a tablet server receives a request to modify the WAL retention time, it will be saved in its RaftGroupReplicaSuperBlockPB so that it can be remembered in case of restart.

We will provide a yb-admin command in a different diff to set the wal retention time of a table: #1931

Test Plan: New unit test

Reviewers: rahuldesirazu, neha, bogdan

Reviewed By: bogdan

Subscribers: bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D6672
@nspiegelberg nspiegelberg reopened this Oct 19, 2021
@nspiegelberg
Copy link
Contributor

The CLI command for this operation was never written. There is a user-facing API to change wal retention through table_alterer.cc, which is part of the C++ YBClient. We still need to add a yb-admin command to call this API so DBAs don't have to write their own custom clients.

@nspiegelberg nspiegelberg mentioned this issue Oct 19, 2021
Closed
@nspiegelberg
Copy link
Contributor

Requested by Support https://yugabyte.slack.com/archives/C4K1838GL/p1634659938118200

@nspiegelberg
Copy link
Contributor

  1. yb-admin_cli.cc to add CLI command.
  2. YBTableAlterer::SetWalRetentionSecs is the client API to call.
  3. yb-admin-test.cc for the unit test

@rahuldesirazu rahuldesirazu mentioned this issue Jan 6, 2022
Closed
@SteveXiSong SteveXiSong self-assigned this Jan 10, 2022
@SteveXiSong
Copy link
Contributor

Per discussion with Nicolas, this is a user interface to update the WAL retention time and collect the following information.

  1. The first straightforward task is to add it to our yb-admin, which will be used by xCluster project by March. (The backend API has been implemented. )

  2. To make it fully compatible with PG (give a PG user-friendly interface), we may need to study how PG does such operations similarly and implement our user interface in ysqlsh to update metadata like this. Metadata may include:

    • WAL retention time
    • TTL / expiration properties cc @john Meehan
    • tablet splitting properties cc @timur
    • generic rocksdb properties (as we add more / explore existing ones) cc @raghavendra T.K.
    • HRT had some asks for per-table qos / rate limiting

    We can also confirm the PG user interface with @franck Pachot.
    This is a little more ambiguous task and needs some time to research. I will create another issue to track this once the use case is confirmed.

  3. Besides ysql, we might also need user interfaces in ycql as completeness. This will also be tracked in another issue.

Reference discussion in slack: https://yugabyte.slack.com/archives/C027M2T0W8Z/p1641254306045600

SteveXiSong added a commit that referenced this issue Jan 25, 2022
@SteveXiSong
[CDC][#1931] Add cmd in yb-admin to support set WAL retention
a005b3c 
Summary:
For supporting xCluster project, per request, we would like to be able
to set the WAL retention time for a table. The backend API has been implemented.
This diff only add the user interface in yb-admin.

Also, re-organized a few header dependencies of using classes under enterprise.
Before the change, they had an implicit dependency and clangd cannot resolve
them properly.

In a more general user interface, we should consider adding such commands to YSQL and YCQL as well.
However, due to the log concept gaps and ambiguity for the use cases at this point,
we implement it in yb-admin first for the short-term needs of the xCluster project.
These implementations are orthogonal. This is not wasted work if we later decided to
add them to our QL interfaces.

Test Plan:
Manually run:
```./build/latest/bin/yb-admin set_wal_retention_secs ysql.yugabyte films3 999
Set table films WAL retention time to 999 seconds.
```
Observed
` log.cc:1052] T f85a9e457146442dbdcf772d86cfa663 P afa6631489ed44e48e44fa395835ddd7: Setting log wal retention time to 999 seconds ` in logs.

`./build/latest/bin/yb-admin set_wal_retention_secs ysql.yugabyte films3 2147483647` succeeded with max 31bit input.

```
./build/latest/bin/yb-admin set_wal_retention_secs ysql.yugabyte films3 2147483648
Error running set_wal_retention_sec: Invalid argument (yb/tools/yb-admin_client.cc:369): Namespace '2147483648' of type 'ycql' not found
```
failed.

This looks like we have a day 1 bug in processing input larger than max 31bit integer. When that input is large (>31 bit int), the command doesn't recognize them correctly. This should be fixed (at least throw meaningful error message) in a separate diff.

Reviewers: smishra, nicolas

Reviewed By: nicolas

Subscribers: bogdan, ybase, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D14909
@SteveXiSong
Copy link
Contributor

SteveXiSong commented Mar 11, 2022 •
edited
Loading

Close this issue as Item 1 is finished. For items 2 and 3, this is a long-term discussion and we don't have a clear vision so far. Moved the long-term discussion to #11735 for separate tracking.

@SteveXiSong SteveXiSong mentioned this issue Mar 11, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SteveXiSong SteveXiSong

@nspiegelberg nspiegelberg

Labels
area/cdc Change Data Capture
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@SteveXiSong @hectorgcr @nspiegelberg