diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java b/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java index 4362ad68af3dd..de93bcaedf1eb 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java @@ -184,16 +184,22 @@ private static class TopicPartitionEntry { // responses which are due to the retention period elapsing, and those which are due to actual lost data. private long lastAckedOffset; + private static final Comparator PRODUCER_BATCH_COMPARATOR = (b1, b2) -> { + if (b1.baseSequence() < b2.baseSequence()) return -1; + else if (b1.baseSequence() > b2.baseSequence()) return 1; + else return b1.equals(b2) ? 0 : 1; + }; + TopicPartitionEntry() { this.producerIdAndEpoch = ProducerIdAndEpoch.NONE; this.nextSequence = 0; this.lastAckedSequence = NO_LAST_ACKED_SEQUENCE_NUMBER; this.lastAckedOffset = ProduceResponse.INVALID_OFFSET; - this.inflightBatchesBySequence = new TreeSet<>(Comparator.comparingInt(ProducerBatch::baseSequence)); + this.inflightBatchesBySequence = new TreeSet<>(PRODUCER_BATCH_COMPARATOR); } void resetSequenceNumbers(Consumer resetSequence) { - TreeSet newInflights = new TreeSet<>(Comparator.comparingInt(ProducerBatch::baseSequence)); + TreeSet newInflights = new TreeSet<>(PRODUCER_BATCH_COMPARATOR); for (ProducerBatch inflightBatch : inflightBatchesBySequence) { resetSequence.accept(inflightBatch); newInflights.add(inflightBatch); diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java index 4227db5e61e62..64be3aeaf47b2 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java @@ -674,6 +674,68 @@ public void testBatchCompletedAfterProducerReset() { assertNull(transactionManager.nextBatchBySequence(tp0)); } + @Test + public void testDuplicateSequenceAfterProducerReset() throws Exception { + initializeTransactionManager(Optional.empty()); + initializeIdempotentProducerId(producerId, epoch); + + Metrics metrics = new Metrics(time); + final int requestTimeout = 10000; + final int deliveryTimeout = 15000; + + RecordAccumulator accumulator = new RecordAccumulator(logContext, 16 * 1024, CompressionType.NONE, 0, 0L, + deliveryTimeout, metrics, "", time, apiVersions, transactionManager, + new BufferPool(1024 * 1024, 16 * 1024, metrics, time, "")); + + Sender sender = new Sender(logContext, this.client, this.metadata, accumulator, false, + MAX_REQUEST_SIZE, ACKS_ALL, MAX_RETRIES, new SenderMetricsRegistry(metrics), this.time, requestTimeout, + 0, transactionManager, apiVersions); + + assertEquals(0, transactionManager.sequenceNumber(tp0).intValue()); + + Future responseFuture1 = accumulator.append(tp0, time.milliseconds(), "1".getBytes(), "1".getBytes(), Record.EMPTY_HEADERS, + null, MAX_BLOCK_TIMEOUT, false, time.milliseconds()).future; + sender.runOnce(); + assertEquals(1, transactionManager.sequenceNumber(tp0).intValue()); + + time.sleep(requestTimeout); + sender.runOnce(); + assertEquals(0, client.inFlightRequestCount()); + assertTrue(transactionManager.hasInflightBatches(tp0)); + assertEquals(1, transactionManager.sequenceNumber(tp0).intValue()); + sender.runOnce(); // retry + assertEquals(1, client.inFlightRequestCount()); + assertTrue(transactionManager.hasInflightBatches(tp0)); + assertEquals(1, transactionManager.sequenceNumber(tp0).intValue()); + + time.sleep(5000); // delivery time out + sender.runOnce(); + + // The retried request will remain inflight until the request timeout + // is reached even though the delivery timeout has expired and the + // future has completed exceptionally. + assertTrue(responseFuture1.isDone()); + TestUtils.assertFutureThrows(responseFuture1, TimeoutException.class); + assertFalse(transactionManager.hasInFlightRequest()); + assertEquals(1, client.inFlightRequestCount()); + + sender.runOnce(); // bump the epoch + assertEquals(epoch + 1, transactionManager.producerIdAndEpoch().epoch); + assertEquals(0, transactionManager.sequenceNumber(tp0).intValue()); + + Future responseFuture2 = accumulator.append(tp0, time.milliseconds(), "2".getBytes(), "2".getBytes(), Record.EMPTY_HEADERS, + null, MAX_BLOCK_TIMEOUT, false, time.milliseconds()).future; + sender.runOnce(); + sender.runOnce(); + assertEquals(0, transactionManager.firstInFlightSequence(tp0)); + assertEquals(1, transactionManager.sequenceNumber(tp0).intValue()); + + time.sleep(5000); // request time out again + sender.runOnce(); + assertTrue(transactionManager.hasInflightBatches(tp0)); // the latter batch failed and retried + assertFalse(responseFuture2.isDone()); + } + private ProducerBatch writeIdempotentBatchWithValue(TransactionManager manager, TopicPartition tp, String value) {