mbutrovich commented on code in PR #2873: URL: https://github.com/apache/iceberg-rust/pull/2873#discussion_r3629675600
########## crates/iceberg/src/arrow/delete_filter.rs: ########## Review Comment: Also line 112 (`try_start_eq_del_load`) above. The loader claims the entry with `try_start_eq_del_load`, which inserts `EqDelState::Loading(notify_A)` and returns `notify_A`. The caller drops `notify_A` unused and calls `insert_equality_delete`, which inserts a second `EqDelState::Loading(notify_B)` over the same key and only ever calls `notify_waiters()` on `notify_B`. A reader that observed the entry while it held `notify_A` waits on a notifier that is never signalled. This is the same class of bug this PR fixes, and it survives the fix. It was flagged on #2859: https://github.com/apache/iceberg-rust/pull/2859#discussion_r3629462979 The window is small today (the two calls run back to back with no `.await` between them, and readers run after the load phase), so this is latent rather than active. Since it is the same root cause and the same file, closing it here rather than in a follow-up keeps the delete-loading paths consistent. `insert_equality_delete` can reuse the notifier returned by `try_start_eq_del_load` instead of minting and inserting a new one. ########## crates/iceberg/src/arrow/delete_filter.rs: ########## @@ -163,17 +163,22 @@ impl DeleteFilter { &self, file_path: &str, ) -> Option<Predicate> { - let notifier = { + // Create the `Notified` while holding the read lock. The read lock ensures that + // when we go inside it, either the state is already at Loaded or it is still at + // Loading AND `notify_waiters()` has not been called yet. Any `Notified` created + // before the invocation of `notify_waiters()` will be notified by it even if + // `await` has not been called on it yet. Review Comment: The new comment explains the outcome (a `Notified` created before `notify_waiters()` still fires) but not the reason. The reason is specific: `notified_owned()` records tokio's internal `notify_waiters_calls` counter at construction, and on its first poll it completes if that counter has advanced. Holding the read lock is what guarantees the counter is read before the loader can advance it. Stating that keeps the code safe against a later refactor that moves `notified_owned()` out from under the lock, which would look harmless but would reintroduce the hang. The same point was made on #2859: https://github.com/apache/iceberg-rust/pull/2859#discussion_r3629462974 Suggested wording: ``` // Build the `Notified` while holding the read lock. `notified_owned()` records tokio's // `notify_waiters_calls` counter at construction and completes on first poll if that // counter has since advanced. Reading the counter under the lock guarantees it is taken // before `insert_equality_delete` can advance it via `notify_waiters()`, so the // notification is never missed even though we `.await` after releasing the lock. ``` ########## crates/iceberg/src/arrow/delete_filter.rs: ########## Review Comment: Not a blocker, just raising it while we are here. On the error path, `parse_equality_deletes_record_batch_stream(...).await?` returns before `sender.send(predicate)`, dropping the sender. The spawned task's `eq_del.await.unwrap()` then panics on `RecvError`, so the `Loaded` insert and `notify_waiters()` never run and the entry stays `Loading`. Within one `load_deletes` call this looks contained: the error propagates via `item?` at `caching_delete_file_loader.rs:214`, the call returns `Err`, and siblings are cancelled rather than parked. It looks like a real hang only if two `load_deletes` calls share one `DeleteFilter` (concurrent scans on the same loader): the first marks the file `Loading`, fails, never signals, and a second reader parks forever. Two questions: is a shared `DeleteFilter` across concurrent loads a supported case? If so, would recording a failure state (or removing the entry so a reader retries) be preferable to leaving it `Loading` and panicking? Same shape as the positional path on #2859: https://github.com/apache/iceberg-rust/pull/2859#discussion_r3629462968. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
