[IO] Honor READ_WITHOUT_GLOBALREGISTRATION in TNetXNGFile by eguiraud · Pull Request #11321 · root-project/root

eguiraud · 2022-09-06T07:56:50Z

...as well as in TNetFile and TWebFile.
This fixes #10742.

@pcanal can you please suggest a test?

phsft-bot · 2022-09-06T07:56:56Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

pcanal · 2022-09-06T19:35:41Z

net/netxng/src/TNetXNGFile.cxx

+TNetXNGFile::TNetXNGFile(const char *url, const char *lurl, Option_t *mode, const char *title, Int_t compress,
+                         Int_t /*netopt*/, Bool_t parallelopen)
+   : TFile((lurl ? lurl : url),
+           strcmp(mode, "READ_WITHOUT_GLOBALREGISTRATION") == 0 ? "NET_WITHOUT_GLOBALREGISTRATION" : "NET", title,


Suggested change

strcmp(mode, "READ_WITHOUT_GLOBALREGISTRATION") == 0 ? "NET_WITHOUT_GLOBALREGISTRATION" : "NET", title,

strstr(mode, "_WITHOUT_GLOBALREGISTRATION") != nullptr ? "NET_WITHOUT_GLOBALREGISTRATION" : "NET", title,

would allow to seamlessly support the other opening type.

what other opening type? NET_WITHOUT_GLOBALREGISTRATION is not available to client code, it's just for internal use (and it's not documented anywhere). the documented option is only READ_WITHOUT_GLOBALREGISTRATION. I think it's simpler this way? (also considering the behavior before this patch)

With the change on line 364 and related, we now support (even if not documented yet) all combination of opening type and _WITHOUT_GLOBALREGISTRATION, including RECREATE_WITHOUT_GLOBALREGISTRATION, UPDATE_WITHOUT_GLOBALREGISTRATION etc. And I think this is a good thing (and should be documented of course :) ).
And without the proposed, change we keep at supporting 'only' READ_WITHOUT_GLOBALREGISTRATION for 'only' theNET/WEB case.

adding support for RECREATE_WITHOUT_GLOBALREGISTRATION etc., adding documentation for them and the corresponding tests is beyond the scope of this PR. The intent is just to fix #10742 .

But to be consistent, you would have to add code to explicitly reject the new supported mode; where as extending is 'simply' using the strstr and extending the doc which is currently bare:

204:/// READ_WITHOUT_GLOBALREGISTRATION | Used by TTreeProcessorMT, not a user callable option.

and could simply be as bare:

204:/// [READ|NEW|UPDATE|RECREATE]_WITHOUT_GLOBALREGISTRATION | Used by TTreeProcessorMT, not a user callable option.

eg. TTreeProcessorMT could use RECREATE_WITHOUT_GLOBALREGISTRATION for Snapshot output file.

to be consistent, you would have to add code to explicitly reject the new supported mode

That's not the behavior anywhere in TFile:

$ root -l root [0] TFile::Open("f.root", "alskjdalksjd") (TFile *) 0x559fa18d0200 root [1] TFile::Open("root://eospublic.cern.ch//eos/root-eos/cms_opendata_2012_nanoaod_skimmed/ZZTo4mu.root", "alsjalsjdalsdjka") (TFile *) 0x555fc11ba680

I just picked up #10742 although it was not assigned to me because that specific issue is problematic for RDF. Adding new features to TFile that were not requested or discussed before can be done in a different PR, and it does not seem reasonable to me to request them from me in the context of a bug fix.

With that said, I applied your suggestion in the original comment so it will be easier to extend this in the future.

pcanal · 2022-09-06T19:42:25Z

Using the file "http://root.cern.ch/files/h1/dstarmb.root" and (when xrootd in enabled) "root://eospublic.cern.ch//eos/root-eos/h1/dstarmb.root" maybe something like:

bool tester(const char *filename) {
   auto f = TFile::Open(filename, "READ_WITHOUT_GLOBALREGISTRATION");
   if (f && ! f->IsZombie() && nullptr == gROOT->GetListOfFiles()->FindObject(filename)) {
      delete f;
      return true;
   } else {
      // failure;
      return false;
  }
}

(possibly separating all 3 failures types).

phsft-bot · 2022-09-07T08:52:39Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-09-07T09:22:26Z

Build failed on ROOT-performance-centos8-multicore/cxx17.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.io.io.test.gtest_io_io_test_TFile

phsft-bot · 2022-09-07T09:23:07Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-09-07T09:25:21Z

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.io.io.test.gtest_io_io_test_TFile

phsft-bot · 2022-09-07T09:31:15Z

Build failed on ROOT-debian10-i386/soversion.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.io.io.test.gtest_io_io_test_TFile

phsft-bot · 2022-09-07T10:07:49Z

Build failed on mac11/cxx14.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.io.io.test.gtest_io_io_test_TFile

phsft-bot · 2022-09-07T10:27:11Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

projectroot.io.io.test.gtest_io_io_test_TFile

phsft-bot · 2022-09-07T11:04:29Z

Build failed on mac1015/cxx17.
Running on macitois22.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.io.io.test.gtest_io_io_test_TFile

eguiraud · 2022-09-07T15:33:29Z

The problem with the test is that on those platforms TDavixFile is used, while on my laptop TWebFile is used instead so I only applied the fix there. Now patching TDavixFile

...as well as in TNetFile, TDavixFile and TWebFile. This fixes root-project#10742.

phsft-bot · 2022-09-07T15:45:47Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-09-07T16:14:13Z

Build failed on ROOT-performance-centos8-multicore/cxx17.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-09-07T16:56:52Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

projectroot.io.io.test.gtest_io_io_test_TFile

io/io/test/TFileTests.cxx

phsft-bot · 2022-09-07T18:09:41Z

Build failed on mac11/cxx14.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

It might work on more platforms. Co-authored-by: Philippe Canal <[email protected]>

phsft-bot · 2022-09-07T19:36:48Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-09-07T20:06:11Z

Build failed on ROOT-performance-centos8-multicore/cxx17.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-09-07T20:58:17Z

Build failed on mac11/cxx14.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-09-07T21:08:04Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-09-07T21:50:39Z

Build failed on mac1015/cxx17.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.python.distrdf.spark.roottest_python_distrdf_spark_test_all

pcanal · 2022-09-08T01:03:28Z

The failures are unrelated.

pcanal

Thanks.

eguiraud requested a review from gganis as a code owner September 6, 2022 07:56

eguiraud self-assigned this Sep 6, 2022

eguiraud requested a review from pcanal as a code owner September 6, 2022 07:56

eguiraud mentioned this pull request Sep 6, 2022

[DF] Avoid registration of inner TChain with global lists #11322

Merged

pcanal reviewed Sep 6, 2022

View reviewed changes

eguiraud force-pushed the fix-no-globalregistration branch from 4dcae88 to a4630cc Compare September 7, 2022 08:52

eguiraud added 2 commits September 7, 2022 17:36

[IO] Honor READ_WITHOUT_GLOBALREGISTRATION in TNetXNGFile

42f4e25

...as well as in TNetFile, TDavixFile and TWebFile. This fixes root-project#10742.

[IO] Add tests for READ_WITHOUT_GLOBALREGISTRATION option

a35c5c7

eguiraud force-pushed the fix-no-globalregistration branch from a4630cc to a35c5c7 Compare September 7, 2022 15:45

pcanal reviewed Sep 7, 2022

View reviewed changes

io/io/test/TFileTests.cxx Outdated Show resolved Hide resolved

[IO] Prefer http to https in test

7c8fe9a

It might work on more platforms. Co-authored-by: Philippe Canal <[email protected]>

pcanal approved these changes Sep 8, 2022

View reviewed changes

eguiraud merged commit 7150f6b into root-project:master Sep 8, 2022

eguiraud deleted the fix-no-globalregistration branch September 8, 2022 07:50

	strcmp(mode, "READ_WITHOUT_GLOBALREGISTRATION") == 0 ? "NET_WITHOUT_GLOBALREGISTRATION" : "NET", title,
	strstr(mode, "_WITHOUT_GLOBALREGISTRATION") != nullptr ? "NET_WITHOUT_GLOBALREGISTRATION" : "NET", title,

Conversation

eguiraud commented Sep 6, 2022

Uh oh!

phsft-bot commented Sep 6, 2022

Uh oh!

pcanal Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

eguiraud Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

pcanal Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

eguiraud Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

pcanal Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

pcanal Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

eguiraud Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

pcanal commented Sep 6, 2022

Uh oh!

phsft-bot commented Sep 7, 2022

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

eguiraud commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Sep 7, 2022

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

phsft-bot commented Sep 7, 2022

Failing tests:

Uh oh!

eguiraud commented Sep 7, 2022 •

edited

Loading