Skip to content

HDDS-7713. listKeys with prefix path as key for FSO buckets is redundant#5170

Merged
sadanand48 merged 5 commits intoapache:masterfrom
Tejaskriya:HDDS-7713
Aug 14, 2023
Merged

HDDS-7713. listKeys with prefix path as key for FSO buckets is redundant#5170
sadanand48 merged 5 commits intoapache:masterfrom
Tejaskriya:HDDS-7713

Conversation

@Tejaskriya
Copy link
Copy Markdown
Contributor

@Tejaskriya Tejaskriya commented Aug 10, 2023

What changes were proposed in this pull request?

The redundant listing of keys was originating from the client module (it was visible from both aws s3 ls and ozone sh key list interfaces). The change proposed is to not add the prefix to the result list if the prefix is for a key and not a directory.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7713

How was this patch tested?

Tested manually using docker.
Through S3:

$ aws s3 ls --endpoint http://localhost:9878 s3://buck1/k1  
2023-08-10 13:37:14          3 k1

Through ozone sh:

$ ozone sh key list s3v/buck1 --prefix k1
[ {
  "volumeName" : "s3v",
  "bucketName" : "buck1",
  "name" : "k1",
  "dataSize" : 3,
  "creationTime" : "2023-08-10T08:07:13.130Z",
  "modificationTime" : "2023-08-10T08:07:14.138Z",
  "replicationConfig" : {
    "replicationFactor" : "THREE",
    "requiredNodes" : 3,
    "replicationType" : "RATIS"
  },
  "metadata" : { },
  "file" : true
} ]

Also added a test in testListKeysAtDifferentLevels() of TestObjectStoreWithFSO.

@Tejaskriya Tejaskriya marked this pull request as ready for review August 10, 2023 08:34
@whbing
Copy link
Copy Markdown
Contributor

whbing commented Aug 11, 2023

LGTM Overall. Thanks ! Here's a minor suggestion from my perspective.
We can add test in TestListKeysWithFSO to compare the listKeys results of legacyBucket and fsoBucket. I've written a demo test in TestListKeysWithFSO:

  @Test
  public void testListKeysWithValidStartKey() throws Exception {
    // case-10: keyPrefix corresponds an exist file
    expectedKeys =
        getExpectedKeyList("a1/b3/e3/e31.tx", "", legacyOzoneBucket);
    checkKeyList("a1/b3/e3/e31.tx", "", expectedKeys, fsoOzoneBucket);
  }

Result will be Actual :[a1/b3/e3/e31.tx, a1/b3/e3/e31.tx] without PR (Test Failed) and Actual :[a1/b3/e3/e31.tx] with this PR (Test passed).

@whbing
Copy link
Copy Markdown
Contributor

whbing commented Aug 11, 2023

NIT: title can be specified FSO

@Tejaskriya Tejaskriya changed the title HDDS-7713. listKeys for prefix path as key is redundant HDDS-7713. listKeys with prefix path as key for FSO buckets is redundant Aug 12, 2023
@Tejaskriya
Copy link
Copy Markdown
Contributor Author

// case-10: keyPrefix corresponds an exist file
    expectedKeys =
        getExpectedKeyList("a1/b3/e3/e31.tx", "", legacyOzoneBucket);
    checkKeyList("a1/b3/e3/e31.tx", "", expectedKeys, fsoOzoneBucket);

Thank you for the suggestion, I have added the test case you've specified in TestListKeysWithFSO

@whbing
Copy link
Copy Markdown
Contributor

whbing commented Aug 13, 2023

+1, once ci passed.

Copy link
Copy Markdown
Contributor

@sadanand48 sadanand48 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@aryangupta1998 aryangupta1998 left a comment

Choose a reason for hiding this comment

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

LGTM, +1

@sadanand48 sadanand48 merged commit 3b3742c into apache:master Aug 14, 2023
@sadanand48
Copy link
Copy Markdown
Contributor

Thanks @Tejaskriya for the patch, @whbing @aryangupta1998 for the review.

@Tejaskriya Tejaskriya deleted the HDDS-7713 branch October 31, 2023 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants