Repository navigation
fix(s3): filter objects during pagination in S3Service.list to avoid premature loop exit - #853
Conversation
There was a problem hiding this comment.
Hey @ADITYASINGH1206,
Took a look through your PR and really liked this fix. That pagination bug was subtle: if the first S3 page was full of files that did not match the filter, the old code was bailing out way too early and dropping files on later pages.
Filtering per page as the chunks come in rather than hoarding all raw objects in memory first is so much cleaner and avoids unnecessary memory allocations. Also, asserting verify(mockClient, times(1)) in the unit test to confirm it stops as soon as maxKeys is hit was a really nice touch.
Code looks super solid and clean to me. Hope the maintainers get a chance to review and merge it soon!
…premature loop exit
2ff380d to
de8966c
Compare
|
Thank you @knowprasad for the thorough review and kind words! Glad you liked the per-page streaming filter and maxKeys verification. |
…le the regExp once
… and share test setup
|
Thanks for the fix, filtering each page is the right way to solve #851. I made a few small changes on top. With a regexp or filter set, it now fetches full pages of 1000 keys. maxKeys 0 returns nothing without calling S3, and a negative value gives a clear error. One thing to know: DeleteList and Trigger now search the whole prefix for matches, not just the first page. I pushed these tweaks. |
Closes #851.
What problem does this PR solve?
In
S3Service.list(), pagination across S3 pages evaluates the termination conditionallContents.size() >= rMaxKeysagainst raw, unfiltered S3 objects before applyingregExpandfilter. When an early page contains objects that do not match the filter (for instance, non-matching files under a common prefix), the loop breaks prematurely once the raw count reachesrMaxKeys, silently dropping matching objects that reside on subsequent pages.What changes are made in this PR?
S3Service.filter(s3Object, regExp, filter)iteratively as each page's contents are received from S3.rMaxKeys.continuationTokenwhenever the current page does not provide enough matching objects andresponse.isTruncated()is true.S3ServiceListTest.java:shouldContinuePaginationWhenInitialPageDoesNotMatchFilter(): verifies that non-matching objects on page 1 do not prematurely stop pagination, allowing matching objects on page 2 to be returned.shouldStopPaginationWhenMatchingCountReachesMaxKeys(): verifies that pagination stops immediately once the requestedmaxKeysquota of matching objects is reached.