-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix chaining in rules and unify prefix and rules of protocols #42530
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42530/36543
|
A new Pull Request was created by @nhduongvn for master. It involves the following packages:
@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Just to state early, this PR needs to be backported to
For earlier release cycles this PR needs to be included in the backports of #37278 (which I already added in the list in #37278 (comment)) |
Could you add tests that show and ensure the chaining is working properly? |
FWCore/Catalog/src/FileLocator.cc
Outdated
@@ -1,6 +1,5 @@ | |||
#include "FWCore/Catalog/interface/FileLocator.h" | |||
#include "FWCore/ServiceRegistry/interface/Service.h" | |||
#include "FWCore/Utilities/interface/Exception.h" |
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.
Looks like this #include
should be kept.
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.
Done
I tested with the file that user in the CMStalk post above reported that it can't be opened at T2_DE_DESY: Error in TNetXNGFile::ReadBuffer: [ERROR] Server responded with an error: Following these steps
(The path is translated correctly. The "/pnfs/..." which is resulted from path translation of protocol "pnfs" chained from protocol "dcap" is included)
(The config is https://drive.google.com/file/d/1sA35yXvVDa6vizKGkYGpDfmpXqwLNfC9/view?usp=sharing) The file can be opened (If run with a MINIAOD file "/store/data/Run2016C/DoubleMuon/MINIAOD/17Jul2018-v1/00000/02B5E9A5-878D-E811-832D-0CC47A545096.root". There is no crash. The above file is NANOAOD.) |
For our record, here is email from Stephan on how chaining works: Hallo Duong, "protocol": "first" "protocol": "second" "protocol": "root" then /store/mc/xxx of protocol root should translate and /test/aaa/xxx of protocol root should translate (I hope i got things right. It's still confusing me and i get things |
Ok, but having those implemented as unit tests would clearly demonstrate the expected behavior, and ensure in the future that the behavior doesn't break. In retrospect insufficient unit tests played a role in the hurdles following the merge of #37278, and I'd really like to avoid that experience. |
I totally agree with this. When Stephan is back we can come up with a site-local-config.xml and storage.json that cover all possible scenarios and design unit tests for them. For now I can add a unit test for this chaining. |
That would be great!
I think for the chaining alone expanding the existing |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42530/36569
|
Pull request #42530 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again. |
@cmsbuild, please test |
@nhduongvn A gentle ping |
05b57b1
to
000b6fb
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42530/36633
|
Pull request #42530 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again. |
Done! I am sorry for slow response due to traveling. |
Thanks! |
@cmsbuild, please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-46ede2/34346/summary.html Comparison SummarySummary:
|
+core |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
The chaining mechanism is still be supported in the new storage description as in the case of Trivial File Catalogue (TFC) before:
https://twiki.cern.ch/twiki/bin/viewauth/CMS/StorageDescription
However, it was effectively turned off (chain="") during development of codes for the new storage description
#37278
Therefore, some sites will not see the file, for example T2_DE_DESY
https://cms-talk.web.cern.ch/t/errors-of-missing-files-at-t2-de-desy/27838/20
(currently there are three sites use 'chain' T1_DE_KIT, T2_DE_DESY, T2_BE_IIHE.)
This PR puts back the chaining feature. It also unifies the prefix and rules of protocols by converting the prefix to a rule. Therefore, codes handling prefix and rules are the same.