Fixed issue for committing offsets in BatchConsumer when almost all m…

…essages are filtered
allegro · Oct 2, 2024 · 21d4673 · 21d4673
1 parent 9615199
commit 21d4673
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 2 deletions.
diff --git a/hermes-consumers/src/main/java/pl/allegro/tech/hermes/consumers/consumer/BatchConsumer.java b/hermes-consumers/src/main/java/pl/allegro/tech/hermes/consumers/consumer/BatchConsumer.java
@@ -180,7 +180,8 @@ public void initialize() {
                 compositeMessageContentWrapper,
                 topic,
                 trackers,
-                loadRecorder
+                loadRecorder,
+                this::commitIfReady
         );
         metrics.initialize();
     }

diff --git a/...s/src/main/java/pl/allegro/tech/hermes/consumers/consumer/batch/MessageBatchReceiver.java b/...s/src/main/java/pl/allegro/tech/hermes/consumers/consumer/batch/MessageBatchReceiver.java
@@ -42,14 +42,16 @@ public class MessageBatchReceiver {
     private final Topic topic;
     private final SubscriptionLoadRecorder loadRecorder;
     private boolean receiving = true;
+    private final Runnable commitIfReady;
 
     public MessageBatchReceiver(MessageReceiver receiver,
                                 MessageBatchFactory batchFactory,
                                 MessageConverterResolver messageConverterResolver,
                                 CompositeMessageContentWrapper compositeMessageContentWrapper,
                                 Topic topic,
                                 Trackers trackers,
-                                SubscriptionLoadRecorder loadRecorder) {
+                                SubscriptionLoadRecorder loadRecorder,
+                                Runnable commitIfReady) {
         this.receiver = receiver;
         this.batchFactory = batchFactory;
         this.messageConverterResolver = messageConverterResolver;
@@ -58,6 +60,7 @@ public MessageBatchReceiver(MessageReceiver receiver,
         this.trackers = trackers;
         this.loadRecorder = loadRecorder;
         this.inflight = new ArrayDeque<>(1);
+        this.commitIfReady = commitIfReady;
     }
 
     public MessageBatchingResult next(Subscription subscription, Runnable signalsInterrupt) {
@@ -74,6 +77,7 @@ public MessageBatchingResult next(Subscription subscription, Runnable signalsInt
         while (isReceiving() && !batch.isReadyForDelivery() && !Thread.currentThread().isInterrupted()) {
             loadRecorder.recordSingleOperation();
             signalsInterrupt.run();
+            commitIfReady.run();
             Optional<Message> maybeMessage = inflight.isEmpty()
                     ? readAndTransform(subscription, batch.getId())
                     : Optional.ofNullable(inflight.poll());

diff --git a/...s/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/BatchDeliveryTest.java b/...s/src/integrationTest/java/pl/allegro/tech/hermes/integrationtests/BatchDeliveryTest.java
@@ -118,6 +118,44 @@ public void shouldFilterIncomingEventsForBatch() {
         );
     }
 
+    @Test
+    public void shouldCommitFilteredMessagesForBatch() {
+        // given
+        TestSubscriber subscriber = subscribers.createSubscriber();
+        Topic topic = hermes.initHelper().createTopic(topicWithRandomName().build());
+        final Subscription subscription = hermes.initHelper().createSubscription(subscriptionWithRandomName(topic.getName(), subscriber.getEndpoint())
+                .withSubscriptionPolicy(buildBatchPolicy()
+                        .withBatchSize(10)
+                        .withBatchTime(Integer.MAX_VALUE)
+                        .withBatchVolume(1024)
+                        .build())
+                .withFilter(MESSAGE_NAME_FILTER)
+                .build());
+
+        // when
+        hermes.api().publishUntilSuccess(topic.getQualifiedName(), ALICE.asJson());
+        hermes.api().publishUntilSuccess(topic.getQualifiedName(), ALICE.asJson());
+        hermes.api().publishUntilSuccess(topic.getQualifiedName(), ALICE.asJson());
+
+        // then
+        waitAtMost(Duration.ofSeconds(10)).untilAsserted(() ->
+                hermes.api().getConsumersMetrics()
+                        .expectStatus()
+                        .isOk()
+                        .expectBody(String.class)
+                        .value((body) -> assertThatMetrics(body)
+                                .contains("hermes_consumers_subscription_filtered_out_total")
+                                .withLabels(
+                                        "group", topic.getName().getGroupName(),
+                                        "subscription", subscription.getName(),
+                                        "topic", topic.getName().getName()
+                                )
+                                .withValue(3.0)
+                        )
+        );
+        hermes.api().waitUntilConsumerCommitsOffset(topic.getQualifiedName(), subscription.getName());
+    }
+
     @Test
     public void shouldDeliverBatchInGivenTimePeriod() {
         // given