diff --git a/spring-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java b/spring-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java index 6bf94c9a..7eaa064e 100644 --- a/spring-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java +++ b/spring-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java @@ -88,6 +88,7 @@ import org.springframework.util.concurrent.ListenableFutureCallback; * @author Loic Talhouarne * @author Vladimir Tsanev * @author Yang Qiju + * @author Tom van den Berge */ public class KafkaMessageListenerContainer extends AbstractMessageListenerContainer { @@ -1003,7 +1004,20 @@ public class KafkaMessageListenerContainer extends AbstractMessageListener } catch (RuntimeException e) { if (this.containerProperties.isAckOnError() && !this.autoCommit && producer == null) { - this.acks.add(record); + if (this.isRecordAck) { + Map offsetsToCommit = + Collections.singletonMap(new TopicPartition(record.topic(), record.partition()), + new OffsetAndMetadata(record.offset() + 1)); + if (this.containerProperties.isSyncCommits()) { + this.consumer.commitSync(offsetsToCommit); + } + else { + this.consumer.commitAsync(offsetsToCommit, this.commitCallback); + } + } + else if (!this.isAnyManualAck) { + this.acks.add(record); + } } if (this.errorHandler == null) { throw e; diff --git a/spring-kafka/src/test/java/org/springframework/kafka/listener/KafkaMessageListenerContainerTests.java b/spring-kafka/src/test/java/org/springframework/kafka/listener/KafkaMessageListenerContainerTests.java index 506d53fc..7b91cc39 100644 --- a/spring-kafka/src/test/java/org/springframework/kafka/listener/KafkaMessageListenerContainerTests.java +++ b/spring-kafka/src/test/java/org/springframework/kafka/listener/KafkaMessageListenerContainerTests.java @@ -89,6 +89,7 @@ import org.springframework.kafka.test.utils.KafkaTestUtils; * @author Martin Dam * @author Artem Bilan * @author Loic Talhouarne + * @author Tom van den Berge */ public class KafkaMessageListenerContainerTests { @@ -128,10 +129,14 @@ public class KafkaMessageListenerContainerTests { private static String topic17 = "testTopic17"; + private static String topic18 = "testTopic18"; + + private static String topic19 = "testTopic19"; + @ClassRule - public static KafkaEmbedded embeddedKafka = new KafkaEmbedded(1, true, topic1, topic2, topic3, topic4, topic5, - topic6, topic7, topic8, topic9, topic10, topic11, topic12, topic13, topic14, topic15, topic16, - topic17); + public static KafkaEmbedded embeddedKafka = new KafkaEmbedded(1, true, topic5, + topic6, topic7, topic8, topic9, topic10, topic11, topic12, topic13, topic14, topic15, topic16, topic17, topic18, + topic19); @Rule public TestName testName = new TestName(); @@ -1518,6 +1523,101 @@ public class KafkaMessageListenerContainerTests { this.logger.info("Stop JSON2"); } + @Test + public void testRebalanceAfterFailedRecord() throws Exception { + logger.info("Start rebalance after failed record"); + Map props = KafkaTestUtils.consumerProps("test18", "false", embeddedKafka); + DefaultKafkaConsumerFactory cf = new DefaultKafkaConsumerFactory<>(props); + ContainerProperties containerProps = new ContainerProperties(topic18); + final List counts = new ArrayList<>(); + counts.add(new AtomicInteger()); + counts.add(new AtomicInteger()); + containerProps.setMessageListener(new MessageListener() { + + @Override + public void onMessage(ConsumerRecord message) { + // The 1st message per partition fails + if (counts.get(message.partition()).incrementAndGet() < 2) { + throw new RuntimeException("Failure wile processing message"); + } + } + }); + containerProps.setSyncCommits(true); + containerProps.setAckMode(AckMode.RECORD); + final CountDownLatch rebalanceLatch = new CountDownLatch(2); + containerProps.setConsumerRebalanceListener(new ConsumerRebalanceListener() { + + @Override + public void onPartitionsRevoked(Collection partitions) { + } + + @Override + public void onPartitionsAssigned(Collection partitions) { + logger.info("manual ack: assigned " + partitions); + rebalanceLatch.countDown(); + } + }); + + CountDownLatch stubbingComplete1 = new CountDownLatch(1); + KafkaMessageListenerContainer container1 = + spyOnContainer(new KafkaMessageListenerContainer<>(cf, containerProps), stubbingComplete1); + container1.setBeanName("testRebalanceAfterFailedRecord"); + container1.start(); + Consumer containerConsumer = spyOnConsumer(container1); + final CountDownLatch commitLatch = new CountDownLatch(2); + willAnswer(invocation -> { + + @SuppressWarnings({ "unchecked" }) + Map map = invocation.getArgument(0); + try { + return invocation.callRealMethod(); + } + finally { + for (Entry entry : map.entrySet()) { + // Decrement when the last (successful) has been committed + if (entry.getValue().offset() == 2) { + commitLatch.countDown(); + } + } + } + + }).given(containerConsumer).commitSync(any()); + stubbingComplete1.countDown(); + ContainerTestUtils.waitForAssignment(container1, embeddedKafka.getPartitionsPerTopic()); + + Map senderProps = KafkaTestUtils.producerProps(embeddedKafka); + ProducerFactory pf = new DefaultKafkaProducerFactory<>(senderProps); + KafkaTemplate template = new KafkaTemplate<>(pf); + template.setDefaultTopic(topic18); + template.sendDefault(0, 0, "foo"); + template.sendDefault(1, 0, "baz"); + template.sendDefault(0, 0, "bar"); + template.sendDefault(1, 0, "qux"); + template.flush(); + + // Wait until both partitions have committed offset 2 (i.e. the last message) + assertThat(commitLatch.await(30, TimeUnit.SECONDS)).isTrue(); + + // Start a 2nd consumer, triggering a rebalance + KafkaMessageListenerContainer container2 = + new KafkaMessageListenerContainer<>(cf, containerProps); + container2.setBeanName("testRebalanceAfterFailedRecord2"); + container2.start(); + // Wait until both consumers have finished rebalancing + assertThat(rebalanceLatch.await(60, TimeUnit.SECONDS)).isTrue(); + + // Stop both consumers + container1.stop(); + container2.stop(); + Consumer consumer = cf.createConsumer(); + consumer.assign(Arrays.asList(new TopicPartition(topic18, 0), new TopicPartition(topic18, 1))); + + // Verify that offset of both partitions is the highest committed offset + assertThat(consumer.position(new TopicPartition(topic18, 0))).isEqualTo(2); + assertThat(consumer.position(new TopicPartition(topic18, 1))).isEqualTo(2); + consumer.close(); + logger.info("Stop rebalance after failed record"); + } private Consumer spyOnConsumer(KafkaMessageListenerContainer container) { Consumer consumer = spy( KafkaTestUtils.getPropertyValue(container, "listenerConsumer.consumer", Consumer.class)); @@ -1526,8 +1626,10 @@ public class KafkaMessageListenerContainerTests { return consumer; } - private KafkaMessageListenerContainer spyOnContainer(KafkaMessageListenerContainer container, + private KafkaMessageListenerContainer spyOnContainer( + KafkaMessageListenerContainer container, final CountDownLatch stubbingComplete) { + KafkaMessageListenerContainer spy = spy(container); willAnswer(i -> { if (stubbingComplete.getCount() > 0 && Thread.currentThread().getName().endsWith("-C-1")) {