Refactored code in StorageFactory #35435

Dr15Jones · 2021-09-27T21:18:17Z

PR description:

moved all code into edm::storage namespace
Removed diamond inheritance in Storage, IOChannel, File relationship by having File contain a IOChannel instead of inheriting from it.

PR validation:

Code compiles.
Framework unit tests all pass.

Changing inheritance to containment avoids diamond inheritance with Storage. Removed some unused methods.

cmsbuild · 2021-09-27T21:25:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35435/25568

This PR adds an extra 212KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Utilities/XrdAdaptor/plugins/XrdStorageMaker.cc modified in PR(s): XrdAdaptor: Fix race condition leading to a crash when doing Prepare … #32763, Backport XrootD 5.3.1 PR to master from DEVEL #34991
- File Utilities/XrdAdaptor/src/XrdRequestManager.cc modified in PR(s): Backport XrootD 5.3.1 PR to master from DEVEL #34991

cmsbuild · 2021-09-27T21:25:53Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

FWCore/Services (core)
IOPool/Common (core)
IOPool/Input (core)
IOPool/Streamer (core)
IOPool/TFileAdaptor (core)
Utilities/DCacheAdaptor (core)
Utilities/LStoreAdaptor (core)
Utilities/StorageFactory (core)
Utilities/XrdAdaptor (core)

@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @wddgit this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2021-09-27T21:46:17Z

please test

cmsbuild · 2021-09-27T21:56:48Z

-1

Failed Tests: Build HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9c5581/19153/summary.html
COMMIT: 99218bb
CMSSW: CMSSW_12_1_X_2021-09-27-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35435/19153/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Package IORawData/SiPixelInputSources built
Entering library rule at IORawData/SiPixelInputSources
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/src/SealModule.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/src/PixelSLinkDataInputSource.cc
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/src/PixelSLinkDataInputSource.cc:24:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/interface/PixelSLinkDataInputSource.h:52:19: error: 'Storage' was not declared in this scope; did you mean 'edm::storage::Storage'?
   52 |   std::unique_ptr storage;
      |                   ^~~~~~~
      |                   edm::storage::Storage
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/interface/PixelSLinkDataInputSource.h:33,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-09-27-1100/src/IORawData/SiPixelInputSources/src/PixelSLinkDataInputSource.cc:24:

Dr15Jones · 2021-09-28T13:16:05Z

@makortel @dan131riley An additional posssible refactoring would be removing the interfaces IOInput, IOOutput and IOChannel all together as nothing in our system actually wants to talk just to those interfaces. I think this is all left over from the SEAL import. Thoughts on keeping or removing said interfaces?

Dr15Jones · 2021-09-28T13:22:30Z

Here is a system diagram for the storage code
StorageFactory System.pdf

dan131riley · 2021-09-28T14:25:50Z

@Dr15Jones I would definitely get rid of IOChannel, it's not a useful abstraction for the way we use StorageFactory, and its effect on the class hierarchy is pernicious. Getting rid of the IOInput and IOOutput sounds ok, but I don't feel nearly as strongly about them.

cmsbuild · 2021-09-28T14:31:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35435/25605

This PR adds an extra 216KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Utilities/XrdAdaptor/plugins/XrdStorageMaker.cc modified in PR(s): XrdAdaptor: Fix race condition leading to a crash when doing Prepare … #32763, Backport XrootD 5.3.1 PR to master from DEVEL #34991
- File Utilities/XrdAdaptor/src/XrdRequestManager.cc modified in PR(s): Backport XrootD 5.3.1 PR to master from DEVEL #34991

cmsbuild · 2021-09-28T18:53:37Z

Pull request #35435 was updated. @SiewYan, @smuzaffar, @Dr15Jones, @mkirsano, @malbouis, @makortel, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @yuanchao, @agrohsje, @alberto-sanchez, @francescobrivio, @tvami can you please check and sign again.

Dr15Jones · 2021-09-28T18:53:45Z

please test

cmsbuild · 2021-09-28T22:05:51Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9c5581/19211/summary.html
COMMIT: c48b59c
CMSSW: CMSSW_12_1_X_2021-09-28-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35435/19211/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 3211080
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3211058
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: no differences found

agrohsje · 2021-09-29T07:49:19Z

GeneratorInterface/LHEInterface/src/LHEReader.cc

@@ -47,6 +47,7 @@ namespace lhef {
  class LHEReader::FileSource : public LHEReader::Source {
  public:
    FileSource(const std::string &fileURL) {
+      using namespace edm::storage;
      auto storage = StorageFactory::get()->open(fileURL, IOFlags::OpenRead);


Hi @Dr15Jones : Shouldn't it be edm::Storage::StorageFactory in this line according to our coding rules?

I've been unable to determine which coding rule to which you refer. Could you point me to the reference?

I thought I got that comment once but indeed I cannot spot in the rules. Let me sign.

agrohsje · 2021-09-29T14:36:08Z

+generators

Dr15Jones · 2021-09-29T14:44:03Z

@agrohsje our coding rules say not to use using namespace ... in global scope in header files. It is perfectly fine to use them anywhere in .cc files or inside functions or class definitions (in both .cc or .h files).

tvami · 2021-09-30T02:59:34Z

+alca

signature is needed for IORawData/SiPixelInputSources, which looks fine
tests pass w no failures

makortel · 2021-09-30T17:53:18Z

+1

cmsbuild · 2021-09-30T17:53:55Z

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, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-10-01T00:34:59Z

+1

Dr15Jones added 2 commits September 27, 2021 10:58

move StorageFactory code to edm::storage namespace

bf8f910

File now contains an IOChannel

99218bb

Changing inheritance to containment avoids diamond inheritance with Storage. Removed some unused methods.

cmsbuild added this to the CMSSW_12_1_X milestone Sep 27, 2021

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Sep 27, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 27, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 27, 2021

cmsbuild added tests-rejected and removed tests-started labels Sep 27, 2021

Updated to new storage system namespace

30f9663

cmsbuild added alca-pending code-checks-pending generators-pending tests-pending and removed tests-rejected code-checks-approved labels Sep 28, 2021

cmsbuild removed the code-checks-pending label Sep 28, 2021

cmsbuild added the code-checks-approved label Sep 28, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 28, 2021

cmsbuild added tests-approved and removed tests-started labels Sep 28, 2021

agrohsje reviewed Sep 29, 2021

View reviewed changes

cmsbuild added generators-approved and removed generators-pending labels Sep 29, 2021

cmsbuild added alca-approved and removed alca-pending labels Sep 30, 2021

cmsbuild mentioned this pull request Sep 30, 2021

For async calls always store XrdCl::FileSystem with the response-handler. #35481

Merged

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Sep 30, 2021

makortel mentioned this pull request Sep 30, 2021

Refactored code in StorageFactory cms-sw/framework-team#291

Closed

cmsbuild added orp-approved and removed orp-pending labels Oct 1, 2021

cmsbuild merged commit f5d784d into cms-sw:master Oct 1, 2021

smuzaffar mentioned this pull request Oct 1, 2021

[XrdAdaptor] Delay additional source acquisition when redirect limit error is returned. #35498

Merged

Dr15Jones deleted the updateStorageFactory branch October 20, 2021 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored code in StorageFactory #35435

Refactored code in StorageFactory #35435

Dr15Jones commented Sep 27, 2021

cmsbuild commented Sep 27, 2021

cmsbuild commented Sep 27, 2021

Dr15Jones commented Sep 27, 2021

cmsbuild commented Sep 27, 2021

Dr15Jones commented Sep 28, 2021

Dr15Jones commented Sep 28, 2021 •

edited

Loading

dan131riley commented Sep 28, 2021

cmsbuild commented Sep 28, 2021

cmsbuild commented Sep 28, 2021

Dr15Jones commented Sep 28, 2021

cmsbuild commented Sep 28, 2021

agrohsje Sep 29, 2021

Dr15Jones Sep 29, 2021

agrohsje Sep 29, 2021

agrohsje commented Sep 29, 2021

Dr15Jones commented Sep 29, 2021

tvami commented Sep 30, 2021

makortel commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

qliphy commented Oct 1, 2021

Refactored code in StorageFactory #35435

Refactored code in StorageFactory #35435

Conversation

Dr15Jones commented Sep 27, 2021

PR description:

PR validation:

cmsbuild commented Sep 27, 2021

cmsbuild commented Sep 27, 2021

Dr15Jones commented Sep 27, 2021

cmsbuild commented Sep 27, 2021

Build

Dr15Jones commented Sep 28, 2021

Dr15Jones commented Sep 28, 2021 • edited Loading

dan131riley commented Sep 28, 2021

cmsbuild commented Sep 28, 2021

cmsbuild commented Sep 28, 2021

Dr15Jones commented Sep 28, 2021

cmsbuild commented Sep 28, 2021

Comparison Summary

agrohsje Sep 29, 2021

Choose a reason for hiding this comment

Dr15Jones Sep 29, 2021

Choose a reason for hiding this comment

agrohsje Sep 29, 2021

Choose a reason for hiding this comment

agrohsje commented Sep 29, 2021

Dr15Jones commented Sep 29, 2021

tvami commented Sep 30, 2021

makortel commented Sep 30, 2021

cmsbuild commented Sep 30, 2021

qliphy commented Oct 1, 2021

Dr15Jones commented Sep 28, 2021 •

edited

Loading