-
Notifications
You must be signed in to change notification settings - Fork 14
Add query id to system.part_log, system_exports and system.replicated_partition_exports
#1330
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
Conversation
|
I don't think automated tests are required for such a change |
| { | ||
| auto context_copy = Context::createCopy(context); | ||
| context_copy->makeQueryContextForExportPart(); | ||
| context_copy->setCurrentQueryId(manifest.transaction_id); |
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.
You might be wondering why I was setting to transaction_id. Guess what: I don't remember :).
Honestly, I think it is simply because the query id was not being saved in the manifest and it wasn't available, so transaction_id sounded like a good replacement.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a06f236ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ExportReplicatedMergeTreePartitionManifest manifest; | ||
| manifest.transaction_id = json->getValue<String>("transaction_id"); | ||
| manifest.query_id = json->getValue<String>("query_id"); | ||
| manifest.partition_id = json->getValue<String>("partition_id"); |
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.
Handle missing query_id in persisted manifests
This now requires query_id to be present in every persisted manifest JSON. Manifests created by older versions (or any already-queued exports before this change) won’t have that key, so json->getValue<String>("query_id") throws and the export cannot be resumed/inspected after upgrade. That can strand in-flight replicated exports until the manifest is manually fixed. Consider defaulting query_id to transaction_id or an empty string when the key is missing to keep backward compatibility.
Useful? React with 👍 / 👎.
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.
As always, I have to explain: this feature is still very experimental and not used at all. Completely fine to make breaking changes
mkmkme
left a comment
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.
LGTM
ianton-ru
left a comment
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.
LGTM
|
Export part test suite covers system.part_log and system.exports expected table columns. All tests pass. Link to relevant test: https://github.com/Altinity/clickhouse-regression/blob/main/s3/tests/export_part/system_monitoring.py#L381 |
|
Wrote an automated tests that checks every field is present and populated after export partition in |
|
|
|
Summary of Failed Tests and Root Causes
|
|
The CI/CD run is expected to be red, we will monitor the final run in the branch. @arthurpassos please merge this PR. |
I don't have the powers to do it. Perhaps @zvonand |
|
94 jobs are in progress yet |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add query id to
system.part_log,system_exportsandsystem.replicated_partition_exportsDocumentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: