Support WASB scheme in ADLSFileIO #11504
Conversation
| @@ -93,7 +93,7 @@ public void applyClientConfiguration(String account, DataLakeFileSystemClientBui | |||
| if (connectionString != null && !connectionString.isEmpty()) { | |||
| builder.endpoint(connectionString); | |||
| } else { | |||
| builder.endpoint("https://" + account); | |||
| builder.endpoint("https://" + account + ".dfs.core.windows.net"); | |||
There was a problem hiding this comment.
Should we really change add .def.core.windows.net by default ?
I mean that use currently include the suffix in account so it should be clearly state that we append the suffix by default now.
There was a problem hiding this comment.
I believe the storageAccount of ADLSLocation should be used for storing the storage account name only. And since the ADLS APIs are accessed via dfs.core.windows.net by default, I think it's appropriate to append it to the storage account name here as the default. If users want to specify a different hostname, they can use the adls.connection-string property.
There was a problem hiding this comment.
I think this is the right thing to do, I would just throw a javadoc on this method though
There was a problem hiding this comment.
This deviates from the abfs[s] scheme defined by Hadoop, where the domain is specified in the URI (docs). I feel we should check if the account already ends with the domain and only append then to support that.
There was a problem hiding this comment.
I see, you are stripping off the domain below, so this isn't an issue. You can ignore the above.
There was a problem hiding this comment.
@bryanck + @danielcweeks - Please check, I was thinking this could be another candidate to put in 1.7.1 as well
There was a problem hiding this comment.
Thanks @mrcnc , yeah I think supporting WASB despite it being deprecated by Azure is worth it and I think the way this PR handles it is correct.
|
Thanks for the PR @mrcnc ! And for the reviews @RussellSpitzer @amogh-jahagirdar @jbonofre ! |
This is a second attempt to resolve #10127 but avoids using
java.net.URIfor parsing, which was found to be problematic after the first attempt was merged.Additionally I've refactored to minimize the number of lines changed and clarify the usage of the
storageAccountvariable, which was previously storing the endpoint host and now will only store the storage account name as the subdomain.