Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
conbench-apache-arrow[bot] commented on PR #41276: URL: https://github.com/apache/arrow/pull/41276#issuecomment-2068463658 After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 25bb627519f17835cb0b6ff95588f2de0cbaf7ad. There were no benchmark performance regressions. The [full Conbench report](https://github.com/apache/arrow/runs/24085006200) has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou merged PR #41276: URL: https://github.com/apache/arrow/pull/41276 -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1573992832
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
Let's look at this in #41315.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on PR #41276: URL: https://github.com/apache/arrow/pull/41276#issuecomment-2068301091 I'll merge this. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
Tom-Newton commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1573734719
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
> I tried az login and ConfigureDefaultCredential() but some operations such
as CreateDir() was blocked...
It seems that we need to implement
https://github.com/apache/arrow/issues/39344 to test this part.
This seems a bit suspicious. Default credential does include CLI auth...
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1573579875
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
#41315
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1573578375
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
Thanks.
I tried `az login` and `ConfigureDefaultCredential()` but some operations
such as `CreateDir()` was blocked...
It seems that we need to implement GH-39344 to test this part.
I'll open a new issue for this part and defer this part in this PR.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
felipecrv commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1572597451
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -2868,8 +2886,19 @@ class AzureFileSystem::Impl {
if (src == dest) {
return Status::OK();
}
+std::string sas_token;
+{
+ Storage::Sas::BlobSasBuilder builder;
+ std::chrono::seconds available_period(60);
+ builder.ExpiresOn = std::chrono::system_clock::now() + available_period;
+ builder.BlobContainerName = src.container;
+ builder.BlobName = src.path;
+ builder.Resource = Storage::Sas::BlobSasResource::Blob;
+ builder.SetPermissions(Storage::Sas::BlobSasPermissions::Read);
+ ARROW_ASSIGN_OR_RAISE(sas_token, options_.GenerateSASToken());
+}
+auto src_url = GetBlobClient(src.container, src.path).GetUrl() + sas_token;
Review Comment:
Ok. It comes with the `?` already.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
Tom-Newton commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1571438084
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
Sorry I misunderstood the intention (I haven't found time to review this).
Configuring other auths in testing might be a bit difficult. For Azurite I
think the first challenge is configuring `https`, which I have struggled with
before. Beyond that I think there are 2 viable options offered by Azure:
1. Service principal
1. Username and password for a service account
2. Surprisingly complicated to create
1. Azure CLI
1. Authenticates as your user
2. Requires installing the Azure CLI and running `az login`, then
completing the login in your browser.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1571416818
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
Yes. We can create a SAS token from an account key. The above
`builder->GenerateSasToken(*storage_shared_key_credential_)` does it.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1570156547
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
@Tom-Newton It seems that we can't use `GetUserDelegationKey()` with account
key credential. We may need to use other authentication to test
`GetUserDelegationKey()`. Do you know how to configure other authentication...?
(I think that we can defer this case to a separated issue.)
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1571400424
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
+ARROW_ASSIGN_OR_RAISE(auto client, MakeBlobServiceClient());
Review Comment:
OK. I've added it.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
Tom-Newton commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1571400330
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
I'm pretty sure its possible to create a SAS token from an account key but
it probably isn't called delegated anymore.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1571394212
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -2868,8 +2886,19 @@ class AzureFileSystem::Impl {
if (src == dest) {
return Status::OK();
}
+std::string sas_token;
+{
+ Storage::Sas::BlobSasBuilder builder;
+ std::chrono::seconds available_period(60);
+ builder.ExpiresOn = std::chrono::system_clock::now() + available_period;
+ builder.BlobContainerName = src.container;
+ builder.BlobName = src.path;
+ builder.Resource = Storage::Sas::BlobSasResource::Blob;
+ builder.SetPermissions(Storage::Sas::BlobSasPermissions::Read);
+ ARROW_ASSIGN_OR_RAISE(sas_token, options_.GenerateSASToken());
+}
+auto src_url = GetBlobClient(src.container, src.path).GetUrl() + sas_token;
Review Comment:
A sample SAS token:
`?se=2024-04-18T21:17:19Z=HI/8c...ZExA%3D=r=https=b=2022-11-02`
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
felipecrv commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1571129208
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
+ARROW_ASSIGN_OR_RAISE(auto client, MakeBlobServiceClient());
Review Comment:
The caller already has a `BlobServiceClient`, so it's probably a good idea
to have a version of `GenerateSASToken` that takes one as parameter.
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -2868,8 +2886,19 @@ class AzureFileSystem::Impl {
if (src == dest) {
return Status::OK();
}
+std::string sas_token;
+{
+ Storage::Sas::BlobSasBuilder builder;
+ std::chrono::seconds available_period(60);
+ builder.ExpiresOn = std::chrono::system_clock::now() + available_period;
+ builder.BlobContainerName = src.container;
+ builder.BlobName = src.path;
+ builder.Resource = Storage::Sas::BlobSasResource::Blob;
+ builder.SetPermissions(Storage::Sas::BlobSasPermissions::Read);
+ ARROW_ASSIGN_OR_RAISE(sas_token, options_.GenerateSASToken());
+}
+auto src_url = GetBlobClient(src.container, src.path).GetUrl() + sas_token;
Review Comment:
What is a sas token that it can be concatenated just like this to an URL?
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
kou commented on code in PR #41276:
URL: https://github.com/apache/arrow/pull/41276#discussion_r1570156547
##
cpp/src/arrow/filesystem/azurefs.cc:
##
@@ -381,6 +388,24 @@ AzureOptions::MakeDataLakeServiceClient() const {
return Status::Invalid("AzureOptions doesn't contain a valid auth
configuration");
}
+Result AzureOptions::GenerateSASToken(
+Storage::Sas::BlobSasBuilder* builder) const {
+ if (storage_shared_key_credential_) {
+return builder->GenerateSasToken(*storage_shared_key_credential_);
+ } else {
+// This part isn't tested. This may not work.
Review Comment:
@Tom-Newton It seems that we can't use `GetUserDelegationKey()` with account
key credential. We may need to other authentication to test
`GetUserDelegationKey()`. Do you know how to configure other authentication...?
(I think that we can defer this case to a separated issue.)
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41095: [C++][FS][Azure] Add support for CopyFile with hierarchical namespace support [arrow]
github-actions[bot] commented on PR #41276: URL: https://github.com/apache/arrow/pull/41276#issuecomment-2063201098 :warning: GitHub issue #41095 **has been automatically assigned in GitHub** to PR creator. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
