-
Notifications
You must be signed in to change notification settings - Fork 724
[incr-backup] Smoke test for full restore with simple schema change #26840
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
base: main
Are you sure you want to change the base?
Conversation
🟢 |
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Renamed to [incr-backup] as it is like tag for a feature in general, even if this particular test covers only full-backup functionality inside this bigger feature |
def _create_backup_collection(self, collection_src, tables: List[str]): | ||
# create backup collection referencing given table full paths | ||
table_entries = ",\n".join([f"TABLE `/Root/{t}`" for t in tables]) | ||
sql = f""" | ||
CREATE BACKUP COLLECTION `{collection_src}` | ||
( {table_entries} ) | ||
WITH ( STORAGE = 'cluster', INCREMENTAL_BACKUP_ENABLED = 'false' ); | ||
""" | ||
res = self._execute_yql(sql) | ||
stderr_out = "" | ||
if getattr(res, 'std_err', None): | ||
stderr_out += res.std_err.decode('utf-8', errors='ignore') | ||
if getattr(res, 'std_out', None): | ||
stderr_out += res.std_out.decode('utf-8', errors='ignore') | ||
assert res.exit_code == 0, f"CREATE BACKUP COLLECTION failed: {stderr_out}" | ||
self.wait_for_collection(collection_src, timeout_s=30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to extract this to common base class to avoid repetition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same applies to other "common" reusable methods as well)
res = self._execute_yql(cmd) | ||
assert res.exit_code == 0, "Failed to apply GRANT in wave 2" | ||
|
||
# capture state after wave 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this purpose usually word "stage" used
try: | ||
session.execute_scheme('DROP TABLE `/Root/extra_table_1`;') | ||
except Exception: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should explicitly state expected behavior. For now we expect it to fail (even though it is not failing actually). So write test that actually fails on it and mark it as muted like it is done here https://github.com/ydb-platform/ydb/pull/26845/files) and create issue (e.g. Incremental backup must forbid removal of managed objects)
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
# remove some tables from step5: drop extra_table_1 | ||
try: | ||
session.execute_scheme('DROP TABLE `/Root/extra_table_1`;') | ||
except Exception: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MUST fail for t1
and t2
until they are under backup collection
. We must allow dropping tables only when backup collection referencing this tables dropped. (Or maybe we need to reconsider this semantics)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, make assert here and create issue, please
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Changelog entry
[full-backup] Smoke test for full restore with simple schema change
...
Changelog category
Description for reviewers
[full-backup] Smoke test for full restore with simple schema change
...