Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d01eb1575f
ℹ️ 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".
| size_t estimatedKeysCount() override { return iterator->estimatedKeysCount(); } | ||
| std::optional<UInt64> getSnapshotVersion() const override { return iterator->getSnapshotVersion(); } | ||
|
|
||
| bool has_concurrent_next() const override { return iterator->has_concurrent_next(); } |
There was a problem hiding this comment.
Keep bucket-splitting iterator serialized
Propagating has_concurrent_next() from the wrapped iterator here allows StorageObjectStorageStableTaskDistributor::getMatchingFileFromIterator to call next() without its mutex, but ObjectIteratorSplitByBuckets::next mutates pending_objects_info via unsynchronized push/pop in IObjectIterator.cpp. When multiple replicas pull tasks concurrently (e.g., distributed reads with cluster_table_function_split_granularity=BUCKET and a concurrent source iterator), this introduces a data race that can corrupt task assignment or crash.
Useful? React with 👍 / 👎.
| [this]() | ||
| [this, thread_group = DB::CurrentThread::getGroup()]() | ||
| { | ||
| ThreadGroupSwitcher switcher(thread_group, "IcebergKeysIterator"); |
There was a problem hiding this comment.
Use a valid ThreadGroupSwitcher thread name length
ThreadGroupSwitcher forwards thread_name to setThreadName, which rejects names longer than 15 bytes; "IcebergKeysIterator" is 19 bytes, so this path deterministically throws and enters the switcher’s exception handler every time. That means the new thread-group setup always takes the error path (with noisy logs and skipped normal switcher teardown logic), so the locking/profile-events fix does not run cleanly.
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex review, please! |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
|
Disclaimer: This review was generated by an AI coding assistant (Cursor, Auto agent). It is not an official human review. Audit summary for PR #1497 (Fix iterator locking) — both commits (d01eb15 + 55cef3c) reviewed. Result: no remaining confirmed defects.
Recommendation: Safe to merge from a concurrency and thread-naming perspective. Ensure any future refactors of |
arthurpassos
left a comment
There was a problem hiding this comment.
LGTM, tho it is sad we had to add yet another mutex
PR #1497 CI Verification ReportSummaryBug fix for issues found in PR #1436 (Improvement locking Iceberg iterator). Three changes:
MasterCI Results85 success, 1 failure, 2 errors, 3 skipped (91 total)
All MasterCI failures are infrastructure-level issues (server init.d failure, CI runner errors). None produced actual test failures related to code changes. The parent commit ( Regression Tests (clickhouse-regression)Iceberg-Specific Regression TestsAll 4 Iceberg regression suites PASSED:
ConclusionPR #1497 is verified. |
|
|
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix issues found in #1436
Documentation entry for user-facing changes
Add
has_concurrent_nextmethod to iterator wrappers.Add separate iterator thread in query thread group to collect ProfileEvents properly.
CI/CD Options
Exclude tests:
Regression jobs to run: