Unify fallback dependent api by anmolanmol1234 · Pull Request #69 · ABFSDriver/AbfsHadoop

anmolanmol1234 · 2023-06-13T05:10:24Z

PR to make sure that if any API fallbacks to DFS endpoint, the dependent getFileStatus call should also go to DFS endpoint. Also refactored the code for openFileForRead and openFileForWrite to depend only on getFileStatus call.

anmolanmol1234 · 2023-06-13T07:34:04Z

:::: AGGREGATED TEST RESULT ::::

HNS-OAuth

[INFO] Results:
[INFO]
[WARNING] Tests run: 111, Failures: 0, Errors: 0, Skipped: 4
[INFO] Results:
[INFO]
[WARNING] Tests run: 746, Failures: 0, Errors: 0, Skipped: 157
[INFO] Results:
[INFO]
[WARNING] Tests run: 273, Failures: 0, Errors: 0, Skipped: 41

HNS-SharedKey

[INFO] Results:
[INFO]
[WARNING] Tests run: 111, Failures: 0, Errors: 0, Skipped: 4
[INFO] Results:
[INFO]
[WARNING] Tests run: 746, Failures: 0, Errors: 0, Skipped: 157
[INFO] Results:
[INFO]
[WARNING] Tests run: 273, Failures: 0, Errors: 0, Skipped: 41

NonHNS-SharedKey

[INFO] Results:
[INFO]
[WARNING] Tests run: 111, Failures: 0, Errors: 0, Skipped: 4
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAzureBlobFileSystemExplictImplicitRename.testRenameImplicitDirectoryToExplicitDirectoryDestinationHavingSameNameSubFile:559->explicitImplicitDirectoryRenameTestWithDestPathNames:796->createSourcePaths:835->AbstractAbfsIntegrationTest.createAzCopyDirectory:524 » IO
[INFO]
[ERROR] Tests run: 746, Failures: 0, Errors: 1, Skipped: 275
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAzureBlobFileSystemBasics.tearDown:59->FileSystemContractBaseTest.tearDown:85->FileSystemContractBaseTest.cleanupDir:92 » TestTimedOut
[INFO]
[ERROR] Tests run: 273, Failures: 0, Errors: 1, Skipped: 45

NonHNS-OAuth

[INFO] Results:
[INFO]
[WARNING] Tests run: 111, Failures: 0, Errors: 0, Skipped: 4
[INFO] Results:
[INFO]
[WARNING] Tests run: 746, Failures: 0, Errors: 0, Skipped: 275
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] ITestAzureBlobFileSystemBasics.tearDown:59->FileSystemContractBaseTest.tearDown:82->FileSystemContractBaseTest.cleanupDir:92 » TestTimedOut
[INFO]
[ERROR] Tests run: 273, Failures: 0, Errors: 1, Skipped: 45

AppendBlob-HNS-OAuth

[INFO] Results:
[INFO]
[WARNING] Tests run: 111, Failures: 0, Errors: 0, Skipped: 4
[INFO] Results:
[INFO]
[WARNING] Tests run: 746, Failures: 0, Errors: 0, Skipped: 157
[INFO] Results:
[INFO]
[WARNING] Tests run: 273, Failures: 0, Errors: 0, Skipped: 41

anujmodi2021

Added a small comment. Rest LGTM

anujmodi2021 · 2023-06-16T05:38:06Z

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java

-      }
-      if (getAbfsStore().getAbfsConfiguration().getPrefixMode()
+            tracingContext, useBlobEndpoint);
+        if (getAbfsStore().getAbfsConfiguration().getPrefixMode()


Should be changed to getAbfsStore().getPrefixMode()

saxenapranav

Mostly look amazing. Two comments.

saxenapranav · 2023-06-20T05:34:42Z

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java

+    boolean useBlobEndpoint = !(OperativeEndpoint.isIngressEnabledOnDFS(prefixMode, abfsConfiguration) ||
+            OperativeEndpoint.isMkdirEnabledOnDFS(prefixMode, abfsConfiguration) ||
+            OperativeEndpoint.isReadEnabledOnDFS(prefixMode, abfsConfiguration));
    try {


why does redirect to dfs on mkdir/ingress/readEnabled matter here?

We want the getFileStatus call to go to dfs in all cases where either of the configs is enabled, though this doesn't come as part of fallback but helps to maintain consistency in the logic

saxenapranav · 2023-06-20T05:39:01Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

-        } catch (AbfsRestOperationException ex) {
-          if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) {
+          fileStatus = (VersionedFileStatus) getFileStatus(new Path(relativePath), tracingContext, useBlobEndpoint);
+        } catch (IOException ex) {


if we keep it as AbfsRestOperationException, the behavior should be same?

getFileStatus throws IOException and hence we would need to change the exception type here. It will catch AbfsRestOperationException as well as that extends from IOException

saxenapranav · 2023-06-20T07:00:01Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

-          if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) {
+          fileStatus = (VersionedFileStatus) getFileStatus(new Path(relativePath), tracingContext, useBlobEndpoint);
+        } catch (IOException ex) {
+          AbfsRestOperationException ex1 = (AbfsRestOperationException) ex;


It will lead to cast issue. Since all IOException may not be AbfsRestOperationException.

saxenapranav · 2023-06-20T07:06:26Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

-        } catch (AbfsRestOperationException ex) {
-          if (ex.getStatusCode() == HttpURLConnection.HTTP_NOT_FOUND) {
+          fileStatus = (VersionedFileStatus) getFileStatus(new Path(relativePath), tracingContext, useBlobEndpoint);
+        } catch (IOException ex) {


saxenapranav · 2023-06-20T10:35:28Z

...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java

-                    + "to honor single writer semantics");
+          fileStatus = (VersionedFileStatus) getFileStatus(new Path(relativePath), tracingContext, useBlobEndpoint);
+        } catch (IOException ex) {
+          if (ex instanceof AbfsRestOperationException) {


since we are still throwing IOexception if not abfsRestOperationException. Lets catch AbfsRestOperationException only?

Makes sense

saxenapranav

LGTM! Thanks for taking all the comments.

snvijaya · 2023-06-26T06:40:20Z

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java

+    PrefixMode prefixMode = getAbfsStore().getPrefixMode();
+    AbfsConfiguration abfsConfiguration = getAbfsStore().getAbfsConfiguration();

+    boolean useBlobEndpoint = !(OperativeEndpoint.isIngressEnabledOnDFS(prefixMode, abfsConfiguration) ||


As discussed, let operativeEndpoint by the single point with the unified logic.

anmolanmol1234 added 7 commits June 11, 2023 21:44

File status

ce92ee4

Make dependent fallback

f03da1e

Test fixes

efc7b69

Test fix

e216b92

Removing comments

f0a0d8d

Fix spacing

cd0b502

Fix generic exception

91d3b12

anmolanmol1234 requested review from anujmodi2021, saxenapranav, snvijaya and sreeb-msft June 13, 2023 05:10

anmolanmol1234 self-assigned this Jun 13, 2023

Merge branch 'ABFS_3.3.2_dev' into unifyFallbackDependentAPI

6db6bf3

anujmodi2021 reviewed Jun 16, 2023

View reviewed changes

anmolanmol1234 added 3 commits June 19, 2023 21:59

Resolving merge conflict

d6cce65

PR comments

6c8d08e

Fix merge conflicts

5ebd42c

saxenapranav requested changes Jun 20, 2023

View reviewed changes

Cast fix

768b75a

saxenapranav reviewed Jun 20, 2023

View reviewed changes

anmolanmol1234 added 2 commits June 20, 2023 05:19

Exception fix

56c6914

space fix

8766c54

saxenapranav approved these changes Jun 21, 2023

View reviewed changes

anmolanmol1234 merged commit f0b75b7 into ABFS_3.3.2_dev Jun 23, 2023

snvijaya reviewed Jun 26, 2023

View reviewed changes

Conversation

anmolanmol1234 commented Jun 13, 2023

Uh oh!

anmolanmol1234 commented Jun 13, 2023

HNS-OAuth

HNS-SharedKey

NonHNS-SharedKey

NonHNS-OAuth

AppendBlob-HNS-OAuth

Uh oh!

anujmodi2021 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saxenapranav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saxenapranav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants