allthingssecurity commented on code in PR #27674:
URL: https://github.com/apache/camel/pull/27674#discussion_r4240520347
##########
components/camel-slack/src/main/java/org/apache/camel/component/slack/SlackConsumer.java:
##########
@@ -77,20 +82,55 @@ protected int poll() throws Exception {
// Maximum limit is 1000. Slack recommends no more than 200 results at
a time.
// https://api.slack.com/methods/conversations.history
// We set the limit to 1 the first call to set the timestamp of the
last message of the history
+ ConversationsHistoryResponse response = conversationsHistory(null);
+
+ // okay we have some response from slack so lets mark the consumer as
ready
+ forceConsumerAsReady();
+
+ List<Message> messages = response.getMessages();
+ if (timestamp != null && response.isHasMore()) {
+ // the most recent messages of the range come first, so the
messages that did not fit into this response
+ // are older ones: read the following pages, or they are skipped
when the timestamp moves past them
+ messages = messages != null ? new ArrayList<>(messages) : new
ArrayList<>();
+ String cursor = nextCursor(response);
+ while (cursor != null) {
Review Comment:
Applied in e7caab3a845c, with two differences from the suggested line, both
checked with a test:
- `isRunAllowed()` is still true for a suspended consumer (`BaseService`:
`status >= STARTING && status <= SUSPENDED`),
so the loop uses `isPollAllowed()`, which is what `ScheduledPollConsumer`
checks before a poll.
- Leaving the loop and routing the pages read so far would move the
timestamp to the most recent message, so the older
messages that were not read would be skipped after a resume or restart,
which is the defect this PR fixes. The poll
now returns 0 without routing and without moving the timestamp, and the
next poll reads the range again
(`SlackConsumer.java:97`).
New test `stopsPagingWhenSuspendedAndReadsTheMessagesAgainAfterResume`: the
consumer is suspended while it waits for
the second of three pages; the third page is not requested, nothing is
routed, and after the resume the five messages
are routed once, in order. On the previous commit, and with `while (cursor
!= null && isRunAllowed())`, it fails with
`expected: <[2]> but was: <[2, 4]>`. It also matters for stop: the graceful
shutdown waits for a poll in progress, so
a long paging run held it up. Module suite: 10 tests, 0 failures (1 skipped,
needs a Slack token).
_Claude Code on behalf of allthingssecurity_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
