mbutrovich commented on code in PR #3268:
URL: https://github.com/apache/iceberg-rust/pull/3268#discussion_r4096430826
##########
crates/iceberg/src/arrow/delete_filter.rs:
##########
@@ -168,17 +168,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 description says this comment now explains why the fix works, as
suggested on #2873, but it's the same text as before. It still describes the
outcome without the reason: `notified_owned()` snapshots tokio's
`notify_waiters_calls` counter when it's built, and holding the read lock is
what orders that snapshot before `insert_equality_delete` can call
`notify_waiters()`. Without that, a later refactor that moves
`notified_owned()` out from under the lock looks harmless and brings the hang
back. Could you push the updated wording? Something like:
```suggestion
// 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.
```
--
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]
