HADOOP-19089: [ABFS] Reverting Back Support of setXAttr() and getXAttr() on root path #6592
Conversation
This file contains 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
| public void testGetSetXAttrOnRoot() throws Exception { | ||
| AzureBlobFileSystem fs = getFileSystem(); | ||
| final Path testPath = new Path("/"); | ||
| testGetSetXAttrHelper(fs, testPath); |
There was a problem hiding this comment.
To prevent revert add of getFileSystemProperties and setFileSystemProperties for getXAttr / setXAttr in future, should we not remove this method, and additionally assert that the getFileSystemProperties and setFileSystemProperties of store are not called, but setPathProperties and getPathStatus of store is called..
There was a problem hiding this comment.
Lets add an assert that this should throw 4xx error.
There was a problem hiding this comment.
Reason for this comment:
- Lets say getFsProperties / setFSProperties are added again in future. This test will raise an CI issue on that patch.
There was a problem hiding this comment.
Makes sense.
Added back the negative test.
| ## <a name="KnownIssues"></a> Known Issues | ||
|
|
||
| Following failures are known and expected to fail as of now. | ||
| 1. AzureBlobFileSystem.setXAttr() and AzureBlobFileSystem.getXAttr() will fail when attempted on root ("/") path. |
There was a problem hiding this comment.
Lets add reason for the failure as well
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saxenapranav
reviewed
Mar 1, 2024
| try { | ||
| fs.getXAttr(testPath, attributeName); | ||
| } catch (AbfsRestOperationException e) { | ||
| Assertions.assertThat(e.getStatusCode()).isEqualTo(HTTP_BAD_REQUEST); |
There was a problem hiding this comment.
Assert will happen only if exception is thrown. We should make sure that exception does happen.. Lets use LambdaTestUtils.intercept. Similarly for the other setXAttr intercept
|
🎊 +1 overall
This message was automatically generated. |
anmolanmol1234
approved these changes
Mar 1, 2024
steveloughran
pushed a commit
that referenced
this pull request
Mar 25, 2024
…r() on root path (#6592) This reverts most of HADOOP-18869: [ABFS] Fix behavior of a File System APIs on root path (#6003). Calling getXAttr("/") or setXAttr("/") on an abfs container will fail with `Operation failed: "The request URI is invalid.", HTTP 400 Bad Request` This change is to ensure: * Consistency across ADLS clients * Consistency across authentication mechanisms. Contributed by Anuj Modi
|
merged to trunk, cherrypicked to branch 3.4, reran modified test. if you want branch-3.3.x support, do a test run yourself and push up a new pr |
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.
Description of PR
Jira: https://issues.apache.org/jira/browse/HADOOP-19089
A while back changes were made to support HDFS.setXAttr() and HDFS.getXAttr() on root path for ABFS Driver.
For these, filesystem level APIs were introduced and used to set/get metadata of container.
Refer to Jira: https://issues.apache.org/jira/browse/HADOOP-18869
Ideally, same set of APIs should be used, and root should be treated as a path like any other path.
This change is to avoid calling container APIs for these HDFS calls.
As a result of this these APIs will fail on root path (as earlier) because service does not support get/set of user properties on root path.
This change will also update the documentation to reflect that these operations are not supported on root path.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?