Add query id to system.part_log, system_exports and system.replicated_partition_exports
#1330
Conversation
|
I don't think automated tests are required for such a change |
arthurpassos
added
antalya
port-antalya
| { | ||
| 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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.
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
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: