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

Avoid Snapshot errors when updating directories #4964

Merged
merged 2 commits into from Feb 12, 2020

Conversation

@goi42
Copy link
Contributor

goi42 commented Feb 11, 2020

If one tries to update a directory inside an existing file, Snapshot complains:
Error in <TFile::mkdir>: An object with name hi exists already

This fixes the problem by using the returnExistingDirectory option in mkdir.

First mentioned in the forum here. Similar to PR4649.

@goi42 goi42 requested review from bluehood and dpiparo as code owners Feb 11, 2020
@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Feb 11, 2020

Can one of the admins verify this patch?

@oshadura

This comment has been minimized.

Copy link
Member

oshadura commented Feb 12, 2020

@phsft-bot build!

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Feb 12, 2020

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
How to customize builds

Copy link
Member

bluehood left a comment

Hi @goi42, thanks for the PR. Are similar changes not needed in SnapshotHelperMT too?

Also please change the indentation to three spaces as per the ROOT coding conventions.

EDIT: if you also add a corresponding test in tree/dataframe/test/dataframe_snapshot.cxx, these changes can go in with the changes requested above.

@bluehood bluehood removed the request for review from dpiparo Feb 12, 2020
@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Feb 12, 2020

@bluehood PR4649 already incorporates similar changes for SnapshotHelperMT. In that case, it calls fOutputFiles[slot]->mkdir(fDirName.c_str(), "", true) regardless of the File Open option since each thread can write to the same directory; thus, no additional adjustment is needed.

@bluehood

This comment has been minimized.

Copy link
Member

bluehood commented Feb 12, 2020

Uhm so half of this fix is in the other PR? 😅

@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Feb 12, 2020

@bluehood no, they address different problems. PR4649 is already merged and prevents errors being raised due to multiple threads writing to the same directory. This PR addresses the case of writing to an existing directory using the "update" option; it's just that no fix is required for MT since it already expects that it may write to an existing directory.

@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Feb 12, 2020

@bluehood It probably would have made sense if I had fixed this in PR4649, but I was trying to solve a different problem in that case and didn't notice/think about this one 🤷🏼‍♂️

@bluehood

This comment has been minimized.

Copy link
Member

bluehood commented Feb 12, 2020

@goi42 ah #4649 was merged some time ago, I see! I got it confused with #4965, sorry!

So this can go in, but I would like to have a test for the fixed usecase. I don't know how to check that TFile printed an Error, do you by chance? I'll ask around :D

@bluehood

This comment has been minimized.

Copy link
Member

bluehood commented Feb 12, 2020

Or maybe the Error caused some visible difference in the output file?

@goi42

This comment has been minimized.

Copy link
Contributor Author

goi42 commented Feb 12, 2020

@bluehood Ah, okay, no problem. I'm currently adding a test to check that both trees get written, but no, I don't know how to check printed errors. Is a test necessary for such a case? A quick check indicates that this PR only affects the printed error statement, not the saved output.

Copy link
Member

bluehood left a comment

Ok let's merge this one and add a test in #4965

@bluehood bluehood merged commit 2f79c02 into root-project:master Feb 12, 2020
2 checks passed
2 checks passed
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@phsft-bot

This comment has been minimized.

Copy link
Collaborator

phsft-bot commented Feb 12, 2020

Build failed on windows10/cxx14.
See console output.

@goi42 goi42 deleted the goi42:SnapshotUpdateDirectory branch Feb 12, 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.