Repository navigation
CAMEL-25760: camel-alibaba-oss - skip a listed object that cannot be got instead of failing the poll - #28075
Conversation
…got instead of failing the poll The consumer gets every object of a listed page. An object that no longer exists was skipped, but any other failure of a get (access denied for that object, an archived object that is not restored) failed the whole poll, and the continuation token was only moved on after the batch was processed, so the next poll listed the same page again. One such object kept the consumer from consuming any object of its page and of the following pages, for good. As camel-aws2-s3 does since CAMEL-25558: a failed get of one listed object is reported to the exception handler of the consumer with its key and the object is skipped for this poll, the other objects are consumed, and the poll fails only when none of the listed objects could be got. The continuation token is taken from the listing before the objects are got, as camel-aws2-s3 and camel-huaweicloud-obs do with their marker, so a page whose objects cannot be got does not hold the consumer back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for this, and for keeping it so close to the camel-aws2-s3 fix from CAMEL-25558: the collect-then-decide loop, the "none could be got, fail the poll" rule and the exception-handler message all match, so the three object-store consumers now act the same way.
What I checked:
- Moving the continuation token on before the gets is the right call here. Before, the token only moved after a successful batch, so one bad object pinned the consumer to its page. The upgrade-guide entry covers that the next page is read even after a failed poll.
- The in-progress and not-found skips (CAMEL-25551) are unchanged, and
releaseNotProcessedstill runs when the poll fails. - The tests use AssertJ like the rest of the module, Awaitility instead of sleeps, and the 3 cases cover the skip, the next page, and the poll that must still fail.
Minor points, none blocking:
- An object that stays forbidden is reported at
WARNwith a stack trace at every poll that lists it (every 500 ms by default in a bucket that fits in one page). That is the same as camel-aws2-s3 and the upgrade guide says so, so I'm fine with it. A user who finds it noisy can set anexceptionHandler. - A page whose other objects are all still in flight (or deleted) counts as "none could be got", so one forbidden object on that page fails the poll. That matches S3, and because the token has already moved on, it doesn't block anything.
- Could you file the camel-minio and camel-google-storage follow-ups you mention in JIRA, so they don't get lost?
CI was just approved and is still running. Approving on the code, assuming CI comes back green.
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 17 (1 retried attempt) Recovered flaky tests on JDK 17 (1)
🔬 Scalpel shadow comparison — Scalpel: 8 of 707 tested, 26 compile-only — current: 8 all testedMaveniverse Scalpel detected 8 affected modules (current approach: 8). Skip-tests mode would test 8 modules (2 direct + 7 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (8)
Modules with tests skipped (26)
All tested modules (35 modules, 6m 8s total)Total reactor time: 6m 8s
Top 20 slowest modules:
|
|
Closing and reopening to re-run CI against main: the Claude Code on behalf of Claus Ibsen |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
One follow-up on the test, to keep it from becoming flaky: see the two suggestions below.
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…test As suggested in the review, the object is deleted when the exchange completes, after it reached mock:result, so the test waits for the bucket content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. No issues found.
Correct port of the CAMEL-25558 collect-then-decide pattern from camel-aws2-s3. The anyGot/failures logic traces correctly through all edge cases (in-progress skips, deleted objects, forbidden objects, empty-page); continuation token advancement before the get loop is sound and well-documented; tests 1 and 2 would definitively fail without the fix (old code throws on the first 403 and never processes the remaining objects; test 2's consumer gets stuck on the forbidden-only page with no token advancement). No gaps vs davsclaus's review found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @allthingssecurity. bd114c4 waits for the bucket with Awaitility (atMost(20, SECONDS)) in both deleteAfterRead tests, so the check no longer races the delete that runs when the exchange completes. objects is a ConcurrentSkipListSet, so reading it from the test thread while the consumer deletes is safe. The third test expects no deletes, so it has no such race. The assertions stay AssertJ, as in the rest of the class. I resolved my two threads.
Claude Code on behalf of Claus Ibsen
3 participants
Description
CAMEL-25760
The OSS consumer lists one page of the bucket and gets every listed object. An object that no longer exists is skipped (CAMEL-25551); any other failure of a get was rethrown, so the whole poll failed, and the continuation token was only moved on after the batch was processed. The next poll therefore listed the same page again. One object whose get always fails (access denied for that object, an Archive or Cold Archive object that is not restored, a disabled KMS key) blocked every other object of its page and all the following pages; with a bucket that fits in one page the consumer consumed nothing, until someone removed or restored the object.
This change does what camel-aws2-s3 does since CAMEL-25558:
An object deleted after the listing is still skipped at
DEBUG. The behaviour change (a skipped object is reported at every poll that lists it; a failed poll no longer repeats the same page) is in the 4.24 upgrade guide.Tests:
OSSConsumerObjectThatCannotBeGotTest(3 tests) with a fake bucket that lists its keys in order,maxKeysat a time with a continuation token, and answers the get of a forbidden object with 403. Without the fixobjectThatCannotBeGotDoesNotBlockTheOtherObjects(a.txt, forbidden b.txt, c.txt) andpageOfObjectsThatCannotBeGotDoesNotBlockTheNextPage(maxMessagesPerPoll=1, forbidden a.txt) receive no object at all.pollFailsWhenNoListedObjectCanBeGotpasses with and without the fix; it is there because the fix must not swallow that case.deleteAfterRead=truean object is deleted when its exchange completes, which is after it reachedmock:result. The test class passed 3 runs in a row.How it was found: comparing the sibling object-store consumers (camel-aws2-s3 after CAMEL-25558, camel-huaweicloud-obs, camel-alibaba-oss) on the same listing, get and commit steps. camel-huaweicloud-obs also fails the poll for one object, but moves its marker on, so it stops for good only when the bucket fits in one page (fixed separately). camel-alibaba-oss also keeps its listing position, so it stops for good whatever the size of the bucket.
Not changed here: camel-minio (a failed get in
MinioConsumer.processBatchstops the rest of the batch and fails the poll) and camel-google-storage (GoogleCloudStorageConsumer.addExchangerethrows, and the poll fails) have the same kind of defect; they are follow-ups.Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.(I built and tested camel-alibaba-oss, including the formatter and import-sort plugins. I did not run the full root build.)
AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a
Co-Authored-Bytrailer.Claude Code on behalf of allthingssecurity
🤖 Generated with Claude Code