Specify non-random location for filesystem's temporary cache #266
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandreeva-nv
requested review from
kthui,
rmccorm4 and
GuanLuo
and removed request for
kthui,
rmccorm4 and
GuanLuo
oandreeva-nv
force-pushed
the
oandreeva_specify_location_filesystem
branch
from
23498bc to
af3f71f
Compare
rmccorm4
reviewed
Sep 18, 2023
rmccorm4
reviewed
Sep 18, 2023
rmccorm4
reviewed
Sep 18, 2023
rmccorm4
approved these changes
Sep 18, 2023
kthui
approved these changes
Sep 18, 2023
| /// \param temp_dir Returns the path to the temporary directory. | ||
| /// \return Error status | ||
| Status MakeTemporaryDirectory( | ||
| const FileSystemType type, std::string dir_path, std::string* temp_dir); |
There was a problem hiding this comment.
A small thing, I think the dir_path can be declared as const std::string& dir_path here, and leave the LocalFileSystem::MakeTemporaryDirectory(std::string dir_path, ...) as it is. Then, we can save a copy of dir_path when passing the variable from the caller to MakeTemporaryDirectory() here, and only copy it once when passing dir_path from here to LocalFileSystem::MakeTemporaryDirectory().
There was a problem hiding this comment.
I'll make sure to check and introduce this in a follow-up pr with the bug fix. Thanks for suggestion!
| /// \param temp_dir Returns the path to the temporary directory. | ||
| /// \return Error status | ||
| Status MakeTemporaryDirectory( | ||
| const FileSystemType type, std::string dir_path, std::string* temp_dir); |
There was a problem hiding this comment.
Suggested change
| const FileSystemType type, std::string dir_path, std::string* temp_dir); | |
| const FileSystemType type, const std::string& dir_path, std::string* temp_dir); |
|
|
||
| Status | ||
| MakeTemporaryDirectory( | ||
| const FileSystemType type, std::string dir_path, std::string* temp_dir) |
There was a problem hiding this comment.
Suggested change
| const FileSystemType type, std::string dir_path, std::string* temp_dir) | |
| const FileSystemType type, const std::string& dir_path, std::string* temp_dir) |
mc-nv
pushed a commit
that referenced
this pull request
Sep 19, 2023
* Add ability to specify directory for cloud storage
mc-nv
pushed a commit
that referenced
this pull request
Sep 19, 2023
* Add ability to specify directory for cloud storage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main ask for DLIS-5340
Previous PR: #254 needs some debugging.
This PR will only read env variable directory (if specified) and create temporary folder there.