This is an automated email from the ASF dual-hosted git repository.
felipecrv pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 4fc2a708ba GH-39718: [C++][FS][Azure] Remove StatusFromErrorResponse
as it's not necessary (#39719)
4fc2a708ba is described below
commit 4fc2a708bac92614a42c07ea1f23b1a4a3af88cb
Author: Felipe Oliveira Carvalho <[email protected]>
AuthorDate: Mon Jan 22 17:23:54 2024 -0300
GH-39718: [C++][FS][Azure] Remove StatusFromErrorResponse as it's not
necessary (#39719)
### Rationale for this change
Only the "*IfExists" functions from the Azure SDK ever set
`response.Value.Deleted` to `false` to indicate that a resource wasn't found
and the request succeeded without deleting anything.
It's better that we use the `Delete()` versions of these functions instead
of `DeleteIfExists` and check the `ErrorCode` ourselves to return an
appropriate `Status` instead of something generic.
### What changes are included in this PR?
- Removing `StatusFromErrorResponse`
- Comments explaining the error handling decisions
- Addition of a boolean parameter to `DeleteDirOnFileSystem` that controls
how it fails when the directory being deleted doesn't exist
### Are these changes tested?
- Yes, by the existing tests in `azurefs_test.cc`.
* Closes: #39718
Authored-by: Felipe Oliveira Carvalho <[email protected]>
Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
---
cpp/src/arrow/filesystem/azurefs.cc | 62 +++++++++++++------------------------
1 file changed, 22 insertions(+), 40 deletions(-)
diff --git a/cpp/src/arrow/filesystem/azurefs.cc
b/cpp/src/arrow/filesystem/azurefs.cc
index 730adabd48..a5179c2219 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc
+++ b/cpp/src/arrow/filesystem/azurefs.cc
@@ -305,31 +305,9 @@ Status ValidateFileLocation(const AzureLocation& location)
{
return internal::AssertNoTrailingSlash(location.path);
}
-std::string_view BodyTextView(const Http::RawResponse& raw_response) {
- const auto& body = raw_response.GetBody();
-#ifndef NDEBUG
- auto& headers = raw_response.GetHeaders();
- auto content_type = headers.find("Content-Type");
- if (content_type != headers.end()) {
- DCHECK_EQ(std::string_view{content_type->second}.substr(5), "text/");
- }
-#endif
- return std::string_view{reinterpret_cast<const char*>(body.data()),
body.size()};
-}
-
-Status StatusFromErrorResponse(const std::string& url,
- const Http::RawResponse& raw_response,
- const std::string& context) {
- // There isn't an Azure specification that response body on error
- // doesn't contain any binary data but we assume it. We hope that
- // error response body has useful information for the error.
- auto body_text = BodyTextView(raw_response);
- return Status::IOError(context, ": ", url, ": ",
raw_response.GetReasonPhrase(), " (",
- static_cast<int>(raw_response.GetStatusCode()),
- "): ", body_text);
-}
-
bool IsContainerNotFound(const Storage::StorageException& e) {
+ // In some situations, only the ReasonPhrase is set and the
+ // ErrorCode is empty, so we check both.
if (e.ErrorCode == "ContainerNotFound" ||
e.ReasonPhrase == "The specified container does not exist." ||
e.ReasonPhrase == "The specified filesystem does not exist.") {
@@ -1515,13 +1493,9 @@ class AzureFileSystem::Impl {
DCHECK(location.path.empty());
try {
auto response = container_client.Delete();
- if (response.Value.Deleted) {
- return Status::OK();
- } else {
- return StatusFromErrorResponse(
- container_client.GetUrl(), *response.RawResponse,
- "Failed to delete a container: " + location.container);
- }
+ // Only the "*IfExists" functions ever set Deleted to false.
+ // All the others either succeed or throw an exception.
+ DCHECK(response.Value.Deleted);
} catch (const Storage::StorageException& exception) {
if (IsContainerNotFound(exception)) {
return PathNotFound(location);
@@ -1530,6 +1504,7 @@ class AzureFileSystem::Impl {
"Failed to delete a container: ",
location.container, ": ",
container_client.GetUrl());
}
+ return Status::OK();
}
/// Deletes contents of a directory and possibly the directory itself
@@ -1649,23 +1624,29 @@ class AzureFileSystem::Impl {
/// \pre location.container is not empty.
/// \pre location.path is not empty.
Status DeleteDirOnFileSystem(const DataLake::DataLakeFileSystemClient&
adlfs_client,
- const AzureLocation& location) {
+ const AzureLocation& location, bool recursive,
+ bool require_dir_to_exist) {
DCHECK(!location.container.empty());
DCHECK(!location.path.empty());
auto directory_client = adlfs_client.GetDirectoryClient(location.path);
- // XXX: should "directory not found" be considered an error?
try {
- auto response = directory_client.DeleteRecursive();
- if (response.Value.Deleted) {
+ auto response =
+ recursive ? directory_client.DeleteRecursive() :
directory_client.DeleteEmpty();
+ // Only the "*IfExists" functions ever set Deleted to false.
+ // All the others either succeed or throw an exception.
+ DCHECK(response.Value.Deleted);
+ } catch (const Storage::StorageException& exception) {
+ if (exception.ErrorCode == "FilesystemNotFound" ||
+ exception.ErrorCode == "PathNotFound") {
+ if (require_dir_to_exist) {
+ return PathNotFound(location);
+ }
return Status::OK();
- } else {
- return StatusFromErrorResponse(directory_client.GetUrl(),
*response.RawResponse,
- "Failed to delete a directory: " +
location.path);
}
- } catch (const Storage::StorageException& exception) {
return ExceptionToStatus(exception, "Failed to delete a directory: ",
location.path,
": ", directory_client.GetUrl());
}
+ return Status::OK();
}
/// \pre location.container is not empty.
@@ -1855,7 +1836,8 @@ Status AzureFileSystem::DeleteDir(const std::string&
path) {
return PathNotFound(location);
}
if (hns_support == HNSSupport::kEnabled) {
- return impl_->DeleteDirOnFileSystem(adlfs_client, location);
+ return impl_->DeleteDirOnFileSystem(adlfs_client, location,
/*recursive=*/true,
+ /*require_dir_to_exist=*/true);
}
DCHECK_EQ(hns_support, HNSSupport::kDisabled);
auto container_client = impl_->GetBlobContainerClient(location.container);
