-
Couldn't load subscription status.
- Fork 9.1k
HADOOP-16455. ABFS: Implement FileSystem.access() method. #1711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| public static final String FS_AZURE_USER_AGENT_PREFIX_KEY = "fs.azure.user.agent.prefix"; | ||
| public static final String FS_AZURE_SSL_CHANNEL_MODE_KEY = "fs.azure.ssl.channel.mode"; | ||
| public static final String FS_AZURE_ENABLE_CHECK_ACCESS = "fs.azure.enable.check.access"; | ||
| /** Provides a config to enable the checkAccess API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the comments above the new config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| return; | ||
| } | ||
| String relativePath = path.isRoot() ? "" : ("/" + getRelativePath(path)); | ||
| throwFileNotFoundIfNotExists(relativePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not do GetFileStatus call. Response for checkaccess call should be the source of response to client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| AbfsRestOperation abfsOp = this.client | ||
| .checkAccess(relativePath, mode.SYMBOL); | ||
| if (abfsOp.getResult().getStatusCode() != CHECK_ACCESS_SUCCESS) { | ||
| throw new AccessControlException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to return AccessControlException for any error server response, even if its a server system error.
If server is already returning AccessControlException in its response for forbidden, then can we not handle the response as other APIs do for error scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, changes suggested in tests so failures are more meaningful.
PR #1611 is about to go in -it will show what extra instrumentation you need to add the round the new rest call
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java
Outdated
Show resolved
Hide resolved
...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/ConfigurationKeys.java
Outdated
Show resolved
Hide resolved
...p-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCheckAccess.java
Outdated
Show resolved
Hide resolved
...p-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCheckAccess.java
Outdated
Show resolved
Hide resolved
...p-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCheckAccess.java
Outdated
Show resolved
Hide resolved
| this.testUserFs.access(path, fsAction); | ||
| } catch (AccessControlException e) { | ||
| isAccessible = false; | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to catch and rethrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| } | ||
| return isAccessible; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Show resolved
Hide resolved
| * @throws IOException see specific implementation | ||
| */ | ||
| @Override | ||
| public void access(final Path path, FsAction mode) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should {{getIsNamespaceEnabled()}} be checked? In the test, you use modifyAclEntries which requires hierarchical namespace to be enabled. Shouldn't this be the case for access() as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| throws AzureBlobFileSystemException { | ||
| LOG.debug("access for filesystem: {}, path: {}, mode: {}", | ||
| this.client.getFileSystem(), path, mode); | ||
| if (!this.abfsConfiguration.isCheckAccessEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed as a configuration? Shouldn't it be always valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the ABFS access() is a noop. Adding the functionality with a config flag to keep it backward compatible.
| public static final String FS_AZURE_USER_AGENT_PREFIX_KEY = "fs.azure.user.agent.prefix"; | ||
| public static final String FS_AZURE_SSL_CHANNEL_MODE_KEY = "fs.azure.ssl.channel.mode"; | ||
| /** Provides a config to enable/disable the checkAccess API. | ||
| * By default this will be false. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be in DEFAULT_ENABLE_CHECK_ACCESS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with the explicit changes, we still have the IDE playing import games.
I'm =0 on whether the access changes should be a config option and what the default should be.
From a test perspective, having it fixed to true is best.
If optional, from a UX perspective, enabling it the most forward looking
but from a backwards compatibility perspective -default = false
Now, what about documentation?
...p-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCheckAccess.java
Outdated
Show resolved
Hide resolved
...p-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemCheckAccess.java
Show resolved
Hide resolved
|
|
||
| import java.io.Closeable; | ||
| import java.io.File; | ||
| import java.io.FileNotFoundException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed according to checkstyle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
Checkstyle I'm not worried about those test name mismatches as they make sense and they are consistent with the rest. Unused imports should be removed. Other than that: it's ready to go in! |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
mvn -T 1C -Dparallel-tests=abfs -Dscale -DtestsThreadCount=1 clean verify Driver test results using an account with namespace support in Central India: Driver test results using an account without namespace support in Central India: |
|
🎊 +1 overall
This message was automatically generated. |
...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look great, +1 :)
|
+1, merged via github. If you want on branch-3.2, cherry pick, retest and provide a new PR |
mvn -T 1C -Dparallel-tests=abfs -Dscale -DtestsThreadCount=8 clean verify
Driver test results using an account with namespace support in Central India:
[INFO] Tests run: 42, Failures: 0, Errors: 0, Skipped: 0
[ERROR] Errors:
[ERROR] ITestAzureBlobFileSystemCLI>AbstractAbfsIntegrationTest.setup:137 ? AbfsRestOperation
[INFO]
[ERROR] Tests run: 405, Failures: 0, Errors: 1, Skipped: 33
[WARNING] Tests run: 192, Failures: 0, Errors: 0, Skipped: 24
Driver test results using an account without namespace support in Central India:
NSD
[INFO] Tests run: 42, Failures: 0, Errors: 0, Skipped: 0
[WARNING] Tests run: 405, Failures: 0, Errors: 0, Skipped: 220
WARNING] Tests run: 192, Failures: 0, Errors: 0, Skipped: 24