Skip to content
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

Empty listing Bug Fix and Other Pending Changes #144

Merged
merged 10 commits into from
Dec 3, 2024

Conversation

anujmodi2021
Copy link
Collaborator

No description provided.

/*
* Non-hierarchical-namespace account can not have a customer-provided-key(CPK).
* Fail initialization of filesystem if the configs are provided. CPK is of
* two types: GLOBAL_KEY, and ENCRYPTION_CONTEXT.
*/
if ((isEncryptionContextCPK(abfsConfiguration) || isGlobalKeyCPK(
abfsConfiguration))
&& !getIsNamespaceEnabled(new TracingContext(tracingContext))) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing the close() method call here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We needed close because we were checking it after creating filesystem.
Now we are checking it before creating file system and failing, so no need to close.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a significant change will trigger a CI.
We can wait for CI results before merging this.

@@ -672,7 +674,7 @@ protected void assumeBlobServiceType() {
*/
protected void assertPathDns(Path path) {
String expectedDns = getAbfsServiceType() == AbfsServiceType.BLOB
? ABFS_BLOB_DOMAIN_NAME : ABFS_DFS_DOMAIN_NAME;
? ABFS_BLOB_DOMAIN_NAME : ABFS_DFS_DOMAIN_NAME;;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double semicolon

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken

@@ -175,7 +175,7 @@ public static String getMaskedUrl(URL url) {

public static URL changeUrlFromBlobToDfs(URL url) throws InvalidUriException {
try {
url = new URL(url.toString().replace(ABFS_BLOB_DOMAIN_NAME, ABFS_DFS_DOMAIN_NAME));
url = new URL(url.toString().replaceFirst(ABFS_BLOB_DOMAIN_NAME, ABFS_DFS_DOMAIN_NAME));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have more than one .blob. in the url ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated code as per the discussion offline

@@ -184,13 +186,30 @@ public static URL changeUrlFromBlobToDfs(URL url) throws InvalidUriException {

public static URL changeUrlFromDfsToBlob(URL url) throws InvalidUriException {
try {
url = new URL(url.toString().replace(ABFS_DFS_DOMAIN_NAME, ABFS_BLOB_DOMAIN_NAME));
int startIndex = url.toString().indexOf("//") + 2;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two can be moved inside replacedUrl(0 function only

Copy link
Collaborator

@snvijaya snvijaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment in AbfsBlobClient. Please take a look before checkin.

@@ -646,12 +646,10 @@ public AbfsRestOperation listPath(final String relativePath, final boolean recur
abfsUriQueryBuilder.addQuery(QUERY_PARAM_COMP, LIST);
abfsUriQueryBuilder.addQuery(QUERY_PARAM_INCLUDE, METADATA);
abfsUriQueryBuilder.addQuery(QUERY_PARAM_PREFIX, getDirectoryQueryParameter(relativePath));
abfsUriQueryBuilder.addQuery(QUERY_PARAM_MARKER, continuation);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (continuation != null) {
  abfsUriQueryBuilder.addQuery(QUERY_PARAM_MARKER, continuation);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a redundant check. Same check is already added in AbfsUriQueryBuilder.java

public void addQuery(final String name, final String value) {
    if (value != null && !value.isEmpty()) {
      this.parameters.put(name, value);
    }
  }

@anujmodi2021
Copy link
Collaborator Author


:::: AGGREGATED TEST RESULT ::::

============================================================
HNS-OAuth-DFS

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 4
[WARNING] Tests run: 784, Failures: 0, Errors: 0, Skipped: 179
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 51

============================================================
HNS-SharedKey-DFS

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 5
[WARNING] Tests run: 784, Failures: 0, Errors: 0, Skipped: 131
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 38

============================================================
NonHNS-SharedKey-DFS

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 768, Failures: 0, Errors: 0, Skipped: 371
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 39

============================================================
AppendBlob-HNS-OAuth-DFS

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 4
[WARNING] Tests run: 784, Failures: 0, Errors: 0, Skipped: 182
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 77

============================================================
NonHNS-SharedKey-Blob

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 11
[WARNING] Tests run: 768, Failures: 0, Errors: 0, Skipped: 285
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 39

============================================================
NonHNS-OAuth-DFS

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 768, Failures: 0, Errors: 0, Skipped: 376
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 52

============================================================
NonHNS-OAuth-Blob

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 11
[ERROR] Tests run: 768, Failures: 0, Errors: 0, Skipped: 289
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 52

============================================================
AppendBlob-NonHNS-OAuth-Blob

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 11
[WARNING] Tests run: 768, Failures: 0, Errors: 0, Skipped: 306
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 78

============================================================
HNS-Oauth-DFS-IngressBlob

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 4
[WARNING] Tests run: 784, Failures: 0, Errors: 0, Skipped: 308
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 51

============================================================
NonHNS-OAuth-DFS-IngressBlob

[WARNING] Tests run: 163, Failures: 0, Errors: 0, Skipped: 10
[WARNING] Tests run: 768, Failures: 0, Errors: 0, Skipped: 372
[WARNING] Tests run: 440, Failures: 0, Errors: 0, Skipped: 52

Time taken: 183 mins 4 secs.

@anujmodi2021 anujmodi2021 merged commit c085ff4 into wasbDepCodeReview Dec 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants