-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ntpl] Allow XRootD redirection with EOS files #20272
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
base: master
Are you sure you want to change the base?
[ntpl] Allow XRootD redirection with EOS files #20272
Conversation
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 for addressing this!
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 45b2a23. ♻️ This comment has been updated with latest results. |
3e7059b to
ab97475
Compare
0ec1f2f to
4422f5e
Compare
4422f5e to
c6f6051
Compare
The logic to extract the extended attribute 'eos.url.xroot' from a path to an EOS file on a FUSE mount is centralised in a separate header in RIO, with an internal function called GetEOSRedirectedXRootUrl. This function can be called anywhere needed in the rest of ROOT. With this commit, the following places use this functionality: - TFile::Open (done before this commit) - RLoopManager::ChangeSpec (done before this commit) - RRawFile::Create (introduced in this commit) In particular the last means introducing the redirection also for RNTupleReader, which follows a different logic to open the TFile.
c6f6051 to
45b2a23
Compare
|
To the best of my understanding, this PR is simply moving the code that used to live separately in which is the same that triggered #17544 . What's even more worrying is that I don't see the error on every test execution, but sometimes the test also runs without failing. Before merging this PR I need to understand why that happens. It's possible the problem is still there somehow also without the code changes of this PR. |
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.
In principle looks good to me! We should think about how to test it. Extended attributes are file system dependent, so we may need to add some code just for testing, e.g. something that lets us control what GetEOSRedirectedXRootURL returns under unit tests.
| #include <string> | ||
| #include <optional> |
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.
Reorder alphabetically
| if (gEnv->GetValue("TFile.CrossProtocolRedirects", 1) == 1) { | ||
| if (auto xurl = ROOT::Internal::GetXAttrVal(inputURL, "eos.url.xroot")) { | ||
| // Sometimes the `getxattr` call may return an invalid URL due | ||
| // to the POSIX attribute not being yet completely filled by EOS. | ||
| if (inputSV.back() != '/') { | ||
| if (auto baseName = inputSV.substr(inputSV.find_last_of("/") + 1); | ||
| std::equal(baseName.crbegin(), baseName.crend(), xurl->crbegin())) { | ||
| // Ensure the redirected URL actually starts with the XRootD protocol string | ||
| if (ROOT::StartsWith(*xurl, "root://")) { | ||
| return xurl; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
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.
To increase readability, I'd reverse the if statements, like
if (gEnv->GetValue("TFile.CrossProtocolRedirects", 1) != 1)
return std::nullopt;
auto xurl = ROOT::Internal::GetXAttrVal(inputURL, "eos.url.xroot");
if (!xurl)
return std::nullopt;
...
| if (auto baseName = inputSV.substr(inputSV.find_last_of("/") + 1); | ||
| std::equal(baseName.crbegin(), baseName.crend(), xurl->crbegin())) { | ||
| // Ensure the redirected URL actually starts with the XRootD protocol string | ||
| if (ROOT::StartsWith(*xurl, "root://")) { |
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.
The schema can also be xroot://
| // Sometimes the `getxattr` call may return an invalid URL due | ||
| // to the POSIX attribute not being yet completely filled by EOS. |
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.
Hm, that doesn't sound great. Is there a corresponding bug report for EOS?
Fixes #20213
With these changes I can see the redirection happening when reading an RNTuple on EOS from lxplus (going from
ntpl001_staff.roottoroot://eoshome-v.cern.ch//eos/user/v/vpadulan/ntpl001_staff.rootThere might be more places where the redirection needs to happen though.