feat(ABFS): Support SAS and OAuth config for ABFS #11623
Conversation
zhli1142015
requested review from
majetideepak and
assignUser
as code owners
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
cc @majetideepak , thanks. I offline verified this change. |
|
@zhli1142015 Thanks for this contribution! It is better to have a separate PR for the dependency change to test the code changes against a new image with these dependencies. |
|
I see, here is the Pr for dep update, please help review it. Thanks. |
zhli1142015
force-pushed
the
abfs_auth_more
branch
3 times, most recently
from
3460abc
to
9cb4099
Compare
|
@majetideepak , could you help to review the change? |
| return url_; | ||
| } | ||
|
|
||
| std::string urlWithSasToken() const { |
There was a problem hiding this comment.
These APIs are only used for testing. Can we remove these, create the read/write clients here, and then use something like GetAccountInfo from the client object in the tests?
https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/storage/azure-storage-blobs/inc/azure/storage/blobs/blob_client.hpp
There was a problem hiding this comment.
Updated. Only available function is getUrl. Others like GetAccountInfo need to retrieve info from remote server.
| // account specific variant). The supported values are SharedKey, OAuth and SAS. | ||
| static std::string kAzureAccountAuthType{"fs.azure.account.auth.type"}; | ||
|
|
||
| static std::string kAzureAccountKey{"fs.azure.account.key"}; |
There was a problem hiding this comment.
We also need to fix the config names to be consistent with other Velox filesystems.
can we use hive.abfs prefix instead of fs.azure?
There was a problem hiding this comment.
These are user-facing configurations that have consistently been referred to by these names. Using a different prefix might cause confusion for users.
zhli1142015
force-pushed
the
abfs_auth_more
branch
from
dd5de10
to
144b95d
Compare
zhli1142015
force-pushed
the
abfs_auth_more
branch
from
144b95d
to
5401bb8
Compare
zhli1142015
force-pushed
the
abfs_auth_more
branch
from
5401bb8
to
bc6cc65
Compare
No description provided.