Add query id to system.part_log, system_exports and system.replicated_partition_exports#1330
Add query id to system.part_log, system_exports and system.replicated_partition_exports#1330MyroTk merged 3 commits intoantalya-25.8from
system.part_log, system_exports and system.replicated_partition_exports#1330Conversation
|
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.
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.
💡 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.
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.
As always, I have to explain: this feature is still very experimental and not used at all. Completely fine to make breaking changes
|
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: