Remove statuses from ls_with_sizes and do not mask failures on POSIX.
#5043
Conversation
It was not done in HDFS to minimize risk.
Specifying UTF-8 encoding when opening the files prevents an error on Windows.
teo-tsirpanis
changed the title
ls_with_sizes and do not mask failures on POSIX.ls_with_sizes and do not mask failures on POSIX.
tiledb/sm/filesystem/posix.cc
Outdated
| @@ -58,6 +58,18 @@ using filesystem::directory_entry; | |||
|
|
|||
| namespace tiledb::sm { | |||
|
|
|||
| struct UniqueDIRDeleter { | |||
There was a problem hiding this comment.
Shouldn't we just wrap DIR in a class here?
There was a problem hiding this comment.
We could, but the implementation would be quite more complex. By extending unique_ptr we take advantage of its RAII capabilities, while reducing complexity and code changes.
There was a problem hiding this comment.
You are using a deleter to implement a close method. It's confusing. The class to do this will not be complex at all.
There was a problem hiding this comment.
I have seen this pattern in other projects, and we are doing similar stuff in the C++ API but sure, I changed it to a dedicated class.
There was a problem hiding this comment.
Are you talking about the one in curl?
There was a problem hiding this comment.
Yes, and also in the zstd compressor. Turns out I was wrong about the C++ API, we are using shared_ptr instead of unique_ptr there.
There was a problem hiding this comment.
The curl code is 5 years old... The ZSTD context one is a free function, which makes more sense as a deleter.
SC-23179
SC-48851
SC-48854
This PR updates the signature of
ls_with_sizesin theVFSclass and the specific VFS implementations1 to indicate errors by throwing exceptions instead of returningStatus.On top of that,
Posix::ls_with_sizeswas updated to fail ifopendirfailed with an error code other thanENOENT(see also #5037 (comment)).Also in
Posix::ls_with_sizes, the pointer returned byopendirwas updated to be placed inside a smart pointer, making sure it gets freed. This caused errors in thefind_head_api_violations.pyscript, which were fixed.TYPE: BUG
DESC: Do not mask failures when listing a directory fails on POSIX.
Footnotes
HDFS was not updated to minimize risk. ↩