Add delimiter support for S3 List API #2996
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2996 +/- ##
============================================
+ Coverage 64.24% 69.95% +5.70%
- Complexity 10398 12153 +1755
============================================
Files 840 889 +49
Lines 71755 75098 +3343
Branches 8611 8988 +377
============================================
+ Hits 46099 52532 +6433
+ Misses 23004 19828 -3176
- Partials 2652 2738 +86 ☔ View full report in Codecov by Sentry. |
| @@ -706,10 +709,27 @@ private Page<NamedBlobRecord> run_list_v2(String accountName, String containerNa | |||
| int resultIndex = 0; | |||
| while (resultSet.next()) { | |||
| String blobName = resultSet.getString(1); | |||
| if (resultIndex++ == maxKeysValue) { | |||
| if (resultIndex == maxKeysValue) { | |||
There was a problem hiding this comment.
we only set nextContinuationToken to the blobName when resultIndex == maxKeysValue. The idea is to only do that when it's the last key, but now that we change the logic when incrementing resultIndex. resultIndex would not equal to maxKeysValue in the last key, so we probably have to use an other index, or other way to set next continuation token here.
There was a problem hiding this comment.
Sure. Reverted back to previous logic of incrementing resultIndex on every iteration and setting nextContinuationToken to the blobName when resultIndex == maxKeysValue
| // Extract the portion after the prefix and before the next '/' | ||
| String remainingPath = blobName.substring(blobNamePrefix == null ? 0 : blobNamePrefix.length()); | ||
| remainingPath = remainingPath.startsWith("/") ? remainingPath.substring(1) : remainingPath; | ||
| int delimiterIndex = remainingPath.indexOf("/"); |
There was a problem hiding this comment.
let's use a constant for "/".
| if (groupDirectories) { | ||
| // Extract the portion after the prefix and before the next '/' | ||
| String remainingPath = blobName.substring(blobNamePrefix == null ? 0 : blobNamePrefix.length()); | ||
| remainingPath = remainingPath.startsWith("/") ? remainingPath.substring(1) : remainingPath; |
There was a problem hiding this comment.
this is not right, if the prefix is "abc" and this blobname is "abc/efg/hij", and the delimiter is "/", then the we should "abc/" as common prefix, not "abc/efg/".
There was a problem hiding this comment.
Sure! Corrected it
| if (groupDirectories) { | ||
| // Add the directories to the result | ||
| entries.addAll(directories.stream() | ||
| .map(directory -> new NamedBlobRecord(accountName, containerName, directory, null, Utils.Infinite_Time, 0, |
There was a problem hiding this comment.
This is not right, when we are returning a common prefix, it should include the prefix as well. if we have "abc/def/ghi", and the prefix is "abc/", and the common prefix should be "abc/def/", not jsut "def/".
There was a problem hiding this comment.
Corrected it..
| remainingPath = remainingPath.startsWith("/") ? remainingPath.substring(1) : remainingPath; | ||
| int delimiterIndex = remainingPath.indexOf("/"); | ||
| if (delimiterIndex != -1) { | ||
| boolean validEntry = directories.add(remainingPath.substring(0, delimiterIndex) + "/"); |
There was a problem hiding this comment.
Nit: it could be remainingPath.substring(0, delimiterIndex +DELIMITER_STRING.length());
ambry-named-mysql/src/main/java/com/github/ambry/named/MySqlNamedBlobDb.java
Outdated
Show resolved
Hide resolved
ambry-frontend/src/main/java/com/github/ambry/frontend/s3/S3ListHandler.java
Outdated
Show resolved
Hide resolved
Arun-LinkedIn
force-pushed
the
AddS3DelimiterSupport
branch
from
de4a0e4 to
0657d1c
Compare
This adds support for treating "/" in blob names as delimiter. It allows to treat prefixes as "subdirectories" and group them under "CommonPrefixes" response in the LIST API. This also enables us to list and delete directories using AWS S3 CLIs
More details can be found in https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html under "CommonPrefixes" section.