HADOOP-19595. ABFS: AbfsConfiguration should store account type information (HNS or FNS) #7765
+217
−70
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
manika137
reviewed
Jun 27, 2025
| void setIsNamespaceEnabledAccount(String isNamespaceEnabledAccount) { | ||
| this.isNamespaceEnabledAccount = isNamespaceEnabledAccount; | ||
| public void setIsNamespaceEnabledAccount(Trilean isNamespaceEnabledAccount) { | ||
| isNamespaceEnabled = isNamespaceEnabledAccount; |
There was a problem hiding this comment.
why did we remove the **this.**isNamespaceEnabled part?
There was a problem hiding this comment.
reverted this.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
manika137
reviewed
Jun 27, 2025
| @@ -443,7 +428,7 @@ private synchronized boolean getNamespaceEnabledInformationFromServer( | |||
| */ | |||
| @VisibleForTesting | |||
| boolean isNamespaceEnabled() throws TrileanConversionException { | |||
| return this.isNamespaceEnabled.toBoolean(); | |||
| return abfsConfiguration.getIsNamespaceEnabledAccount().toBoolean(); | |||
There was a problem hiding this comment.
here also should we have a try-catch block for TrileanConversionException like we have in AbfsClient-line 1722?
There was a problem hiding this comment.
This method should throw an exception if namespace enabled isn't initialized, so a try-catch block isn't necessary here.
manika137
reviewed
Jun 30, 2025
| * @throws Exception if any error occurs during configuration setup or evaluation | ||
| */ | ||
| @Test | ||
| public void testNameSpaceConfigNotSet() throws Exception { |
There was a problem hiding this comment.
Would it be better if we have a single test to verify the expected isnamespace enabled value for all 3 cases? We could set and reset the values along with the respective assertions
There was a problem hiding this comment.
Yaa, make sense. I can make it one and will write similar test to set the config from store.
manika137
reviewed
Jun 30, 2025
| * @throws Exception if any error occurs during configuration setup or evaluation | ||
| */ | ||
| @Test | ||
| public void testNameSpaceConfigNotSet() throws Exception { |
There was a problem hiding this comment.
Should we also test if setting and getting namespace enabled values from store and client level would point to abfsconfiguration class?
There was a problem hiding this comment.
There is already a test case ensureGetAclDetermineHnsStatusAccurately() for this, will add check on client's getIsNamespace as well in the same method.
bhattmanish98
changed the title
bhattmanish98
changed the title
| * @throws Exception if any error occurs during configuration setup or evaluation | ||
| */ | ||
| @Test | ||
| public void testNameSpaceConfig() throws Exception { |
There was a problem hiding this comment.
Here also validate that client returns the same value as what is set in the config
| // In DFS endpoint, renamePath is O(1) API call and idempotency issue can happen. | ||
| // For blob endpoint, client orchestrates the rename operation. | ||
| assumeDfsServiceType(); | ||
| AzureBlobFileSystem fs = getFileSystem(); | ||
| Assume.assumeTrue(!getIsNamespaceEnabled(fs)); |
There was a problem hiding this comment.
Use isHnsDisabled function
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Code Changes looks good. Added a few comments.
One test sugestion. Even without any config present, just after the file system init without making any hdfs call, both store and client should be able to get the value of isNamespace enabled without an network calls
| @@ -443,7 +428,7 @@ private synchronized boolean getNamespaceEnabledInformationFromServer( | |||
| */ | |||
| @VisibleForTesting | |||
| boolean isNamespaceEnabled() throws TrileanConversionException { | |||
| return this.isNamespaceEnabled.toBoolean(); | |||
| return abfsConfiguration.getIsNamespaceEnabledAccount().toBoolean(); | |||
There was a problem hiding this comment.
Better to use getter here and other places for abfsconfiguration. This will make sure same object is being referred everywhere and can easily be mocked if needed in future.
| @@ -1525,8 +1529,8 @@ void setMaxBackoffIntervalMilliseconds(int maxBackoffInterval) { | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| void setIsNamespaceEnabledAccount(String isNamespaceEnabledAccount) { | |||
| this.isNamespaceEnabledAccount = isNamespaceEnabledAccount; | |||
| public void setIsNamespaceEnabledAccount(Trilean isNamespaceEnabledAccount) { | |||
There was a problem hiding this comment.
We should be setting this value on abfsconfiguration object only when we know the exact value. Do we really need this to be Trilean? I think we should pass the definitive argument and do the Boolean to Trilean conversion here just to make sure that no one accidently sets UNKNOWN here.
There was a problem hiding this comment.
Make sense, made changes accordingly.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
============================================================
|
|
🎊 +1 overall
This message was automatically generated. |
bhattmanish98
added a commit
to ABFSDriver/AbfsHadoop
that referenced
this pull request
Jul 7, 2025
anujmodi2021
approved these changes
Jul 7, 2025
manika137
reviewed
Jul 8, 2025
| @@ -1726,18 +1727,15 @@ protected String getUserAgent() { | |||
| * Checks if the namespace is enabled. | |||
| * | |||
| * @return True if the namespace is enabled, false otherwise. | |||
| * @throws AzureBlobFileSystemException if the conversion fails. | |||
There was a problem hiding this comment.
Should this be AbfsDriverException?
There was a problem hiding this comment.
AzureBlobFileSystemException is parent of AbfsDriverException, so fine to throw it like this,
manika137
approved these changes
Jul 8, 2025
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| return isNamespaceEnabled(); | ||
| } | ||
| try { | ||
| LOG.debug("Get root ACL status"); | ||
| getClient(AbfsServiceType.DFS).getAclStatus(AbfsHttpConstants.ROOT_PATH, tracingContext); | ||
| // If getAcl succeeds, namespace is enabled. | ||
| setNamespaceEnabled(Trilean.TRUE); | ||
| setNamespaceEnabled(true); |
There was a problem hiding this comment.
Use true false variable from AbfsHttpConstants
There was a problem hiding this comment.
This is the boolean value, don't think we need to get it from AbfsHttpConstants.
| && abfsConfiguration.getFsConfiguredServiceType() == BLOB) { | ||
| // Current filesystem init restricts HNS-Blob combination | ||
| // so namespace check not required. | ||
| if (BLOB == abfsConfiguration.getFsConfiguredServiceType()) { |
There was a problem hiding this comment.
Why was the FNS check removed ?
There was a problem hiding this comment.
Currently, we only support FNS over Blob endpoint. HNS-Blob will fail during init only.
After having offline discussion with the team, I made this change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA: https://issues.apache.org/jira/browse/HADOOP-19595
Currently, we keep the namespaceEnabled information at both the client and store levels, updating it in both locations when a filesystem is created. With this PR, we've moved this information to the ABFS configuration class, allowing anyone who needs it to access it from there. This update helps minimize confusion and removes data redundancy.