Skip to content
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

Snapshot uses existing TDirectory when MT enabled #4649

Merged
merged 14 commits into from Dec 3, 2019
Merged

Conversation

@goi42
Copy link
Contributor

goi42 commented Nov 26, 2019

This addresses the bug raised in the forum.

Adds option to TDirectoryFile to call mkdir with an existing directory name without raising an error, and implements this option when creating a Snapshot with multithreading enabled. Also changes CreateSnaphotRDF to CreateSnapshotRDF.

@goi42 goi42 requested review from bluehood, dpiparo and pcanal as code owners Nov 26, 2019
@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Nov 26, 2019

Can one of the admins verify this patch?

@bluehood bluehood requested a review from Axel-Naumann Nov 26, 2019
Copy link
Member

pcanal left a comment

I think this is a good improvement. I am pondering if we can find a better name than "ifNotExist".
i.e. technically the current behavior is 'already' "make the directory if it does not exist" ... the new behavior is more akin to (quoting the mkdir man pages :) ) "with this option specified, no error will be reported if a directory given as an operand already exists"

io/io/src/TDirectoryFile.cxx Outdated Show resolved Hide resolved
core/base/src/TDirectory.cxx Outdated Show resolved Hide resolved
io/io/src/TDirectoryFile.cxx Outdated Show resolved Hide resolved
goi42 and others added 2 commits Nov 26, 2019
readability

Co-Authored-By: Philippe Canal <pcanal@fnal.gov>
readability

Co-Authored-By: Philippe Canal <pcanal@fnal.gov>
@bluehood

This comment has been minimized.

Copy link
Member

bluehood commented Nov 26, 2019

Hi @goi42 , thank you very much for the contribution! We definitely want the typo fix and it looks like @pcanal likes the new feature in TDirectoryFile.

However, for Snapshot I wonder whether we shouldn't figure out why Snapshot tries to create the same directory multiple times in the first place -- i.e. your change might silence the only sign we have that something fishy is going on in Snapshot.

@goi42 goi42 force-pushed the goi42:fixRDF branch from 9ca9137 to 15ea790 Nov 26, 2019
@goi42 goi42 force-pushed the goi42:fixRDF branch from 15ea790 to 188aa9f Nov 26, 2019
@bluehood

This comment has been minimized.

Copy link
Member

bluehood commented Nov 26, 2019

Is it in fact the case that Snapshot should not be trying to create the directory multiple times?

Let's just say it's not a conscious design choice 😅 requires a bit of investigation -- might be nothing, and it's fine to just silence the warnings, or it might be some undesirable behavior that we should correct (maybe there is more work than that that is done too many times and shouldn't)

@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Nov 26, 2019

Thanks, @bluehood and @pcanal, for your interest and prompt responses. I've responded to the issues you've raised as best I can (apologies for the force pushes; there won't be any more). @pcanal I've switched from ifNotExist to the longer but more descriptive returnExistingDirectory.

@pcanal

This comment has been minimized.

Copy link
Member

pcanal commented Nov 26, 2019

Looks good. Thanks. One last question (my apologies if I missed the answer): why a difference in behavior between TDirectory and TDirectoryFile?

core/base/src/TDirectory.cxx Outdated Show resolved Hide resolved
Copy link
Member

bluehood left a comment

Looks good to me. Thanks a lot for the contribution.

@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Nov 27, 2019

Great, thanks! Looks like we just need @pcanal to verify the requested changes and for @Axel-Naumann and @dpiparo to sign off.

@pcanal

This comment has been minimized.

Copy link
Member

pcanal commented Nov 27, 2019

I'll go ahead and merge this. Would you also be able to add a test for the feature (tested either directly or indirectly via TDataFrame)?

@pcanal

This comment has been minimized.

Copy link
Member

pcanal commented Dec 2, 2019

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Dec 2, 2019

@pcanal I have switched to using %p, but I see you have already started the build. Let me know if you prefer I revert to casting after all.

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Build failed on ROOT-performance-centos7-multicore/default.
See console output.

Warnings:

  • /data/sftnight/workspace/root-pullrequests-build/root/test/stress.cxx:1636:291: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘intptr_t {aka long int}’ [-Wformat=]
  • /data/sftnight/workspace/root-pullrequests-build/root/test/stress.cxx:1636:291: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘intptr_t {aka long int}’ [-Wformat=]
  • /data/sftnight/workspace/root-pullrequests-build/root/test/stress.cxx:1636:291: warning: format ‘%d’ expects argument of type ‘int’, but argument 5 has type ‘intptr_t {aka long int}’ [-Wformat=]
  • /data/sftnight/workspace/root-pullrequests-build/root/test/stress.cxx:1636:291: warning: format ‘%d’ expects argument of type ‘int’, but argument 6 has type ‘intptr_t {aka long int}’ [-Wformat=]
  • /data/sftnight/workspace/root-pullrequests-build/root/test/stress.cxx:1636:291: warning: format ‘%d’ expects argument of type ‘int’, but argument 8 has type ‘intptr_t {aka long int}’ [-Wformat=]
@pcanal

This comment has been minimized.

Copy link
Member

pcanal commented Dec 2, 2019

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Build failed on ROOT-performance-centos7-multicore/default.
See console output.

Warnings:

  • /data/sftnight/workspace/root-pullrequests-build/root/test/stress.cxx:1636:231: warning: format ‘%p’ expects argument of type ‘void*’, but argument 7 has type ‘int’ [-Wformat=]
@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Build failed on ROOT-fedora30/cxx14.
See console output.

Warnings:

  • /build/workspace/root-pullrequests-build/root/test/stress.cxx:1636:112: warning: format ‘%p’ expects argument of type ‘void*’, but argument 7 has type ‘int’ [-Wformat=]
@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Build failed on ROOT-fedora29/python3.
See console output.

Warnings:

  • /mnt/build/workspace/root-pullrequests-build/root/test/stress.cxx:1636:14: warning: format ‘%p’ expects argument of type ‘void*’, but argument 7 has type ‘int’ [-Wformat=]
@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Build failed on ROOT-fedora27/noimt.
See console output.

Warnings:

  • /mnt/build/workspace/root-pullrequests-build/root/test/stress.cxx:1636:231: warning: format ‘%p’ expects argument of type ‘void*’, but argument 7 has type ‘int’ [-Wformat=]
@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Build failed on ROOT-ubuntu16/nortcxxmod.
See console output.

Warnings:

  • /mnt/build/workspace/root-pullrequests-build/root/test/stress.cxx:1636:216: warning: format ‘%p’ expects argument of type ‘void*’, but argument 7 has type ‘int’ [-Wformat=]
@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Dec 2, 2019

@pcanal sorry for the many adjustments. I don't see these warnings on my build, so I'm sort of shooting in the dark. Hopefully this last change finally fixes it.

@pcanal

This comment has been minimized.

Copy link
Member

pcanal commented Dec 2, 2019

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Build failed on mac1014/cxx17.
See console output.

Warnings:

  • /build/jenkins/workspace/root-pullrequests-build/root/test/stress.cxx:1636:201: warning: format specifies type 'void *' but the argument has type 'int' [-Wformat]
@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 2, 2019

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu18.04-i386/cxx14, mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

@phsft-bot

This comment has been minimized.

@pcanal

This comment has been minimized.

Copy link
Member

pcanal commented Dec 2, 2019

The macos issue seem unrelated and transient.

@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Dec 2, 2019

@pcanal I agree.

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 3, 2019

Build failed on windows10/cxx14.
See console output.

@pcanal

This comment has been minimized.

Copy link
Member

pcanal commented Dec 3, 2019

@phsft-bot build just on mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 3, 2019

Starting build on mac1014/cxx17, windows10/cxx14 with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Dec 3, 2019

Build failed on windows10/cxx14.
See console output.

@pcanal

This comment has been minimized.

Copy link
Member

pcanal commented Dec 3, 2019

Windows failure is infrastructure related. Merging.

Thank you very much for your contribution!!

@pcanal pcanal merged commit e192a58 into root-project:master Dec 3, 2019
2 of 3 checks passed
2 of 3 checks passed
Jenkins CI build Build failed
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Dec 3, 2019

Thanks!

@goi42 goi42 deleted the goi42:fixRDF branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.