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

Add create dir, delete dir/file #41082

Merged
merged 2 commits into from
Jul 7, 2020

Conversation

vnghia
Copy link
Contributor

@vnghia vnghia commented Jul 4, 2020

This PR adds create dir, delete dir/file

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Jul 4, 2020
@gbaned gbaned self-assigned this Jul 6, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Jul 6, 2020
Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit is just a question.

return;
}

MaybeAppendSlash(&object);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can we ensure that object always ends with a /?

Copy link
Contributor Author

@vnghia vnghia Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I dont understand what you mean. MaybeAppendSlach will ensure object will end with a '/'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant object is always produced by other functions. Can we ensure that those functions add the trailing slash?

Basically, from the point of view of a caller to ParseGCSPath, object either always has the trailing / or not but not something in between. Otherwise, you always have to write code that uses both ParseGCSPath and MaybeAppendSlash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could force object always has the trailing / or not in ParseGCSPath but it causes many unwanted behaviors.

In the local filesystem, we have a function mkdir to create directory and the directory name can have / or not, it does not matter.

But with gcs filesystem, we have to create an empty object with the name ends with / so Google Cloud Console could display it as a directory.

If we force object does not have / at the end, Google Cloud Console will display that object as a empty object.
If we force object has / at the end, Google Cloud Console will display everything as a directory ( even the object's size is bigger than 0 ).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then let's leave it as it is

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Jul 6, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 6, 2020
@tensorflow-copybara tensorflow-copybara merged commit 1f862f5 into tensorflow:master Jul 7, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Jul 7, 2020
@vnghia vnghia deleted the gcs-file-op branch July 8, 2020 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kokoro:force-run Tests on submitted change ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants