kou commented on code in PR #39450:
URL: https://github.com/apache/arrow/pull/39450#discussion_r1442238902
##########
cpp/src/arrow/filesystem/azurefs.h:
##########
@@ -92,30 +107,30 @@ struct ARROW_EXPORT AzureOptions {
private:
enum class CredentialKind {
+ kDefault,
kAnonymous,
- kTokenCredential,
- kStorageSharedKeyCredential,
- } credential_kind_ = CredentialKind::kAnonymous;
+ kStorageSharedKey,
+ kClientSecret,
+ kManagedIdentity,
+ kWorkloadIdentity,
+ } credential_kind_ = CredentialKind::kDefault;
- std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
+ mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential>
token_credential_;
Review Comment:
How about always creating `Azure::Identity::DefaultAzureCredential` instead
of using `mutable`?
```diff
diff --git a/cpp/src/arrow/filesystem/azurefs.cc
b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc
+++ b/cpp/src/arrow/filesystem/azurefs.cc
@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const
std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ =
std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>>
AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return
std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {
- token_credential_ =
std::make_shared<Azure::Identity::DefaultAzureCredential>();
- }
- [[fallthrough]];
+ return std::make_unique<Blobs::BlobServiceClient>(
+ AccountBlobUrl(account_name),
+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {
- token_credential_ =
std::make_shared<Azure::Identity::DefaultAzureCredential>();
- }
- [[fallthrough]];
+ return std::make_unique<DataLake::DataLakeServiceClient>(
+ AccountDfsUrl(account_name),
+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h
b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h
+++ b/cpp/src/arrow/filesystem/azurefs.h
@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential>
token_credential_;
+ std::shared_ptr<Azure::Core::Credentials::TokenCredential>
token_credential_;
public:
AzureOptions();
```
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -280,22 +280,41 @@ TEST(AzureFileSystem,
InitializingFilesystemWithoutAccountNameFails) {
ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}
-TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
+TEST(AzureFileSystem, InitializeWithDefaultCredential) {
AzureOptions options;
options.account_name = "dummy-account-name";
- ARROW_EXPECT_OK(
- options.ConfigureClientSecretCredential("tenant_id", "client_id",
"client_secret"));
+ ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}
-TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
+TEST(AzureFileSystem, InitializeWithDefaultCredentialImplicitly) {
AzureOptions options;
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
+
Review Comment:
What is tested in this test?
Should we remove these lines?
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
