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.

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

Extend index tables' partitioning settings to be able to restore them from a backup with the same partitioning as before #5119

Merged
merged 9 commits into from
Jun 21, 2024

Conversation

jepett0
Copy link
Collaborator

@jepett0 jepett0 commented Jun 3, 2024

#4955

There are 3 major commits in this PR, whose purposes are the following:

  • 90162f5 Extend ydb_table.proto to be able to specify partitioning settings of the indexImplTable and explicit partition boundaries (or uniform partition count) of the index table at the moment of its creation. There is also a functional test using GRPC calls that shows that it becomes possible to create a table with extended index partitioning settings.
  • c96c24f Changes in cpp SDK to be able to describe extended partitioning settings and explicit partition boundaries (if any) of secondary indexes of the table. This is necessary to be able to dump and restore the table with the extended index partitioning info.
  • 49321ec Unit tests and functional tests of the feature.

Note: each major commit is preceded by a minor refactoring change. For the best reviewing experience, please review these changes separately.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jepett0 jepett0 force-pushed the SchemeShard.index_backup_restore.1 branch from 4d688de to e5f1b60 Compare June 14, 2024 16:06

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jepett0 jepett0 force-pushed the SchemeShard.index_backup_restore.1 branch from e5f1b60 to e2cb00c Compare June 17, 2024 18:42

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jepett0 jepett0 changed the title Protobuf implementation of alter table add index with extended partitioning settings Extend index tables' partitioning settings to be able to restore them from a backup with the same partitioning as before Jun 18, 2024
@jepett0 jepett0 force-pushed the SchemeShard.index_backup_restore.1 branch from e2cb00c to 49321ec Compare June 19, 2024 11:36

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jepett0
Copy link
Collaborator Author

jepett0 commented Jun 19, 2024

There is a direct leak in one of the added unit tests. However, it is not a new problem. It can be reproduced on a YDB cluster that uses ydbd binary that was built with --sanitize=leak flag by exporting to S3 and then importing back and stopping the server.

I propose to fix the leak in an other PR. I have created a ticket to track the issue.

@jepett0 jepett0 requested review from CyberROFL and ijon June 19, 2024 14:27
@jepett0 jepett0 marked this pull request as ready for review June 19, 2024 14:27
ydb/core/ydb_convert/table_description.cpp Outdated Show resolved Hide resolved
ydb/services/ydb/ydb_index_table_ut.cpp Outdated Show resolved Hide resolved
ydb/core/protos/flat_scheme_op.proto Outdated Show resolved Hide resolved
- proto field naming
- more reasonable uniform partitions count and min partitions count settings in tests
- remove dead code
@jepett0 jepett0 force-pushed the SchemeShard.index_backup_restore.1 branch from 7d3258f to a10ebbe Compare June 20, 2024 11:55
@jepett0 jepett0 requested review from CyberROFL and ijon June 20, 2024 12:07

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

- repeated field naming

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

- They fail under leak sanitizer. Will investigate in scope of another ticket.
Copy link

github-actions bot commented Jun 21, 2024

2024-06-21 12:12:36 UTC Pre-commit check for 112511b has started.
2024-06-21 12:16:08 UTC Build linux-x86_64-release-asan is running...
🟢 2024-06-21 12:47:08 UTC Build successful.
2024-06-21 12:47:21 UTC Tests are running...
🔴 2024-06-21 15:01:55 UTC Some tests failed, follow the links below.

Test history | Test log

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
15500 14868 0 101 360 171

🟡 2024-06-21 15:02:42 UTC ydbd size 5.5 GiB changed* by +423.0 KiB, which is >= 100.0 KiB vs main: Warning

ydbd size dash main: e61d9af merge: 112511b diff diff %
ydbd size 5 854 489 424 Bytes 5 854 922 608 Bytes +423.0 KiB +0.007%
ydbd stripped size 1 255 507 080 Bytes 1 255 576 776 Bytes +68.1 KiB +0.006%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

Copy link

github-actions bot commented Jun 21, 2024

2024-06-21 12:19:30 UTC Pre-commit check for 112511b has started.
2024-06-21 12:23:09 UTC Build linux-x86_64-relwithdebinfo is running...
🟢 2024-06-21 12:55:37 UTC Build successful.
2024-06-21 12:55:47 UTC Tests are running...
🔴 2024-06-21 15:34:26 UTC Some tests failed, follow the links below.

Test history | Test log

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
76212 62892 0 5 13305 10

🟡 2024-06-21 15:35:13 UTC ydbd size 8.3 GiB changed* by +656.3 KiB, which is >= 100.0 KiB vs main: Warning

ydbd size dash main: 247b6a0 merge: 112511b diff diff %
ydbd size 8 960 488 848 Bytes 8 961 160 856 Bytes +656.3 KiB +0.007%
ydbd stripped size 488 356 264 Bytes 488 377 704 Bytes +20.9 KiB +0.004%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

Copy link

github-actions bot commented Jun 21, 2024

2024-06-21 12:31:21 UTC Pre-commit check for 112511b has started.
2024-06-21 12:34:59 UTC Build linux-x86_64-release-clang14 is running...
🟢 2024-06-21 13:15:06 UTC Build successful.

@jepett0
Copy link
Collaborator Author

jepett0 commented Jun 21, 2024

There is a direct leak in one of the added unit tests. However, it is not a new problem. It can be reproduced on a YDB cluster that uses ydbd binary that was built with --sanitize=leak flag by exporting to S3 and then importing back and stopping the server.

I propose to fix the leak in an other PR. I have created a ticket to track the issue.

I decided to remove this test from the PR. I will add it back after I fix the problem with the leak

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants