Skip to content

Commit

Permalink
apacheGH-44269: [C++][FS][Azure] Catch missing exceptions on HNS supp…
Browse files Browse the repository at this point in the history
…ort check (apache#44274)

### Rationale for this change

`Azure::Storage::Files::DataLake::DataLakeDirectoryClient` may throw `Azure::Core::Http::TransportException` and `std::runtime_error` exceptions but they aren't caught. Arrow C++ uses `arrow::Status` not C++ exception. So we must catch all exceptions from Azure SDK for C++.

### What changes are included in this PR?

Add catches `Azure::Core::Http::TransportException` and `std::exception`. 

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* GitHub Issue: apache#44269

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
  • Loading branch information
kou authored Oct 2, 2024
1 parent 5d689b4 commit 15cc84d
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 1 deletion.
9 changes: 8 additions & 1 deletion cpp/src/arrow/filesystem/azurefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ std::string BuildBaseUrl(const std::string& scheme, const std::string& authority
}

template <typename... PrefixArgs>
Status ExceptionToStatus(const Storage::StorageException& exception,
Status ExceptionToStatus(const Azure::Core::RequestFailedException& exception,
PrefixArgs&&... prefix_args) {
return Status::IOError(std::forward<PrefixArgs>(prefix_args)..., " Azure Error: [",
exception.ErrorCode, "] ", exception.what());
Expand Down Expand Up @@ -1381,6 +1381,13 @@ Result<HNSSupport> CheckIfHierarchicalNamespaceIsEnabled(
"Check for Hierarchical Namespace support on '",
adlfs_client.GetUrl(), "' failed.");
}
} catch (const Azure::Core::Http::TransportException& exception) {
return ExceptionToStatus(exception, "Check for Hierarchical Namespace support on '",
adlfs_client.GetUrl(), "' failed.");
} catch (const std::exception& exception) {
return Status::UnknownError(
"Check for Hierarchical Namespace support on '", adlfs_client.GetUrl(),
"' failed: ", typeid(exception).name(), ": ", exception.what());
}
}

Expand Down
18 changes: 18 additions & 0 deletions cpp/src/arrow/filesystem/azurefs_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2308,6 +2308,24 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, MovePath) { this->TestMovePath();

// Tests using Azurite (the local Azure emulator)

TEST_F(TestAzuriteFileSystem, CheckIfHierarchicalNamespaceIsEnabledRuntimeError) {
ASSERT_OK(options_.ConfigureAccountKeyCredential("not-base64"));
ASSERT_OK_AND_ASSIGN(auto datalake_service_client,
options_.MakeDataLakeServiceClient());
auto adlfs_client = datalake_service_client->GetFileSystemClient("nonexistent");
ASSERT_RAISES(UnknownError,
internal::CheckIfHierarchicalNamespaceIsEnabled(adlfs_client, options_));
}

TEST_F(TestAzuriteFileSystem, CheckIfHierarchicalNamespaceIsEnabledTransportError) {
options_.dfs_storage_authority = "127.0.0.1:20000"; // Wrong port
ASSERT_OK_AND_ASSIGN(auto datalake_service_client,
options_.MakeDataLakeServiceClient());
auto adlfs_client = datalake_service_client->GetFileSystemClient("nonexistent");
ASSERT_RAISES(IOError,
internal::CheckIfHierarchicalNamespaceIsEnabled(adlfs_client, options_));
}

TEST_F(TestAzuriteFileSystem, GetFileInfoSelector) {
SetUpSmallFileSystemTree();

Expand Down

0 comments on commit 15cc84d

Please sign in to comment.