HDDS-13447. [S3G] ListObjectsV2 should accept maxKeys=0 #8833
Conversation
Change-Id: Id96bc33fb933334b8ff8c6869d3213beecc76118
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
Almost good to one. There's one potential corner case
| } | ||
| String lastKey = null; | ||
| int count = 0; | ||
| while (ozoneKeyIterator != null && ozoneKeyIterator.hasNext()) { |
There was a problem hiding this comment.
The get() mthod is too long, containing unrelated code pieces. Suggest to refactor it into multiple smaller helper methods in a follow up task.
| response.addPrefix(EncodingTypeObject.createNullable( | ||
| prefix + dirName + delimiter, encodingType)); | ||
| prevDir = dirName; | ||
| if (maxKeys > 0) { |
There was a problem hiding this comment.
potential bug:
If maxKeys = 0, it does not enter this if statement. Later, lastKey would still be null. If the bucket is not emtpy, and key iterator has next item, this line looks problematic:
ContinueToken nextToken = new ContinueToken(lastKey, prevDir);
response.setNextToken(nextToken.encodeToString());
|
@Jimmyweng006 would you like to take another look? Or would you be ok if I push the proposed fix in? |
|
Hi @jojochuang , sorry that I haven’t been able to focus on this issue recently. Please feel free to go ahead and push the commit. Thanks! |
peterxcli
left a comment
peterxcli
left a comment
There was a problem hiding this comment.
@jojochuang I have handled the case you indicated, please take another look. Thanks.
| } | ||
| String lastKey = null; | ||
| int count = 0; | ||
| while (ozoneKeyIterator != null && ozoneKeyIterator.hasNext()) { |
|
Thanks @peterxcli for helping on this PR. After re-run all CI pass. |
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
This PR fixes the bug, but a few nits were found in this piece of code that existed before.
I'll merge this one but we should refactor the code in a separate task.
| if (count < maxKeys) { | ||
| response.setTruncated(false); | ||
| } else if (ozoneKeyIterator.hasNext()) { | ||
| } else if (ozoneKeyIterator.hasNext() && lastKey != null) { |
There was a problem hiding this comment.
ozoneKeyIterator could be null and throw NPE here.
But that's an existing problem. Suggest to move to a new issue.
| if (maxKeys > 0) { | ||
| while (ozoneKeyIterator != null && ozoneKeyIterator.hasNext()) { | ||
| OzoneKey next = ozoneKeyIterator.next(); | ||
| if (bucket != null && bucket.getBucketLayout().isFileSystemOptimized() && |
There was a problem hiding this comment.
bucket is always non-null here.
|
Thanks @peterxcli for helping on the PR, and @jojochuang for the review! |
4 participants
What changes were proposed in this pull request?
[S3G] ListObjectsV2 should accept maxKeys=0
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13447
How was this patch tested?
CI Pass