HADOOP-19766: ABFS. Force Blob Instance for FNS Accounts #8152
Conversation
|
💔 -1 overall
This message was automatically generated. |
Test Results============================================================
|
| */ | ||
| private boolean isClosed = true; | ||
| private final String fileSystemId = UUID.randomUUID().toString(); | ||
| private final String DFS_DOMAIN_INDICATOR = ".dfs."; |
There was a problem hiding this comment.
We have a constant already defined for this, we can use that
| dfsDomain); | ||
|
|
||
| // Initialize filesystem with DFS endpoint | ||
| AzureBlobFileSystem fs = |
There was a problem hiding this comment.
Should be inside try to autoclose
| throw new InvalidConfigurationValueException(FS_AZURE_ACCOUNT_IS_HNS_ENABLED, ex); | ||
| } | ||
|
|
||
| // For FNS-DFS accounts, reset the endpoint to Blob and update the tracing |
There was a problem hiding this comment.
Nit: use block comment as above
| */ | ||
| private boolean isClosed = true; | ||
| private final String fileSystemId = UUID.randomUUID().toString(); | ||
| private final String DFS_DOMAIN_INDICATOR = ".dfs."; |
| * Resets all service types to use BLOB. | ||
| * Updates the client to reflect the new default service type. | ||
| */ | ||
| public void resetEndpointforFNS() { |
There was a problem hiding this comment.
Nit: camel case in method name
| * | ||
| * @return the default ingress service type | ||
| */ | ||
| public AbfsServiceType getDefaultIngressServiceType() { |
There was a problem hiding this comment.
methond name should be getngressServiceType()
| * | ||
| * @return the default service type | ||
| */ | ||
| public AbfsServiceType getDefaultServiceType() { |
There was a problem hiding this comment.
Not used, can be removed
| * @return the configured AbfsServiceType from the URL | ||
| */ | ||
| public AbfsServiceType getFsConfiguredServiceTypeFromURL() { | ||
| return fsConfiguredServiceType; |
There was a problem hiding this comment.
Nit: Better to change this variable name also to fsConfiguredServiceTypeFromUrl
| * Sets the configured service type to BLOB. | ||
| * Majorly required to correctly set user agent for FNS-Blob | ||
| */ | ||
| void setFsConfiguredServiceTypetoBlob() { |
There was a problem hiding this comment.
Nit: camel case in method name
There was a problem hiding this comment.
Better to have generic method that can accept Service Type and caller can call it with Blob
| */ | ||
| public void resetEndpointforFNS() { | ||
| getAbfsConfiguration().setFsConfiguredServiceTypetoBlob(); | ||
| getClientHandler().setDefaultServiceType(AbfsServiceType.BLOB); |
There was a problem hiding this comment.
Repeated use of gets, can be initialized in a variable
| </property> | ||
| ``` | ||
|
|
||
| - ABFS allows you to implement your custom SAS Token Provider. The declared class must implement |
There was a problem hiding this comment.
Was this expected to be covered as part of this PR ?
There was a problem hiding this comment.
No. We had earlier mentioned to add configs for both OAuth and delegated SAS by referring to the sections above. For SAS however, the configs remain constant since we dont have multiple implementations of it (unlike OAuth).
Would be better to mention all fixed configs ones at one place so user only needs to check configs for OAuth implementation
There was a problem hiding this comment.
Better readability since its less scattered now
| import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_ACCOUNT_KEY_PROPERTY_NAME; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.FS_AZURE_INGRESS_SERVICE_TYPE; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.ConfigurationKeys.accountProperty; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.*; |
There was a problem hiding this comment.
- imports should be reverted back
| import static org.apache.hadoop.fs.azurebfs.services.RetryPolicyConstants.EXPONENTIAL_RETRY_POLICY_ABBREVIATION; | ||
| import static org.apache.hadoop.test.LambdaTestUtils.intercept; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.Mockito.*; |
| * @throws Exception if the test fails | ||
| */ | ||
| @Test | ||
| public void testFNSDfsUsesBlobInstance() throws Exception { |
There was a problem hiding this comment.
Ingress type validations should also be added that no matter what user configures for FNS it should be BLOB and also around the changes made in AbfsOutputStream class for dfstoBlobFallback
| ContextEncryptionAdapter contextEncryptionAdapter, | ||
| TracingContext tracingContext) { | ||
| int bufferSize = abfsConfiguration.getWriteBufferSize(); | ||
| getClientHandler().initServiceType(abfsConfiguration); |
There was a problem hiding this comment.
Why is it needed? During client handler init we do initialize the service types.
There was a problem hiding this comment.
This was purely was test purpose. We had some tests that validate ingress handlers, clients by changing the service type using the configuration
Now since we use clientHandler as source of truth for these values- we were trying to reset it here. For users- nothing changes since they cant change the config value mid-workload
There was a problem hiding this comment.
Rethought on this- better to remove it and change the tests to set service types through clienthandler instead before validating
This comment was marked as outdated.
This comment was marked as outdated.
|
💔 -1 overall
This message was automatically generated. |
| * Restricts the service type to BLOB for the current client handler and configuration when FNS account detected | ||
| * This method sets the ingress service type and the configured service type to {@code AbfsServiceType.BLOB}. | ||
| */ | ||
| public void restrictServiceTypeToBlob() { |
There was a problem hiding this comment.
Why 2 different methods?
Can't we combine them. Any way they are called together
There was a problem hiding this comment.
Makes sense. Combined them
|
💔 -1 overall
This message was automatically generated. |
| private String header = EMPTY_STRING; | ||
| private String ingressHandler = EMPTY_STRING; | ||
| private Boolean fnsEndpointConverted = false; | ||
| private String fnsEndptConvertedIndicator = "T"; |
There was a problem hiding this comment.
Add comment to specify reason to choose T as an indicator.
|
💔 -1 overall
This message was automatically generated. |
| private AbfsRestOperation executedRestOperation; | ||
| private String continuationToken; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Is this a requirement from Javadoc report?
|
Spotbug reported is not related to the changes done in this patch. |
5 participants
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-19766
We have observed cases of ABFS driver being initialised with DFS for FNS accounts which is not a recommended endpoint by the service.
To prevent this, we’re normalizing endpoint to Blob for FNS accounts even if they were initialized with DFS endpoint. We also shall restrict usage of a separate ingress service type for FNS accounts.
How was this patch tested?
Test suite was run. Results of which are added in the comments below