-
Notifications
You must be signed in to change notification settings - Fork 111
[WIP] Fixing extra download for filesystem's localization #254
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
e5a40b7
8474fa0
8511943
af94ad7
2bd9a25
6903e1c
c5ded7e
d35a794
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,11 +145,27 @@ Status ReadTextFile(const std::string& path, std::string* contents); | |
|
||
/// Create an object representing a local copy of a path. | ||
/// \param path The path of the directory or file. | ||
/// \param recursive If true, will fetch all sub-directories in | ||
/// the provided path. | ||
/// \param localized Returns the LocalizedPath object | ||
/// representing the local copy of the path. | ||
/// \return Error status | ||
Status LocalizePath( | ||
const std::string& path, std::shared_ptr<LocalizedPath>* localized); | ||
const std::string& path, const bool recursive, | ||
std::shared_ptr<LocalizedPath>* localized); | ||
|
||
/// Create an object representing a local copy of a path. | ||
/// \param path The path of the directory or file. | ||
/// \param recursive If true, will fetch all sub-directories in | ||
/// the provided path. | ||
/// \param mount_dir If specified, will use provided local directory | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean "If not empty" rather than "If specified"? I do not think the caller can skip this parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll clarify, thank you for suggestion |
||
/// for localization. | ||
/// \param localized Returns the LocalizedPath object | ||
/// representing the local copy of the path. | ||
/// \return Error status | ||
Status LocalizePath( | ||
const std::string& path, const bool recursive, const std::string& mount_dir, | ||
std::shared_ptr<LocalizedPath>* localized); | ||
|
||
/// Write a string to a file. | ||
/// \param path The path of the file. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,7 +75,8 @@ class GCSFileSystem : public FileSystem { | |
const std::string& path, std::set<std::string>* files) override; | ||
Status ReadTextFile(const std::string& path, std::string* contents) override; | ||
Status LocalizePath( | ||
const std::string& path, | ||
const std::string& path, const bool recursive, | ||
const std::string& mount_dir, | ||
std::shared_ptr<LocalizedPath>* localized) override; | ||
Status WriteTextFile( | ||
const std::string& path, const std::string& contents) override; | ||
|
@@ -363,7 +364,8 @@ GCSFileSystem::ReadTextFile(const std::string& path, std::string* contents) | |
|
||
Status | ||
GCSFileSystem::LocalizePath( | ||
const std::string& path, std::shared_ptr<LocalizedPath>* localized) | ||
const std::string& path, const bool recursive, const std::string& mount_dir, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you going to implement the new API in other cloud file system in this PR as well? If not, should return error if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I wanted to first get an agreement on the implementation for s3. I'll populate it to GCS and AS in the next commit for easier review |
||
std::shared_ptr<LocalizedPath>* localized) | ||
{ | ||
bool exists; | ||
RETURN_IF_ERROR(FileExists(path, &exists)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,7 +49,8 @@ class LocalFileSystem : public FileSystem { | |
const std::string& path, std::set<std::string>* files) override; | ||
Status ReadTextFile(const std::string& path, std::string* contents) override; | ||
Status LocalizePath( | ||
const std::string& path, | ||
const std::string& path, const bool recursive, | ||
const std::string& mount_dir, | ||
std::shared_ptr<LocalizedPath>* localized) override; | ||
Status WriteTextFile( | ||
const std::string& path, const std::string& contents) override; | ||
|
@@ -204,7 +205,8 @@ LocalFileSystem::ReadTextFile(const std::string& path, std::string* contents) | |
|
||
Status | ||
LocalFileSystem::LocalizePath( | ||
const std::string& path, std::shared_ptr<LocalizedPath>* localized) | ||
const std::string& path, const bool recursive, const std::string& mount_dir, | ||
std::shared_ptr<LocalizedPath>* localized) | ||
{ | ||
// For local file system we don't actually need to download the | ||
// directory or file. We use it in place. | ||
|
@@ -254,8 +256,12 @@ LocalFileSystem::MakeDirectory(const std::string& dir, const bool recursive) | |
if (mkdir(dir.c_str(), S_IRWXU) == -1) | ||
#endif | ||
{ | ||
// Only allow the error due to parent directory does not exist | ||
// if 'recursive' is requested | ||
// Return success if directory already exists | ||
if (errno == EEXIST) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it be a function argument on whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I'll add it |
||
return Status::Success; | ||
} | ||
// In all other cases only allow the error due to parent directory | ||
// does not exist, if 'recursive' is requested | ||
if ((errno == ENOENT) && (!dir.empty()) && recursive) { | ||
RETURN_IF_ERROR(MakeDirectory(DirName(dir), recursive)); | ||
// Retry the creation | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this (L565-L567) can potentially be shortened to