Plain merge tree partition exports - #2290
arthurpassos wants to merge 52 commits into
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 240da8173c
ℹ️ 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".
| catch (...) | ||
| { | ||
| replicated_error = std::current_exception(); | ||
| tryLogCurrentException(getLogger("InterpreterKillQueryQuery"), | ||
| "KILL EXPORT PARTITION: could not read system.replicated_partition_exports (the WHERE may " | ||
| "reference columns that only exist for plain MergeTree); ignoring ReplicatedMergeTree tables"); |
There was a problem hiding this comment.
Propagate system-table read failures during KILL
If either system-table query fails for an operational reason such as a memory limit, timeout, or internal exception, this broad catch treats it like an engine-specific unknown column and continues with the other table. The command can therefore report successful results while silently leaving matching exports from the failed table running. Only suppress the specific predicate-resolution error that proves the filter belongs exclusively to the other table; propagate all other failures.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
…ya-26.6/export-partition-plain-merge-tree
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8d038d350
ℹ️ 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".
|
Iceberg export partition regression suite is fully passing on no/ice/glue catalogs with plain merge tree tables. I will wait for dev review and the CI run to do a final CI triage to confirm no PR caused failures. |
mkmkme
left a comment
There was a problem hiding this comment.
Overall looks good and worth an approval. I left some comments, I'll let you decide what to do about them
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2662683255
ℹ️ 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".
| return ExportPartitionUtils::compositeKey( | ||
| manifest.partition_id, manifest.destination_database, manifest.destination_table); |
There was a problem hiding this comment.
Preserve or migrate the previous ZooKeeper task keys
After upgrading with an existing replicated export, its ZooKeeper child still uses the old <partition>_<database>.<table> key, while this method now derives the escaped dot-separated key. Although poll reads the old child, the inserted entry is indexed by this new derived key and removeStaleEntries immediately removes it because that key is absent from the ZooKeeper child list; the scheduler therefore cannot resume its remaining parts, and a new export also misses the old path during duplicate detection. Migrate existing nodes or retain the actual ZooKeeper child key in the entry.
Useful? React with 👍 / 👎.
| entry.part_references = std::move(part_references); | ||
| entry.setDescriptor(std::move(descriptor)); | ||
| persist(composite_key, entry.getDescriptor().toJsonString()); | ||
| tasks.insert_or_assign(composite_key, std::move(entry)); |
There was a problem hiding this comment.
Block force replacement while the prior commit is running
When a forced re-export arrives while the existing entry has committing == true, this replacement proceeds even though KILL explicitly refuses that state. killExportPart cannot cancel the destination commit, so the old commit can publish successfully and then find its registry entry replaced, while the new transaction subsequently exports and commits the same partition again; for an Iceberg destination this can produce duplicate rows under two transaction IDs. Reject or defer the replacement until the existing commit lease is released.
Useful? React with 👍 / 👎.
|
@mkmkme shared with me a couple of AI findings, below you can find the assessment of each:
True, but the feature is still marked as experimental. Adding a conversion layer adds complexity, I think it is better to make this a breaking change.
This is complaining about blocks like the below, but the json object is a Poco::Ptr, which has a null check:
Honestly, I think that's the only sane way of handling this. If we can't read it, what can we do about it aside from logging?
Fixed on e21bc07
Force operation is not safe by design
Presented two "issues". The first one is about killing an already completed export because the write to disk has ffailed. It doesn't really have a fix afaict, it is a split brain issue. The case is very narrow. The second one is only applicable to plain exports and is about file already exists policy == error after a re-start with the commit file already present. Before this PR, it would throw and kill the export, even though it has been committed to the destination. This PR now changes it so that it considers it a success without reading any further data. Fixed in f0422be
Fixed by 67bbe0e
This is true, but I would rather not optimize locking and threading until we face an actual performance issue. |
…n-plain-merge-tree
…n-plain-merge-tree
List of changes:
system.partition_exportstable that keeps track of both plain and replicated cases. The oldsystem.replicated_partition_exportsis aliased to this new one for backwards compatibility.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Export partition on plain merge tree. Should close #1908
User facing & functional changes:
system.partition_exportstable that covers both the previousreplicated_partition_exportsand the new one for plain merge treeDocumentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: