laskoviymishka commented on code in PR #3435:
URL: https://github.com/apache/iceberg-rust/pull/3435#discussion_r4362919039
##########
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:
I don't feel strongly either way, but I'd prefer surfacing the error, having
more insights in problem always helpful, as I suggested on #2859.
But If there's a good reason to keep the retry, I can be convinced.
--
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]
