ChaladiMohanVamsi commented on code in PR #13241:
URL: https://github.com/apache/iceberg/pull/13241#discussion_r2164207641
##########
azure/src/integration/java/org/apache/iceberg/azure/adlsv2/ADLSFileIOTest.java:
##########
@@ -190,4 +200,188 @@ public void
testSerialization(TestHelpers.RoundTripSerializer<FileIO> roundTripS
assertThat(testFileIO.properties()).isEqualTo(roundTripSerializedFileIO.properties());
}
+
+ @Test
+ public void noStorageCredentialConfigured() {
+ ADLSFileIO fileIO = new ADLSFileIO();
+ fileIO.setCredentials(ImmutableList.of());
+ fileIO.initialize(
+ ImmutableMap.of(
+ sasTokenForAccount("account1"), "sasTokenFromProperties1",
+ sasTokenForAccount("account2"), "sasTokenFromProperties2"));
+
+ assertThat(fileIO.clientForStoragePath(abfsPath("my-container1",
"account1", "/path/to/file")))
+ .isSameAs(
+ fileIO.clientForStoragePath(abfsPath("my-container2", "account2",
"/path/to/file")))
+ .isSameAs(fileIO.clientForStoragePath(wasbPath("container", "account",
"/path/to/file")))
+ .isSameAs(fileIO.clientForStoragePath(abfsPath("random", "account",
"/path/to/file")));
+
+ assertThat(
+ fileIO
+ .clientForStoragePath(abfsPath("my-container1", "account1",
"/path/to/file"))
+ .azureProperties())
+ .extracting("adlsSasTokens")
+ .asInstanceOf(InstanceOfAssertFactories.map(String.class,
String.class))
+ .containsEntry(accountHost("account1"), "sasTokenFromProperties1")
+ .containsEntry(accountHost("account2"), "sasTokenFromProperties2");
+ }
+
+ @Test
+ public void singleStorageCredentialConfigured() {
+ StorageCredential adlsCredential =
+ StorageCredential.create(
+ abfsPath("custom-container", "account1", ""),
+ ImmutableMap.of(sasTokenForAccount("account1"),
"sasTokenFromCredential"));
+ ADLSFileIO fileIO = new ADLSFileIO();
+ fileIO.setCredentials(ImmutableList.of(adlsCredential));
+ fileIO.initialize(
+ ImmutableMap.of(
+ sasTokenForAccount("account1"), "sasTokenFromProperties1",
+ sasTokenForAccount("account2"), "sasTokenFromProperties2"));
+
+ // verify that the generic ADLS client is used if the storage prefix
doesn't match the prefixes
+ // in the storage credentials
+ assertThat(
+ fileIO.clientForStoragePath(abfsPath("custom-container",
"account1", "/path/to/file")))
+ .isNotSameAs(
+ fileIO.clientForStoragePath(abfsPath("my-container", "account1",
"/path/to/file")));
+
+ assertThat(fileIO.clientForStoragePath(abfsPath("my-container",
"account1", "/path/to/file")))
+ .isSameAs(
+ fileIO.clientForStoragePath(abfsPath("my-container2", "account2",
"/path/to/file")));
+
+ // Custom credentials from storage credentials.
+ assertThat(
+ fileIO
+ .clientForStoragePath(abfsPath("custom-container", "account1",
"/path/to/file"))
+ .azureProperties())
+ .extracting("adlsSasTokens")
+ .asInstanceOf(InstanceOfAssertFactories.map(String.class,
String.class))
+ .containsEntry(accountHost("account1"), "sasTokenFromCredential")
+ .containsEntry(accountHost("account2"), "sasTokenFromProperties2");
+
+ // Generic credentials from properties
+ assertThat(
+ fileIO
+ .clientForStoragePath(abfsPath("my-container1", "account1",
"/path/to/file"))
+ .azureProperties())
+ .extracting("adlsSasTokens")
+ .asInstanceOf(InstanceOfAssertFactories.map(String.class,
String.class))
+ .containsEntry(accountHost("account1"), "sasTokenFromProperties1")
+ .containsEntry(accountHost("account2"), "sasTokenFromProperties2");
+ }
+
+ @Test
+ public void multipleStorageCredentialsConfigured() {
+ StorageCredential adlsCredential1 =
+ StorageCredential.create(
+ abfsPath("custom-container", "account1", "/table1"),
+ ImmutableMap.of(sasTokenForAccount("account1"),
"sasTokenFromCredential1"));
+ StorageCredential adlsCredential2 =
+ StorageCredential.create(
+ abfsPath("custom-container", "account1", "/table2"),
+ ImmutableMap.of(sasTokenForAccount("account1"),
"sasTokenFromCredential2"));
+ ADLSFileIO fileIO = new ADLSFileIO();
+ fileIO.setCredentials(ImmutableList.of(adlsCredential1, adlsCredential2));
+ fileIO.initialize(
+ ImmutableMap.of(
+ sasTokenForAccount("account1"), "sasTokenFromProperties1",
+ sasTokenForAccount("account2"), "sasTokenFromProperties2"));
+
+ // verify that the generic ADLS client is used if the storage prefix
doesn't match the prefixes
+ // in the storage credentials
+ assertThat(fileIO.clientForStoragePath(abfsPath("custom-container",
"account1", "/table1")))
+ .isNotSameAs(
+ fileIO.clientForStoragePath(abfsPath("custom-container",
"account1", "/table2")))
+ .isNotSameAs(
+ fileIO.clientForStoragePath(abfsPath("my-container", "account1",
"/path/to/file")));
Review Comment:
Unlike S3FileIO and GCSFileIO we are not caching the clients in ADLSFileIO,
we are always creating a new client with reused HTTP client during any FileIO
operation. I kept the behaviour same even with prefix based clients. Hence we
are not validating client as they would be different for each file.
I am not completely sure of overall benefit we can get out of caching the
clients as we are already reusing HTTP client. Wanted your inputs if we should
go in this route of caching the clients similar to other FileIO implementations.
cc// @nastra
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
