Add commit info to partition exports table#1832
Conversation
arthurpassos
added
antalya-26.3
port-antalya
|
@codex review |
![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: 0a9022d641
ℹ️ 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".
| LOG_INFO(log, | ||
| "Export transaction {} already committed, skipping re-commit", | ||
| transaction_id); | ||
| return; | ||
| /// Empty commit info: paths produced by the original commit are not recoverable | ||
| /// here without re-reading the snapshot. ExportPartitionUtils::commit only | ||
| /// writes the commit_info znode when this struct is non-empty, so the original | ||
| /// committer's record (if any) is preserved. | ||
| return {}; |
There was a problem hiding this comment.
When isExportPartitionTransactionAlreadyCommitted is true here, the function returns an empty ExportPartitionCommitInfo, which makes ExportPartitionUtils::commit skip writing <export-entry>/commit_info and only flip status to COMPLETED. In the common recovery scenario (snapshot committed previously, then retried before ZooKeeper was updated), this leaves committed_* fields permanently empty and contradicts the new observability behavior/documentation that expects a visible marker for already-committed retries.
Useful? React with 👍 / 👎.
| assert committed_metadata_file == "<committed in a previous run, paths unavailable>", ( | ||
| f"Expected sentinel in committed_metadata_file for already-committed retry, got: {committed_metadata_file!r}" |
There was a problem hiding this comment.
This assertion is inconsistent with the implementation under iceberg_writes_post_publish_throw: that failpoint is ONCE, and the published catch path now returns real file paths (storage_metadata_name, manifest list, manifest file), so committed_metadata_file should be a metadata path, not the "previous run" sentinel. As written, the test will fail despite correct behavior and can block CI.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
AI audit note: This review comment was generated by AI (gpt-5.3-codex). Audit update for PR #1832 (Add commit info to partition exports table): Confirmed defectsMedium: commit_info serialization failure can block COMPLETED transitionImpact: A successful destination commit (Iceberg snapshot or object-storage marker) can remain stuck in Anchor: Trigger: Any exception thrown by Why defect: The new observability side-effect (JSON stringify) is now on the critical path of the state transition; a stringify exception aborts the function before the status update, changing behavior from “commit then mark completed” to “commit then throw”. Fix direction (short): Make commit_info persistence best-effort: wrap Regression test direction (short): Add a failpoint (or targeted fault injection) that throws during commit_info serialization and assert the task still reaches EvidenceSerialization enables stream exceptions: std::string toJsonString() const
{
Poco::JSON::Object json;
json.set("iceberg_metadata_file", iceberg_metadata_file);
// ...
std::ostringstream oss;
oss.exceptions(std::ios::failbit);
Poco::JSON::Stringifier::stringify(json, oss);
return oss.str();
}
if (!destination_commit_info.empty())
{
ExportReplicatedMergeTreePartitionCommitInfoEntry commit_info_entry { /* ... */ };
const std::string commit_info_path = fs::path(entry_path) / "commit_info";
Coordination::Requests ops;
ops.emplace_back(zkutil::makeCreateRequest(commit_info_path, commit_info_entry.toJsonString(), zkutil::CreateMode::Persistent));
ops.emplace_back(zkutil::makeSetRequest(status_path, completed_name, -1));
// ...
if (rc == Coordination::Error::ZOK)
{
LOG_INFO(log, "ExportPartition: Marked export as completed and persisted commit_info");
return;
}
// fall through to status-only set on ZNODEEXISTS / other errors
}Coverage summary
|
| ### Commit info columns | ||
|
|
||
| These columns surface paths produced by the destination storage during commit, so it is possible to inspect what was written without consulting the destination directly: | ||
|
|
There was a problem hiding this comment.
Description for destination_file_paths column is missing.
| /// files and reaching this point, the task still completes via the recovery | ||
| /// path but commit_info will be absent. Recovering commit_info from the | ||
| /// live Iceberg snapshot in that case is a possible future enhancement. | ||
| const std::string status_path = fs::path(entry_path) / "status"; |
There was a problem hiding this comment.
AI thinks that exception in the block below can breaks commit transaction.
There was a problem hiding this comment.
I don't think that is totally true. here's what can happen:
once we commit to iceberg, we need to mark it as completed in zookeeper. If the code that creates the commit info throws, we don't mark it as completed in zookeeper and it remains in pending state. In the next scheduler tick, we'll try to commit it again, and we'll notice it has already been committed. In that case, we just mark it as completed. It won't remain in pending forever as far as I can tell
|
@arthurpassos Looks like build is still failing. Maybe check IcebergMetadata.cpp? |
|
Failing integration tests (new fails in PR) are fixed in other PRs. Regression fails unrelated. Manually tested, no issues. (Auto-tests will be added later.) AI audit returned no major blockers. LGTM |
|
Following integration tests and one stateless test are failing with recent changes (new fails in this PR). |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76f1cd7290
ℹ️ 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".
| if (auto fetched_commit_info = readCommitInfo( | ||
| zk, fs::path(storage.zookeeper_path) / "exports" / key, key, log)) | ||
| { | ||
| it->commit_info = std::move(fetched_commit_info); |
There was a problem hiding this comment.
commit_info reads after transient misses
When the status-change handler sees COMPLETED, readCommitInfo returns nullopt not only when the znode is genuinely absent but also on a transient Keeper read failure. This branch then silently skips the assignment and later stores the entry as COMPLETED, so subsequent polls no longer retry the read because their status_changed check is false; the committed_* columns can remain empty for this in-memory entry until restart even though <export-entry>/commit_info exists. Retry while a completed entry has no local commit_info, or avoid treating a transient miss as final.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54799a7e82
ℹ️ 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".
| local_entry->last_exception_per_replica = std::move(last_exception_per_replica); | ||
| local_entry->destination_file_paths_per_part = std::move(destination_file_paths_per_part); |
There was a problem hiding this comment.
When an existing task is polled, these assignments overwrite the in-memory mirror even when the ZooKeeper reads returned an empty map because tryGetChildren failed or a child disappeared concurrently; the new readDestinationFilePathsPerPart comment explicitly says callers must skip assignment in that case, and readLastExceptionPerReplica has the same lenient behavior. Under a transient Keeper hiccup, a completed export that already had exceptions or destination paths will briefly lose last_exception_per_replica, exception_count, and destination_file_paths from system.replicated_partition_exports, which is the table this change is trying to improve for debugging.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f67e46302
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e30bdeb525
ℹ️ 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".
This reverts commit e30bdeb.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9d0bc9dd1
ℹ️ 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".
| auto destination_file_paths_per_part = readDestinationFilePathsPerPart( | ||
| zk, fs::path(entry_path), key, log); |
There was a problem hiding this comment.
When a local entry is already COMPLETED/FAILED/KILLED, this call still lists /processed and batch-gets every processed part on every manifest-updater poll. Completed export znodes are kept for system.replicated_partition_exports and removal is only done on force-overwrite, so a table with historical exports or large partitions will keep generating O(total processed parts) Keeper reads indefinitely. Please skip this refresh for terminal entries once the in-memory map is populated, while still reading it for new entries and status transitions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I opt for ignoring this optimization for now
|
I am tyring to debug the flaky failures on CICD - not easy tho. |
Surprisingly enough (and with tons of help from AI), it seems like the transaction ids are colliding. Snowflakeid does not seem to be that reliable. |
…kwards compatible
| continue; | ||
| } | ||
|
|
||
| if (response.error != Coordination::Error::ZOK) |
There was a problem hiding this comment.
I am actually re-thinking this err handling. Your opinion would be appreciated. This fetch operation, among others, is done in the background to sync the export manifests. ClickHouse keeps an in-memory list that is a mirror of zookeeper.
When the user queries system.replicated_partition_exports, he queries the in-memory mirror, no zookeeper. If during the background sync the query for part_2 destination file paths fails, with the current error handling mechanism, we'll get something like:
part_1: ['f1.parquet', 'f2.parquet']
part_2: []
part_3: ['f5.parquet']
even though part_2 has already exported files.
At a second glance, it would be better if we could warn the user something went wrong during the background sync and the data might be incomplete.
Initially, I thought of introducing a marker / poison pill:
part_2: ['ERR DURING SYNC']
But I also see a different pattern for other zookeeper backed tables like system.replicas where a there is a column called zookeeper_exception.
The former is easy to implement. The latter requires some sort of refactoring.
There was a problem hiding this comment.
I think option n2 is the best solution, especially if I can flat out the zookeeper schema to reduce the chances of this happening.
At the same time, option n1 is easier to implement and I think I'll go for this one in this iteration. Especially because implementing option n2 could be backwards incompatible.
|
|
||
| auto fetched = readLastExceptionPerReplica( | ||
| zk, fs::path(storage.zookeeper_path) / "exports" / key, key, storage.log.load()); | ||
| zk, fs::path(storage.zookeeper_path) / "exports" / key, key, log); |
There was a problem hiding this comment.
The same path is made in lines 829 and 840 below, better to make it only once.
|
Hmm, failed to build for some reason: https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=1832&sha=a785899938fc7cef4f62ec0774c116f5538f058b&name_0=PR&name_1=Fast%20test Some logs: |
| it->destination_file_paths_per_part = std::move(*destination_file_paths_per_part); | ||
| auto destination_file_paths_per_part = readDestinationFilePathsPerPart( | ||
| zk, export_path, key, log); | ||
| it->destination_file_paths_per_part = std::move(destination_file_paths_per_part); |
There was a problem hiding this comment.
I think there's something broken here? Does destination_file_paths_per_part need to be dereferenced?
5 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add Iceberg metadata file paths and data file paths to the
system.replicated_partition_exportsto improve observability and debugging. Closes #1832Also replaces clickhouse snowflakeid implementation with uuidv4
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: