JeonDaehong commented on code in PR #3435:
URL: https://github.com/apache/iceberg-rust/pull/3435#discussion_r4265047300
##########
crates/iceberg/src/arrow/caching_delete_file_loader.rs:
##########
@@ -271,25 +271,31 @@ impl CachingDeleteFileLoader {
return Self::load_deletion_vector(task,
basic_delete_file_loader).await;
}
- match del_filter.try_start_pos_del_load(task.file_path()) {
- PosDelLoadAction::AlreadyLoaded =>
Ok(DeleteFileContext::ExistingPosDel),
- PosDelLoadAction::WaitFor(notified) => {
- // Positional deletes are accessed synchronously by
ArrowReader.
- // We must wait here to ensure the data is ready
before returning,
- // otherwise ArrowReader might get an empty/partial
result.
- notified.await;
- Ok(DeleteFileContext::ExistingPosDel)
+ loop {
+ match del_filter.try_start_pos_del_load(task.file_path()) {
+ PosDelLoadAction::AlreadyLoaded => {
+ return Ok(DeleteFileContext::ExistingPosDel);
+ }
+ PosDelLoadAction::WaitFor(notified) => {
+ // Positional deletes are accessed synchronously
by ArrowReader.
+ // We must wait here to ensure the data is ready
before returning,
+ // otherwise ArrowReader might get an
empty/partial result.
+ // The wait also ends if that load fails, so check
again.
+ notified.await;
+ }
Review Comment:
Good point. I'd keep the retry. Given that #3433 involves potential
transient failures (e.g., size stats), surfacing the first error would cause
every waiter to fail on something a second attempt could fix.
You're right about the overhead for a file that keeps failing, as K waiters
would retry sequentially. However, storing the error in the state would require
a larger change since Error isn't Clone. I'd prefer to handle this as a
follow-up. Happy to open an issue if you agree.
--
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]
