-
Notifications
You must be signed in to change notification settings - Fork 3.7k
GH-46149: [C++] Opening dataset fails with sshfs-3.7.3 due to F_RDADVISE error #46346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@github-actions autotune |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @thisisnic , minor suggestion here.
cpp/src/arrow/io/caching.cc
Outdated
@@ -188,7 +188,9 @@ struct ReadRangeCache::Impl { | |||
entries = std::move(new_entries); | |||
} | |||
// Prefetch immediately, regardless of executor availability, if possible | |||
return file->WillNeed(ranges); | |||
// As this is optimisation only, failures should not be treated as fatal | |||
ARROW_UNUSED(file->WillNeed(ranges)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather we just ignore IOError but less other errors propagate (they might be indicative of a logic error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, have updated it now.
fee89f2
to
195d032
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, thank you @thisisnic !
Thanks @thisisnic 🥇 .... bummer we missed 20.0.0 a couple of days, 🤞 a patch-release is soon :-) |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit c3855b9. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 35 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Fix a bug where cache optimisation errors prevent files being read
What changes are included in this PR?
No longer treat
WillNeed
errors inReadRangeCache::Cache
as fatalAre these changes tested?
No
Are there any user-facing changes?
No