From 37edeed0777bacb3dc34e97bc6959bf9d043de2d Mon Sep 17 00:00:00 2001 From: David Jacot Date: Wed, 12 Jan 2022 10:01:10 +0100 Subject: [PATCH 01/51] Bump version to 3.1.0 --- gradle.properties | 2 +- streams/quickstart/java/pom.xml | 2 +- .../java/src/main/resources/archetype-resources/pom.xml | 2 +- streams/quickstart/pom.xml | 2 +- tests/kafkatest/__init__.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/gradle.properties b/gradle.properties index 57328013de1ee..85abd73ef8b41 100644 --- a/gradle.properties +++ b/gradle.properties @@ -20,7 +20,7 @@ group=org.apache.kafka # - tests/kafkatest/__init__.py # - tests/kafkatest/version.py (variable DEV_VERSION) # - kafka-merge-pr.py -version=3.1.0-SNAPSHOT +version=3.1.0 scalaVersion=2.13.6 task=build org.gradle.jvmargs=-Xmx2g -Xss4m -XX:+UseParallelGC diff --git a/streams/quickstart/java/pom.xml b/streams/quickstart/java/pom.xml index 8b254a2c4abf8..0f93c6ef2ad7e 100644 --- a/streams/quickstart/java/pom.xml +++ b/streams/quickstart/java/pom.xml @@ -26,7 +26,7 @@ org.apache.kafka streams-quickstart - 3.1.0-SNAPSHOT + 3.1.0 .. diff --git a/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml b/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml index b5962319f1f01..07a0b675513f3 100644 --- a/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml +++ b/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml @@ -29,7 +29,7 @@ UTF-8 - 3.1.0-SNAPSHOT + 3.1.0 1.7.7 1.2.17 diff --git a/streams/quickstart/pom.xml b/streams/quickstart/pom.xml index 08b97772297db..e063a39839b63 100644 --- a/streams/quickstart/pom.xml +++ b/streams/quickstart/pom.xml @@ -22,7 +22,7 @@ org.apache.kafka streams-quickstart pom - 3.1.0-SNAPSHOT + 3.1.0 Kafka Streams :: Quickstart diff --git a/tests/kafkatest/__init__.py b/tests/kafkatest/__init__.py index af57ff3cb3a41..7f331f93d441e 100644 --- a/tests/kafkatest/__init__.py +++ b/tests/kafkatest/__init__.py @@ -22,4 +22,4 @@ # Instead, in development branches, the version should have a suffix of the form ".devN" # # For example, when Kafka is at version 1.0.0-SNAPSHOT, this should be something like "1.0.0.dev0" -__version__ = '3.1.0.dev0' +__version__ = '3.1.0' From 4818aa79190615c2088de586155d3f76d4738ac4 Mon Sep 17 00:00:00 2001 From: David Jacot Date: Fri, 21 Jan 2022 14:54:31 +0100 Subject: [PATCH 02/51] MINOR: Update 3.1 branch version to 3.1.1-SNAPSHOT --- docs/js/templateData.js | 2 +- gradle.properties | 2 +- kafka-merge-pr.py | 2 +- streams/quickstart/java/pom.xml | 2 +- .../java/src/main/resources/archetype-resources/pom.xml | 2 +- streams/quickstart/pom.xml | 2 +- tests/kafkatest/__init__.py | 2 +- tests/kafkatest/version.py | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/js/templateData.js b/docs/js/templateData.js index 45e1eb284a2ae..d54d15b8f12c8 100644 --- a/docs/js/templateData.js +++ b/docs/js/templateData.js @@ -19,6 +19,6 @@ limitations under the License. var context={ "version": "31", "dotVersion": "3.1", - "fullDotVersion": "3.1.0", + "fullDotVersion": "3.1.1-SNAPSHOT", "scalaVersion": "2.13" }; diff --git a/gradle.properties b/gradle.properties index 85abd73ef8b41..50746b6195b2f 100644 --- a/gradle.properties +++ b/gradle.properties @@ -20,7 +20,7 @@ group=org.apache.kafka # - tests/kafkatest/__init__.py # - tests/kafkatest/version.py (variable DEV_VERSION) # - kafka-merge-pr.py -version=3.1.0 +version=3.1.1-SNAPSHOT scalaVersion=2.13.6 task=build org.gradle.jvmargs=-Xmx2g -Xss4m -XX:+UseParallelGC diff --git a/kafka-merge-pr.py b/kafka-merge-pr.py index abe7cbdd1abf1..5292b9131c247 100755 --- a/kafka-merge-pr.py +++ b/kafka-merge-pr.py @@ -70,7 +70,7 @@ DEV_BRANCH_NAME = "trunk" -DEFAULT_FIX_VERSION = os.environ.get("DEFAULT_FIX_VERSION", "3.1.0") +DEFAULT_FIX_VERSION = os.environ.get("DEFAULT_FIX_VERSION", "3.1.1-SNAPSHOT") def get_json(url): try: diff --git a/streams/quickstart/java/pom.xml b/streams/quickstart/java/pom.xml index 0f93c6ef2ad7e..c8bf39125c4cc 100644 --- a/streams/quickstart/java/pom.xml +++ b/streams/quickstart/java/pom.xml @@ -26,7 +26,7 @@ org.apache.kafka streams-quickstart - 3.1.0 + 3.1.1-SNAPSHOT .. diff --git a/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml b/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml index 07a0b675513f3..b7907321ec30d 100644 --- a/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml +++ b/streams/quickstart/java/src/main/resources/archetype-resources/pom.xml @@ -29,7 +29,7 @@ UTF-8 - 3.1.0 + 3.1.1-SNAPSHOT 1.7.7 1.2.17 diff --git a/streams/quickstart/pom.xml b/streams/quickstart/pom.xml index e063a39839b63..52158493bbae3 100644 --- a/streams/quickstart/pom.xml +++ b/streams/quickstart/pom.xml @@ -22,7 +22,7 @@ org.apache.kafka streams-quickstart pom - 3.1.0 + 3.1.1-SNAPSHOT Kafka Streams :: Quickstart diff --git a/tests/kafkatest/__init__.py b/tests/kafkatest/__init__.py index 7f331f93d441e..958155e85d238 100644 --- a/tests/kafkatest/__init__.py +++ b/tests/kafkatest/__init__.py @@ -22,4 +22,4 @@ # Instead, in development branches, the version should have a suffix of the form ".devN" # # For example, when Kafka is at version 1.0.0-SNAPSHOT, this should be something like "1.0.0.dev0" -__version__ = '3.1.0' +__version__ = '3.1.1.dev0' diff --git a/tests/kafkatest/version.py b/tests/kafkatest/version.py index d64fe87f2f17e..7f702b4df22f9 100644 --- a/tests/kafkatest/version.py +++ b/tests/kafkatest/version.py @@ -116,7 +116,7 @@ def get_version(node=None): return DEV_BRANCH DEV_BRANCH = KafkaVersion("dev") -DEV_VERSION = KafkaVersion("3.1.0-SNAPSHOT") +DEV_VERSION = KafkaVersion("3.1.1-SNAPSHOT") # 0.8.2.x versions V_0_8_2_1 = KafkaVersion("0.8.2.1") From 52e2d1cf498a9df5101303498a68ddba27e80e64 Mon Sep 17 00:00:00 2001 From: David Jacot Date: Thu, 20 Jan 2022 14:14:44 +0100 Subject: [PATCH 03/51] MINOR: Upgrade jetty-server to 9.4.44.v20210927 (#11692) Release notes: https://github.com/eclipse/jetty.project/releases/tag/jetty-9.4.44.v20210927 Reviewers: Rajini Sivaram --- gradle/dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 5bb9d61476e6f..fa3998b7b5993 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -69,7 +69,7 @@ versions += [ jackson: "2.12.3", jacoco: "0.8.7", javassist: "3.27.0-GA", - jetty: "9.4.43.v20210629", + jetty: "9.4.44.v20210927", jersey: "2.34", jline: "3.12.1", jmh: "1.32", From 630195458ccab46536d79838edbf26fc6deb3c91 Mon Sep 17 00:00:00 2001 From: David Jacot Date: Fri, 21 Jan 2022 17:38:40 +0100 Subject: [PATCH 04/51] KAFKA-13591; Fix flaky test `ControllerIntegrationTest.testTopicIdCreatedOnUpgrade` (#11666) The issue is that when `zkClient.getTopicIdsForTopics(Set(tp.topic)).get(tp.topic)` is called after the new controller is brought up, there is not guarantee that the controller has already written the topic id to the topic znode. Reviewers: Jason Gustafson --- .../ControllerIntegrationTest.scala | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala b/core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala index 2302007c5275e..300db0047b441 100644 --- a/core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala +++ b/core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala @@ -1127,27 +1127,26 @@ class ControllerIntegrationTest extends QuorumTestHarness { TestUtils.createTopic(zkClient, tp.topic, partitionReplicaAssignment = assignment, servers = servers) waitForPartitionState(tp, firstControllerEpoch, controllerId, LeaderAndIsr.initialLeaderEpoch, "failed to get expected partition state upon topic creation") - val topicIdAfterCreate = zkClient.getTopicIdsForTopics(Set(tp.topic())).get(tp.topic()) - assertEquals(None, topicIdAfterCreate) - val emptyTopicId = controller.controllerContext.topicIds.get("t") - assertEquals(None, emptyTopicId) + assertEquals(None, zkClient.getTopicIdsForTopics(Set(tp.topic)).get(tp.topic)) + assertEquals(None, controller.controllerContext.topicIds.get(tp.topic)) servers(controllerId).shutdown() servers(controllerId).awaitShutdown() servers = makeServers(1) TestUtils.waitUntilTrue(() => zkClient.getControllerId.isDefined, "failed to elect a controller") - val topicIdAfterUpgrade = zkClient.getTopicIdsForTopics(Set(tp.topic())).get(tp.topic()) - assertNotEquals(emptyTopicId, topicIdAfterUpgrade) + + val (topicIdAfterUpgrade, _) = TestUtils.computeUntilTrue(zkClient.getTopicIdsForTopics(Set(tp.topic)).get(tp.topic))(_.nonEmpty) + assertNotEquals(None, topicIdAfterUpgrade, s"topic id for ${tp.topic} not found in ZK") + val controller2 = getController().kafkaController - assertNotEquals(emptyTopicId, controller2.controllerContext.topicIds.get("t")) - val topicId = controller2.controllerContext.topicIds.get("t").get - assertEquals(topicIdAfterUpgrade.get, topicId) - assertEquals("t", controller2.controllerContext.topicNames(topicId)) + val topicId = controller2.controllerContext.topicIds.get(tp.topic) + assertEquals(topicIdAfterUpgrade, topicId) + assertEquals(tp.topic, controller2.controllerContext.topicNames(topicId.get)) TestUtils.waitUntilTrue(() => servers(0).logManager.getLog(tp).isDefined, "log was not created") val topicIdInLog = servers(0).logManager.getLog(tp).get.topicId - assertEquals(Some(topicId), topicIdInLog) + assertEquals(topicId, topicIdInLog) adminZkClient.deleteTopic(tp.topic) TestUtils.waitUntilTrue(() => !servers.head.kafkaController.controllerContext.allTopics.contains(tp.topic), From ca37f14076adbaa302a558a750be197c202f1038 Mon Sep 17 00:00:00 2001 From: David Jacot Date: Fri, 21 Jan 2022 17:44:56 +0100 Subject: [PATCH 05/51] KAFKA-13388; Kafka Producer nodes stuck in CHECKING_API_VERSIONS (#11671) At the moment, the `NetworkClient` will remain stuck in the `CHECKING_API_VERSIONS` state forever if the `Channel` does not become ready. To prevent this from happening, this patch changes the logic to transition to the `CHECKING_API_VERSIONS` only when the `ApiVersionsRequest` is queued to be sent out. With this, the connection will timeout if the `Channel` does not become ready within the connection setup timeout. Once the `ApiVersionsRequest` is queued up, the request timeout takes over. Reviewers: Rajini Sivaram --- .../apache/kafka/clients/NetworkClient.java | 6 +++++- .../kafka/clients/NetworkClientTest.java | 21 +++++++++++++++++++ .../org/apache/kafka/test/MockSelector.java | 14 ++++++++++--- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java b/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java index e7a0ac7c8d029..cabc3cccddece 100644 --- a/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java +++ b/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java @@ -947,7 +947,6 @@ private void handleConnections() { // Therefore, it is still necessary to check isChannelReady before attempting to send on this // connection. if (discoverBrokerVersions) { - this.connectionStates.checkingApiVersions(node); nodesNeedingApiVersionsFetch.put(node, new ApiVersionsRequest.Builder()); log.debug("Completed connection to node {}. Fetching API versions.", node); } else { @@ -964,6 +963,11 @@ private void handleInitiateApiVersionRequests(long now) { String node = entry.getKey(); if (selector.isChannelReady(node) && inFlightRequests.canSendMore(node)) { log.debug("Initiating API versions fetch from node {}.", node); + // We transition the connection to the CHECKING_API_VERSIONS state only when + // the ApiVersionsRequest is queued up to be sent out. Without this, the client + // could remain in the CHECKING_API_VERSIONS state forever if the channel does + // not before ready. + this.connectionStates.checkingApiVersions(node); ApiVersionsRequest.Builder apiVersionRequestBuilder = entry.getValue(); ClientRequest clientRequest = newClientRequest(node, apiVersionRequestBuilder, now, true); doSend(clientRequest, true, now); diff --git a/clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java b/clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java index 4fbfd4293409a..fe1e9d19202db 100644 --- a/clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java @@ -1097,6 +1097,27 @@ public void testCloseConnectingNode() { assertTrue(client.isReady(node0, time.milliseconds())); } + @Test + public void testConnectionDoesNotRemainStuckInCheckingApiVersionsStateIfChannelNeverBecomesReady() { + final Cluster cluster = TestUtils.clusterWith(1); + final Node node = cluster.nodeById(0); + + // Channel is ready by default so we mark it as not ready. + client.ready(node, time.milliseconds()); + selector.channelNotReady(node.idString()); + + // Channel should not be ready. + client.poll(0, time.milliseconds()); + assertFalse(NetworkClientUtils.isReady(client, node, time.milliseconds())); + + // Connection should time out if the channel does not become ready within + // the connection setup timeout. This ensures that the client does not remain + // stuck in the CHECKING_API_VERSIONS state. + time.sleep((long) (connectionSetupTimeoutMsTest * 1.2) + 1); + client.poll(0, time.milliseconds()); + assertTrue(client.connectionFailed(node)); + } + private RequestHeader parseHeader(ByteBuffer buffer) { buffer.getInt(); // skip size return RequestHeader.parse(buffer.slice()); diff --git a/clients/src/test/java/org/apache/kafka/test/MockSelector.java b/clients/src/test/java/org/apache/kafka/test/MockSelector.java index d1d79dced5e97..2d7c1517d82c7 100644 --- a/clients/src/test/java/org/apache/kafka/test/MockSelector.java +++ b/clients/src/test/java/org/apache/kafka/test/MockSelector.java @@ -29,9 +29,11 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Predicate; /** @@ -48,6 +50,7 @@ public class MockSelector implements Selectable { private final List connected = new ArrayList<>(); private final List delayedReceives = new ArrayList<>(); private final Predicate canConnect; + private final Set ready = new HashSet<>(); public MockSelector(Time time) { this(time, null); @@ -62,6 +65,7 @@ public MockSelector(Time time, Predicate canConnect) { public void connect(String id, InetSocketAddress address, int sendBufferSize, int receiveBufferSize) throws IOException { if (canConnect == null || canConnect.test(address)) { this.connected.add(id); + this.ready.add(id); } } @@ -91,8 +95,8 @@ public void close(String id) { /** * Since MockSelector.connect will always succeed and add the * connection id to the Set connected, we can only simulate - * that the connection is still pending by remove the connection - * id from the Set connected + * that the connection is still pending by removing the connection + * id from the Set connected. * * @param id connection id */ @@ -221,9 +225,13 @@ public void muteAll() { public void unmuteAll() { } + public void channelNotReady(String id) { + ready.remove(id); + } + @Override public boolean isChannelReady(String id) { - return true; + return ready.contains(id); } public void reset() { From cde0046d15a6f5f13e6700ab7e83ef38074ff072 Mon Sep 17 00:00:00 2001 From: Jason Gustafson Date: Wed, 19 Jan 2022 13:20:41 -0800 Subject: [PATCH 06/51] KAFKA-13412; Ensure initTransactions() safe for retry after timeout (#11452) If the user's `initTransactions` call times out, the user is expected to retry. However, the producer will continue retrying the `InitProducerId` request in the background. If it happens to return before the user retry of `initTransactions`, then the producer will raise an exception about an invalid state transition. The patch fixes the issue by tracking the pending state transition until the user has acknowledged the operation's result. In the case of `initTransactions`, even if the `InitProducerId` returns in the background and the state changes, we can still retry the `initTransactions` call to obtain the result. Reviewers: David Jacot --- .../kafka/clients/producer/KafkaProducer.java | 8 +- .../internals/TransactionManager.java | 133 ++++-- .../internals/TransactionalRequestResult.java | 34 +- .../common/utils/ProducerIdAndEpoch.java | 2 +- .../clients/producer/KafkaProducerTest.java | 44 +- .../producer/internals/SenderTest.java | 32 +- .../internals/TransactionManagerTest.java | 417 +++++++++--------- .../kafka/api/AuthorizerIntegrationTest.scala | 65 +-- .../kafka/api/TransactionsTest.scala | 4 +- 9 files changed, 421 insertions(+), 318 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java index dbb908d4cbf80..a7f9389219add 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java @@ -964,9 +964,10 @@ private Future doSend(ProducerRecord record, Callback call // producer callback will make sure to call both 'callback' and interceptor callback Callback interceptCallback = new InterceptorCallback<>(callback, this.interceptors, tp); - if (transactionManager != null && transactionManager.isTransactional()) { - transactionManager.failIfNotReadyForSend(); + if (transactionManager != null) { + transactionManager.maybeAddPartition(tp); } + RecordAccumulator.RecordAppendResult result = accumulator.append(tp, timestamp, serializedKey, serializedValue, headers, interceptCallback, remainingWaitMs, true, nowMs); @@ -985,9 +986,6 @@ private Future doSend(ProducerRecord record, Callback call serializedValue, headers, interceptCallback, remainingWaitMs, false, nowMs); } - if (transactionManager != null && transactionManager.isTransactional()) - transactionManager.maybeAddPartitionToTransaction(tp); - if (result.batchIsFull || result.newBatchCreated) { log.trace("Waking up the sender since topic {} partition {} is either full or getting a new batch", record.topic(), partition); this.sender.wakeup(); 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 2de31a03c5694..521d5da4f8ed6 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 @@ -225,7 +225,7 @@ void resetSequenceNumbers(Consumer resetSequence) { private final Set newPartitionsInTransaction; private final Set pendingPartitionsInTransaction; private final Set partitionsInTransaction; - private TransactionalRequestResult pendingResult; + private PendingStateTransition pendingTransition; // This is used by the TxnRequestHandlers to control how long to back off before a given request is retried. // For instance, this value is lowered by the AddPartitionsToTxnHandler when it receives a CONCURRENT_TRANSACTIONS @@ -329,6 +329,8 @@ public synchronized TransactionalRequestResult initializeTransactions() { } synchronized TransactionalRequestResult initializeTransactions(ProducerIdAndEpoch producerIdAndEpoch) { + maybeFailWithError(); + boolean isEpochBump = producerIdAndEpoch != ProducerIdAndEpoch.NONE; return handleCachedTransactionRequestResult(() -> { // If this is an epoch bump, we will transition the state as part of handling the EndTxnRequest @@ -347,11 +349,12 @@ synchronized TransactionalRequestResult initializeTransactions(ProducerIdAndEpoc isEpochBump); enqueueRequest(handler); return handler.result; - }, State.INITIALIZING); + }, State.INITIALIZING, "initTransactions"); } public synchronized void beginTransaction() { ensureTransactional(); + throwIfPendingState("beginTransaction"); maybeFailWithError(); transitionTo(State.IN_TRANSACTION); } @@ -361,7 +364,7 @@ public synchronized TransactionalRequestResult beginCommit() { maybeFailWithError(); transitionTo(State.COMMITTING_TRANSACTION); return beginCompletingTransaction(TransactionResult.COMMIT); - }, State.COMMITTING_TRANSACTION); + }, State.COMMITTING_TRANSACTION, "commitTransaction"); } public synchronized TransactionalRequestResult beginAbort() { @@ -373,7 +376,7 @@ public synchronized TransactionalRequestResult beginAbort() { // We're aborting the transaction, so there should be no need to add new partitions newPartitionsInTransaction.clear(); return beginCompletingTransaction(TransactionResult.ABORT); - }, State.ABORTING_TRANSACTION); + }, State.ABORTING_TRANSACTION, "abortTransaction"); } private TransactionalRequestResult beginCompletingTransaction(TransactionResult transactionResult) { @@ -404,10 +407,13 @@ private TransactionalRequestResult beginCompletingTransaction(TransactionResult public synchronized TransactionalRequestResult sendOffsetsToTransaction(final Map offsets, final ConsumerGroupMetadata groupMetadata) { ensureTransactional(); + throwIfPendingState("sendOffsetsToTransaction"); maybeFailWithError(); - if (currentState != State.IN_TRANSACTION) - throw new KafkaException("Cannot send offsets to transaction either because the producer is not in an " + - "active transaction"); + + if (currentState != State.IN_TRANSACTION) { + throw new IllegalStateException("Cannot send offsets if a transaction is not in progress " + + "(currentState= " + currentState + ")"); + } log.debug("Begin adding offsets {} for consumer group {} to transaction", offsets, groupMetadata); AddOffsetsToTxnRequest.Builder builder = new AddOffsetsToTxnRequest.Builder( @@ -423,34 +429,31 @@ public synchronized TransactionalRequestResult sendOffsetsToTransaction(final Ma return handler.result; } - public synchronized void maybeAddPartitionToTransaction(TopicPartition topicPartition) { - if (isPartitionAdded(topicPartition) || isPartitionPendingAdd(topicPartition)) - return; + public synchronized void maybeAddPartition(TopicPartition topicPartition) { + maybeFailWithError(); + throwIfPendingState("send"); - log.debug("Begin adding new partition {} to transaction", topicPartition); - topicPartitionBookkeeper.addPartition(topicPartition); - newPartitionsInTransaction.add(topicPartition); + if (isTransactional()) { + if (!hasProducerId()) { + throw new IllegalStateException("Cannot add partition " + topicPartition + + " to transaction before completing a call to initTransactions"); + } else if (currentState != State.IN_TRANSACTION) { + throw new IllegalStateException("Cannot add partition " + topicPartition + + " to transaction while in state " + currentState); + } else if (isPartitionAdded(topicPartition) || isPartitionPendingAdd(topicPartition)) { + return; + } else { + log.debug("Begin adding new partition {} to transaction", topicPartition); + topicPartitionBookkeeper.addPartition(topicPartition); + newPartitionsInTransaction.add(topicPartition); + } + } } RuntimeException lastError() { return lastError; } - public synchronized void failIfNotReadyForSend() { - if (hasError()) - throw new KafkaException("Cannot perform send because at least one previous transactional or " + - "idempotent request has failed with errors.", lastError); - - if (isTransactional()) { - if (!hasProducerId()) - throw new IllegalStateException("Cannot perform a 'send' before completing a call to initTransactions " + - "when transactions are enabled."); - - if (currentState != State.IN_TRANSACTION) - throw new IllegalStateException("Cannot call send in state " + currentState); - } - } - synchronized boolean isSendToPartitionAllowed(TopicPartition tp) { if (hasFatalError()) return false; @@ -500,8 +503,8 @@ synchronized void transitionToFatalError(RuntimeException exception) { log.info("Transiting to fatal error state due to {}", exception.toString()); transitionTo(State.FATAL_ERROR, exception); - if (pendingResult != null) { - pendingResult.fail(exception); + if (pendingTransition != null) { + pendingTransition.result.fail(exception); } } @@ -919,8 +922,8 @@ synchronized void close() { KafkaException shutdownException = new KafkaException("The producer closed forcefully"); pendingRequests.forEach(handler -> handler.fatalError(shutdownException)); - if (pendingResult != null) { - pendingResult.fail(shutdownException); + if (pendingTransition != null) { + pendingTransition.result.fail(shutdownException); } } @@ -1073,7 +1076,7 @@ private void transitionTo(State target) { private void transitionTo(State target, RuntimeException error) { if (!currentState.isTransitionValid(currentState, target)) { String idString = transactionalId == null ? "" : "TransactionalId " + transactionalId + ": "; - throw new KafkaException(idString + "Invalid transition attempted from state " + throw new IllegalStateException(idString + "Invalid transition attempted from state " + currentState.name() + " to state " + target.name()); } @@ -1103,10 +1106,12 @@ private void maybeFailWithError() { // for ProducerFencedException, do not wrap it as a KafkaException // but create a new instance without the call trace since it was not thrown because of the current call if (lastError instanceof ProducerFencedException) { - throw new ProducerFencedException("The producer has been rejected from the broker because " + - "it tried to use an old epoch with the transactionalId"); + throw new ProducerFencedException("Producer with transactionalId '" + transactionalId + + "' and " + producerIdAndEpoch + " has been fenced by another producer " + + "with the same transactionalId"); } else if (lastError instanceof InvalidProducerEpochException) { - throw new InvalidProducerEpochException("Producer attempted to produce with an old epoch " + producerIdAndEpoch); + throw new InvalidProducerEpochException("Producer with transactionalId '" + transactionalId + + "' and " + producerIdAndEpoch + " attempted to produce with an old epoch"); } else { throw new KafkaException("Cannot execute transactional method because we are in an error state", lastError); } @@ -1183,20 +1188,40 @@ private TxnOffsetCommitHandler txnOffsetCommitHandler(TransactionalRequestResult return new TxnOffsetCommitHandler(result, builder); } + private void throwIfPendingState(String operation) { + if (pendingTransition != null) { + if (pendingTransition.result.isAcked()) { + pendingTransition = null; + } else { + throw new IllegalStateException("Cannot attempt operation `" + operation + "` " + + "because the previous call to `" + pendingTransition.operation + "` " + + "timed out and must be retried"); + } + } + } + private TransactionalRequestResult handleCachedTransactionRequestResult( - Supplier transactionalRequestResultSupplier, - State targetState) { + Supplier transactionalRequestResultSupplier, + State nextState, + String operation + ) { ensureTransactional(); - if (pendingResult != null && currentState == targetState) { - TransactionalRequestResult result = pendingResult; - if (result.isCompleted()) - pendingResult = null; - return result; + if (pendingTransition != null) { + if (pendingTransition.result.isAcked()) { + pendingTransition = null; + } else if (nextState != pendingTransition.state) { + throw new IllegalStateException("Cannot attempt operation `" + operation + "` " + + "because the previous call to `" + pendingTransition.operation + "` " + + "timed out and must be retried"); + } else { + return pendingTransition.result; + } } - pendingResult = transactionalRequestResultSupplier.get(); - return pendingResult; + TransactionalRequestResult result = transactionalRequestResultSupplier.get(); + pendingTransition = new PendingStateTransition(result, nextState, operation); + return result; } // package-private for testing @@ -1762,4 +1787,22 @@ private boolean isFatalException(Errors error) { || error == Errors.PRODUCER_FENCED || error == Errors.UNSUPPORTED_FOR_MESSAGE_FORMAT; } + + private static final class PendingStateTransition { + private final TransactionalRequestResult result; + private final State state; + private final String operation; + + private PendingStateTransition( + TransactionalRequestResult result, + State state, + String operation + ) { + this.result = result; + this.state = state; + this.operation = operation; + } + } + + } diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionalRequestResult.java b/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionalRequestResult.java index d442b188d4520..6739da815273e 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionalRequestResult.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionalRequestResult.java @@ -20,15 +20,14 @@ import org.apache.kafka.common.errors.InterruptException; import org.apache.kafka.common.errors.TimeoutException; -import java.util.Locale; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; public final class TransactionalRequestResult { - private final CountDownLatch latch; private volatile RuntimeException error = null; private final String operation; + private volatile boolean isAcked = false; public TransactionalRequestResult(String operation) { this(new CountDownLatch(1), operation); @@ -49,29 +48,20 @@ public void done() { } public void await() { - boolean completed = false; - - while (!completed) { - try { - latch.await(); - completed = true; - } catch (InterruptedException e) { - // Keep waiting until done, we have no other option for these transactional requests. - } - } - - if (!isSuccessful()) - throw error(); + this.await(Long.MAX_VALUE, TimeUnit.MILLISECONDS); } public void await(long timeout, TimeUnit unit) { try { boolean success = latch.await(timeout, unit); - if (!isSuccessful()) { - throw error(); - } if (!success) { - throw new TimeoutException("Timeout expired after " + timeout + " " + unit.name().toLowerCase(Locale.ROOT) + " while awaiting " + operation); + throw new TimeoutException("Timeout expired after " + unit.toMillis(timeout) + + "ms while awaiting " + operation); + } + + isAcked = true; + if (error != null) { + throw error; } } catch (InterruptedException e) { throw new InterruptException("Received interrupt while awaiting " + operation, e); @@ -83,11 +73,15 @@ public RuntimeException error() { } public boolean isSuccessful() { - return error == null; + return isCompleted() && error == null; } public boolean isCompleted() { return latch.getCount() == 0L; } + public boolean isAcked() { + return isAcked; + } + } diff --git a/clients/src/main/java/org/apache/kafka/common/utils/ProducerIdAndEpoch.java b/clients/src/main/java/org/apache/kafka/common/utils/ProducerIdAndEpoch.java index 674b42352b787..5061da1343cdc 100644 --- a/clients/src/main/java/org/apache/kafka/common/utils/ProducerIdAndEpoch.java +++ b/clients/src/main/java/org/apache/kafka/common/utils/ProducerIdAndEpoch.java @@ -35,7 +35,7 @@ public boolean isValid() { @Override public String toString() { - return "(producerId=" + producerId + ", epoch=" + epoch + ")"; + return "ProducerIdAndEpoch(producerId=" + producerId + ", epoch=" + epoch + ")"; } @Override diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java index 1e45a58b98efe..96a30341b842f 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java @@ -903,6 +903,48 @@ public void testPartitionsForWithNullTopic() { } } + @Test + public void testInitTransactionsResponseAfterTimeout() throws Exception { + int maxBlockMs = 500; + + Map configs = new HashMap<>(); + configs.put(ProducerConfig.TRANSACTIONAL_ID_CONFIG, "bad-transaction"); + configs.put(ProducerConfig.MAX_BLOCK_MS_CONFIG, maxBlockMs); + configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9000"); + + Time time = new MockTime(); + MetadataResponse initialUpdateResponse = RequestTestUtils.metadataUpdateWith(1, singletonMap("topic", 1)); + ProducerMetadata metadata = newMetadata(0, Long.MAX_VALUE); + metadata.updateWithCurrentRequestVersion(initialUpdateResponse, false, time.milliseconds()); + + MockClient client = new MockClient(time, metadata); + + ExecutorService executor = Executors.newFixedThreadPool(1); + + Producer producer = kafkaProducer(configs, new StringSerializer(), + new StringSerializer(), metadata, client, null, time); + try { + client.prepareResponse( + request -> request instanceof FindCoordinatorRequest && + ((FindCoordinatorRequest) request).data().keyType() == FindCoordinatorRequest.CoordinatorType.TRANSACTION.id(), + FindCoordinatorResponse.prepareResponse(Errors.NONE, "bad-transaction", host1)); + + Future future = executor.submit(producer::initTransactions); + TestUtils.waitForCondition(client::hasInFlightRequests, + "Timed out while waiting for expected `InitProducerId` request to be sent"); + + time.sleep(maxBlockMs); + TestUtils.assertFutureThrows(future, TimeoutException.class); + + client.respond(initProducerIdResponse(1L, (short) 5, Errors.NONE)); + + Thread.sleep(1000); + producer.initTransactions(); + } finally { + producer.close(Duration.ZERO); + } + } + @Test public void testInitTransactionTimeout() { Map configs = new HashMap<>(); @@ -1249,7 +1291,7 @@ public void testOnlyCanExecuteCloseAfterInitTransactionsTimeout() { assertThrows(TimeoutException.class, producer::initTransactions); // other transactional operations should not be allowed if we catch the error after initTransactions failed try { - assertThrows(KafkaException.class, producer::beginTransaction); + assertThrows(IllegalStateException.class, producer::beginTransaction); } finally { producer.close(Duration.ofMillis(0)); } diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java index 34e4f180e7866..60e9f06186255 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java @@ -1465,8 +1465,7 @@ public void testUnresolvedSequencesAreNotFatal() throws Exception { doInitTransactions(txnManager, producerIdAndEpoch); txnManager.beginTransaction(); - txnManager.failIfNotReadyForSend(); - txnManager.maybeAddPartitionToTransaction(tp0); + txnManager.maybeAddPartition(tp0); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp0, Errors.NONE))); sender.runOnce(); @@ -1751,7 +1750,8 @@ public void testTransactionalUnknownProducerHandlingWhenRetentionLimitReached() doInitTransactions(transactionManager, new ProducerIdAndEpoch(producerId, (short) 0)); assertTrue(transactionManager.hasProducerId()); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.beginTransaction(); + transactionManager.maybeAddPartition(tp0); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp0, Errors.NONE))); sender.runOnce(); // Receive AddPartitions response @@ -2307,8 +2307,7 @@ public void testTransactionalSplitBatchAndSend() throws Exception { doInitTransactions(txnManager, producerIdAndEpoch); txnManager.beginTransaction(); - txnManager.failIfNotReadyForSend(); - txnManager.maybeAddPartitionToTransaction(tp); + txnManager.maybeAddPartition(tp); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp, Errors.NONE))); sender.runOnce(); @@ -2654,8 +2653,7 @@ public void testTransactionalRequestsSentOnShutdown() { doInitTransactions(txnManager, producerIdAndEpoch); txnManager.beginTransaction(); - txnManager.failIfNotReadyForSend(); - txnManager.maybeAddPartitionToTransaction(tp); + txnManager.maybeAddPartition(tp); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp, Errors.NONE))); sender.runOnce(); sender.initiateClose(); @@ -2697,7 +2695,7 @@ public void testRecordsFlushedImmediatelyOnTransactionCompletion() throws Except // Now begin the commit and assert that the Produce request is sent immediately // without waiting for the linger. - txnManager.beginCommit(); + TransactionalRequestResult commitResult = txnManager.beginCommit(); runUntil(sender, client::hasInFlightRequests); // Respond to the produce request and wait for the EndTxn request to be sent. @@ -2708,6 +2706,9 @@ public void testRecordsFlushedImmediatelyOnTransactionCompletion() throws Except respondToEndTxn(Errors.NONE); runUntil(sender, txnManager::isReady); + assertTrue(commitResult.isSuccessful()); + commitResult.await(); + // Finally, we want to assert that the linger time is still effective // when the new transaction begins. txnManager.beginTransaction(); @@ -2772,7 +2773,7 @@ public void testAwaitPendingRecordsBeforeCommittingTransaction() throws Exceptio } private void addPartitionToTxn(Sender sender, TransactionManager txnManager, TopicPartition tp) { - txnManager.maybeAddPartitionToTransaction(tp); + txnManager.maybeAddPartition(tp); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp, Errors.NONE))); runUntil(sender, () -> txnManager.isPartitionAdded(tp)); assertFalse(txnManager.hasInFlightRequest()); @@ -2813,8 +2814,7 @@ public void testIncompleteTransactionAbortOnShutdown() { doInitTransactions(txnManager, producerIdAndEpoch); txnManager.beginTransaction(); - txnManager.failIfNotReadyForSend(); - txnManager.maybeAddPartitionToTransaction(tp); + txnManager.maybeAddPartition(tp); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp, Errors.NONE))); sender.runOnce(); sender.initiateClose(); @@ -2848,8 +2848,7 @@ public void testForceShutdownWithIncompleteTransaction() { doInitTransactions(txnManager, producerIdAndEpoch); txnManager.beginTransaction(); - txnManager.failIfNotReadyForSend(); - txnManager.maybeAddPartitionToTransaction(tp); + txnManager.maybeAddPartition(tp); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp, Errors.NONE))); sender.runOnce(); @@ -2874,7 +2873,7 @@ public void testTransactionAbortedExceptionOnAbortWithoutError() throws Interrup doInitTransactions(txnManager, producerIdAndEpoch); // Begin the transaction txnManager.beginTransaction(); - txnManager.maybeAddPartitionToTransaction(tp0); + txnManager.maybeAddPartition(tp0); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp0, Errors.NONE))); // Run it once so that the partition is added to the transaction. sender.runOnce(); @@ -2912,7 +2911,7 @@ public void testTooLargeBatchesAreSafelyRemoved() throws InterruptedException { doInitTransactions(txnManager, producerIdAndEpoch); txnManager.beginTransaction(); - txnManager.maybeAddPartitionToTransaction(tp0); + txnManager.maybeAddPartition(tp0); client.prepareResponse(new AddPartitionsToTxnResponse(0, Collections.singletonMap(tp0, Errors.NONE))); sender.runOnce(); @@ -3191,7 +3190,7 @@ private InitProducerIdResponse initProducerIdResponse(long producerId, short pro } private void doInitTransactions(TransactionManager transactionManager, ProducerIdAndEpoch producerIdAndEpoch) { - transactionManager.initializeTransactions(); + TransactionalRequestResult result = transactionManager.initializeTransactions(); prepareFindCoordinatorResponse(Errors.NONE, transactionManager.transactionalId()); sender.runOnce(); sender.runOnce(); @@ -3199,6 +3198,7 @@ private void doInitTransactions(TransactionManager transactionManager, ProducerI prepareInitProducerResponse(Errors.NONE, producerIdAndEpoch.producerId, producerIdAndEpoch.epoch); sender.runOnce(); assertTrue(transactionManager.hasProducerId()); + result.await(); } private void prepareFindCoordinatorResponse(Errors error, String txnid) { 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 6c1e2fdf1e785..4227db5e61e62 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 @@ -103,6 +103,7 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -184,8 +185,7 @@ public void testSenderShutdownWithPendingTransactions() throws Exception { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); FutureRecordMetadata sendFuture = appendToAccumulator(tp0); prepareAddPartitionsToTxn(tp0, Errors.NONE); @@ -206,8 +206,7 @@ public void testEndTxnNotSentIfIncompleteBatches() { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxn(tp0, Errors.NONE); runUntil(() -> transactionManager.isPartitionAdded(tp0)); @@ -218,26 +217,26 @@ public void testEndTxnNotSentIfIncompleteBatches() { @Test public void testFailIfNotReadyForSendNoProducerId() { - assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend()); + assertThrows(IllegalStateException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test public void testFailIfNotReadyForSendIdempotentProducer() { initializeTransactionManager(Optional.empty()); - transactionManager.failIfNotReadyForSend(); + transactionManager.maybeAddPartition(tp0); } @Test public void testFailIfNotReadyForSendIdempotentProducerFatalError() { initializeTransactionManager(Optional.empty()); transactionManager.transitionToFatalError(new KafkaException()); - assertThrows(KafkaException.class, () -> transactionManager.failIfNotReadyForSend()); + assertThrows(KafkaException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test public void testFailIfNotReadyForSendNoOngoingTransaction() { doInitTransactions(); - assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend()); + assertThrows(IllegalStateException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test @@ -245,14 +244,14 @@ public void testFailIfNotReadyForSendAfterAbortableError() { doInitTransactions(); transactionManager.beginTransaction(); transactionManager.transitionToAbortableError(new KafkaException()); - assertThrows(KafkaException.class, transactionManager::failIfNotReadyForSend); + assertThrows(KafkaException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test public void testFailIfNotReadyForSendAfterFatalError() { doInitTransactions(); transactionManager.transitionToFatalError(new KafkaException()); - assertThrows(KafkaException.class, transactionManager::failIfNotReadyForSend); + assertThrows(KafkaException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test @@ -266,8 +265,7 @@ public void testHasOngoingTransactionSuccessfulAbort() { transactionManager.beginTransaction(); assertTrue(transactionManager.hasOngoingTransaction()); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); runUntil(transactionManager::hasOngoingTransaction); prepareAddPartitionsToTxn(partition, Errors.NONE); @@ -291,8 +289,7 @@ public void testHasOngoingTransactionSuccessfulCommit() { transactionManager.beginTransaction(); assertTrue(transactionManager.hasOngoingTransaction()); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); assertTrue(transactionManager.hasOngoingTransaction()); prepareAddPartitionsToTxn(partition, Errors.NONE); @@ -316,8 +313,7 @@ public void testHasOngoingTransactionAbortableError() { transactionManager.beginTransaction(); assertTrue(transactionManager.hasOngoingTransaction()); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); assertTrue(transactionManager.hasOngoingTransaction()); prepareAddPartitionsToTxn(partition, Errors.NONE); @@ -344,8 +340,7 @@ public void testHasOngoingTransactionFatalError() { transactionManager.beginTransaction(); assertTrue(transactionManager.hasOngoingTransaction()); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); assertTrue(transactionManager.hasOngoingTransaction()); prepareAddPartitionsToTxn(partition, Errors.NONE); @@ -361,8 +356,7 @@ public void testMaybeAddPartitionToTransaction() { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); assertTrue(transactionManager.hasPartitionsToAdd()); assertFalse(transactionManager.isPartitionAdded(partition)); assertTrue(transactionManager.isPartitionPendingAdd(partition)); @@ -375,8 +369,7 @@ public void testMaybeAddPartitionToTransaction() { assertFalse(transactionManager.isPartitionPendingAdd(partition)); // adding the partition again should not have any effect - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); assertFalse(transactionManager.hasPartitionsToAdd()); assertTrue(transactionManager.isPartitionAdded(partition)); assertFalse(transactionManager.isPartitionPendingAdd(partition)); @@ -388,8 +381,7 @@ public void testAddPartitionToTransactionOverridesRetryBackoffForConcurrentTrans doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); assertTrue(transactionManager.hasPartitionsToAdd()); assertFalse(transactionManager.isPartitionAdded(partition)); assertTrue(transactionManager.isPartitionPendingAdd(partition)); @@ -408,8 +400,7 @@ public void testAddPartitionToTransactionRetainsRetryBackoffForRegularRetriableE doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); assertTrue(transactionManager.hasPartitionsToAdd()); assertFalse(transactionManager.isPartitionAdded(partition)); assertTrue(transactionManager.isPartitionPendingAdd(partition)); @@ -428,8 +419,7 @@ public void testAddPartitionToTransactionRetainsRetryBackoffWhenPartitionsAlread doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(partition); + transactionManager.maybeAddPartition(partition); assertTrue(transactionManager.hasPartitionsToAdd()); assertFalse(transactionManager.isPartitionAdded(partition)); assertTrue(transactionManager.isPartitionPendingAdd(partition)); @@ -438,8 +428,7 @@ public void testAddPartitionToTransactionRetainsRetryBackoffWhenPartitionsAlread runUntil(() -> transactionManager.isPartitionAdded(partition)); TopicPartition otherPartition = new TopicPartition("foo", 1); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(otherPartition); + transactionManager.maybeAddPartition(otherPartition); prepareAddPartitionsToTxn(otherPartition, Errors.CONCURRENT_TRANSACTIONS); TransactionManager.TxnRequestHandler handler = transactionManager.nextRequest(false); @@ -449,13 +438,13 @@ public void testAddPartitionToTransactionRetainsRetryBackoffWhenPartitionsAlread @Test public void testNotReadyForSendBeforeInitTransactions() { - assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend()); + assertThrows(IllegalStateException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test public void testNotReadyForSendBeforeBeginTransaction() { doInitTransactions(); - assertThrows(IllegalStateException.class, () -> transactionManager.failIfNotReadyForSend()); + assertThrows(IllegalStateException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test @@ -463,14 +452,14 @@ public void testNotReadyForSendAfterAbortableError() { doInitTransactions(); transactionManager.beginTransaction(); transactionManager.transitionToAbortableError(new KafkaException()); - assertThrows(KafkaException.class, () -> transactionManager.failIfNotReadyForSend()); + assertThrows(KafkaException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test public void testNotReadyForSendAfterFatalError() { doInitTransactions(); transactionManager.transitionToFatalError(new KafkaException()); - assertThrows(KafkaException.class, () -> transactionManager.failIfNotReadyForSend()); + assertThrows(KafkaException.class, () -> transactionManager.maybeAddPartition(tp0)); } @Test @@ -478,8 +467,7 @@ public void testIsSendToPartitionAllowedWithPendingPartitionAfterAbortableError( doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); transactionManager.transitionToAbortableError(new KafkaException()); assertFalse(transactionManager.isSendToPartitionAllowed(tp0)); @@ -491,8 +479,7 @@ public void testIsSendToPartitionAllowedWithInFlightPartitionAddAfterAbortableEr doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); // Send the AddPartitionsToTxn request and leave it in-flight runUntil(transactionManager::hasInFlightRequest); @@ -507,8 +494,7 @@ public void testIsSendToPartitionAllowedWithPendingPartitionAfterFatalError() { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); transactionManager.transitionToFatalError(new KafkaException()); assertFalse(transactionManager.isSendToPartitionAllowed(tp0)); @@ -520,8 +506,7 @@ public void testIsSendToPartitionAllowedWithInFlightPartitionAddAfterFatalError( doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); // Send the AddPartitionsToTxn request and leave it in-flight runUntil(transactionManager::hasInFlightRequest); @@ -537,8 +522,7 @@ public void testIsSendToPartitionAllowedWithAddedPartitionAfterAbortableError() transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, epoch, producerId); runUntil(() -> !transactionManager.hasPartitionsToAdd()); @@ -553,8 +537,7 @@ public void testIsSendToPartitionAllowedWithAddedPartitionAfterFatalError() { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, epoch, producerId); runUntil(() -> !transactionManager.hasPartitionsToAdd()); @@ -747,8 +730,7 @@ public void testBasicTransaction() throws InterruptedException { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -813,7 +795,7 @@ public void testDisconnectAndRetry() { public void testInitializeTransactionsTwiceRaisesError() { doInitTransactions(producerId, epoch); assertTrue(transactionManager.hasProducerId()); - assertThrows(KafkaException.class, () -> transactionManager.initializeTransactions()); + assertThrows(IllegalStateException.class, () -> transactionManager.initializeTransactions()); } @Test @@ -1075,7 +1057,7 @@ public void testTransactionalIdAuthorizationFailureInFindCoordinator() { assertTrue(transactionManager.hasFatalError()); assertTrue(transactionManager.lastError() instanceof TransactionalIdAuthorizationException); assertFalse(initPidResult.isSuccessful()); - assertTrue(initPidResult.error() instanceof TransactionalIdAuthorizationException); + assertThrows(TransactionalIdAuthorizationException.class, initPidResult::await); assertFatalError(TransactionalIdAuthorizationException.class); } @@ -1090,8 +1072,7 @@ public void testTransactionalIdAuthorizationFailureInInitProducerId() { runUntil(transactionManager::hasError); assertTrue(initPidResult.isCompleted()); assertFalse(initPidResult.isSuccessful()); - assertTrue(initPidResult.error() instanceof TransactionalIdAuthorizationException); - + assertThrows(TransactionalIdAuthorizationException.class, initPidResult::await); assertFatalError(TransactionalIdAuthorizationException.class); } @@ -1202,10 +1183,8 @@ public void testTopicAuthorizationFailureInAddPartitions() throws InterruptedExc doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp1); + transactionManager.maybeAddPartition(tp0); + transactionManager.maybeAddPartition(tp1); FutureRecordMetadata firstPartitionAppend = appendToAccumulator(tp0); FutureRecordMetadata secondPartitionAppend = appendToAccumulator(tp1); @@ -1242,8 +1221,8 @@ public void testCommitWithTopicAuthorizationFailureInAddPartitionsInFlight() thr // Begin a transaction, send two records, and begin commit transactionManager.beginTransaction(); - transactionManager.maybeAddPartitionToTransaction(tp0); - transactionManager.maybeAddPartitionToTransaction(tp1); + transactionManager.maybeAddPartition(tp0); + transactionManager.maybeAddPartition(tp1); FutureRecordMetadata firstPartitionAppend = appendToAccumulator(tp0); FutureRecordMetadata secondPartitionAppend = appendToAccumulator(tp1); TransactionalRequestResult commitResult = transactionManager.beginCommit(); @@ -1287,8 +1266,7 @@ public void testRecoveryFromAbortableErrorTransactionNotStarted() throws Excepti doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(unauthorizedPartition); + transactionManager.maybeAddPartition(unauthorizedPartition); Future responseFuture = appendToAccumulator(unauthorizedPartition); @@ -1296,7 +1274,7 @@ public void testRecoveryFromAbortableErrorTransactionNotStarted() throws Excepti runUntil(() -> !client.hasPendingResponses()); assertTrue(transactionManager.hasAbortableError()); - transactionManager.beginAbort(); + TransactionalRequestResult abortResult = transactionManager.beginAbort(); runUntil(responseFuture::isDone); assertProduceFutureFailed(responseFuture); @@ -1304,12 +1282,13 @@ public void testRecoveryFromAbortableErrorTransactionNotStarted() throws Excepti runUntil(transactionManager::isReady); assertFalse(transactionManager.hasPartitionsToAdd()); assertFalse(accumulator.hasIncomplete()); + assertTrue(abortResult.isSuccessful()); + abortResult.await(); // ensure we can now start a new transaction transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); responseFuture = appendToAccumulator(tp0); @@ -1326,6 +1305,104 @@ public void testRecoveryFromAbortableErrorTransactionNotStarted() throws Excepti runUntil(transactionManager::isReady); } + @Test + public void testRetryAbortTransactionAfterTimeout() throws Exception { + doInitTransactions(); + + transactionManager.beginTransaction(); + transactionManager.maybeAddPartition(tp0); + + prepareAddPartitionsToTxn(tp0, Errors.NONE); + appendToAccumulator(tp0); + runUntil(() -> transactionManager.isPartitionAdded(tp0)); + + TransactionalRequestResult result = transactionManager.beginAbort(); + assertThrows(TimeoutException.class, () -> result.await(0, TimeUnit.MILLISECONDS)); + + prepareEndTxnResponse(Errors.NONE, TransactionResult.ABORT, producerId, epoch); + runUntil(transactionManager::isReady); + assertTrue(result.isSuccessful()); + assertFalse(result.isAcked()); + assertFalse(transactionManager.hasOngoingTransaction()); + + assertThrows(IllegalStateException.class, transactionManager::initializeTransactions); + assertThrows(IllegalStateException.class, transactionManager::beginTransaction); + assertThrows(IllegalStateException.class, transactionManager::beginCommit); + assertThrows(IllegalStateException.class, () -> transactionManager.maybeAddPartition(tp0)); + + assertSame(result, transactionManager.beginAbort()); + result.await(); + + transactionManager.beginTransaction(); + assertTrue(transactionManager.hasOngoingTransaction()); + } + + @Test + public void testRetryCommitTransactionAfterTimeout() throws Exception { + doInitTransactions(); + + transactionManager.beginTransaction(); + transactionManager.maybeAddPartition(tp0); + + prepareAddPartitionsToTxn(tp0, Errors.NONE); + prepareProduceResponse(Errors.NONE, producerId, epoch); + + appendToAccumulator(tp0); + runUntil(() -> transactionManager.isPartitionAdded(tp0)); + + TransactionalRequestResult result = transactionManager.beginCommit(); + assertThrows(TimeoutException.class, () -> result.await(0, TimeUnit.MILLISECONDS)); + + prepareEndTxnResponse(Errors.NONE, TransactionResult.COMMIT, producerId, epoch); + runUntil(transactionManager::isReady); + assertTrue(result.isSuccessful()); + assertFalse(result.isAcked()); + assertFalse(transactionManager.hasOngoingTransaction()); + + assertThrows(IllegalStateException.class, transactionManager::initializeTransactions); + assertThrows(IllegalStateException.class, transactionManager::beginTransaction); + assertThrows(IllegalStateException.class, transactionManager::beginAbort); + assertThrows(IllegalStateException.class, () -> transactionManager.maybeAddPartition(tp0)); + + assertSame(result, transactionManager.beginCommit()); + result.await(); + + transactionManager.beginTransaction(); + assertTrue(transactionManager.hasOngoingTransaction()); + } + + @Test + public void testRetryInitTransactionsAfterTimeout() { + TransactionalRequestResult result = transactionManager.initializeTransactions(); + prepareFindCoordinatorResponse(Errors.NONE, false, CoordinatorType.TRANSACTION, transactionalId); + runUntil(() -> transactionManager.coordinator(CoordinatorType.TRANSACTION) != null); + assertEquals(brokerNode, transactionManager.coordinator(CoordinatorType.TRANSACTION)); + + assertThrows(TimeoutException.class, () -> result.await(0, TimeUnit.MILLISECONDS)); + + prepareInitPidResponse(Errors.NONE, false, producerId, epoch); + runUntil(transactionManager::hasProducerId); + assertTrue(result.isSuccessful()); + assertFalse(result.isAcked()); + + // At this point, the InitProducerId call has returned, but the user has yet + // to complete the call to `initTransactions`. Other transitions should be + // rejected until they do. + + assertThrows(IllegalStateException.class, transactionManager::beginTransaction); + assertThrows(IllegalStateException.class, transactionManager::beginAbort); + assertThrows(IllegalStateException.class, transactionManager::beginCommit); + assertThrows(IllegalStateException.class, () -> transactionManager.maybeAddPartition(tp0)); + + assertSame(result, transactionManager.initializeTransactions()); + result.await(); + assertTrue(result.isAcked()); + assertThrows(IllegalStateException.class, transactionManager::initializeTransactions); + + transactionManager.beginTransaction(); + assertTrue(transactionManager.hasOngoingTransaction()); + } + @Test public void testRecoveryFromAbortableErrorTransactionStarted() throws Exception { final TopicPartition unauthorizedPartition = new TopicPartition("foo", 0); @@ -1333,15 +1410,13 @@ public void testRecoveryFromAbortableErrorTransactionStarted() throws Exception doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxn(tp0, Errors.NONE); Future authorizedTopicProduceFuture = appendToAccumulator(unauthorizedPartition); runUntil(() -> transactionManager.isPartitionAdded(tp0)); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(unauthorizedPartition); + transactionManager.maybeAddPartition(unauthorizedPartition); Future unauthorizedTopicProduceFuture = appendToAccumulator(unauthorizedPartition); prepareAddPartitionsToTxn(singletonMap(unauthorizedPartition, Errors.TOPIC_AUTHORIZATION_FAILED)); runUntil(transactionManager::hasAbortableError); @@ -1351,19 +1426,20 @@ public void testRecoveryFromAbortableErrorTransactionStarted() throws Exception assertFalse(unauthorizedTopicProduceFuture.isDone()); prepareEndTxnResponse(Errors.NONE, TransactionResult.ABORT, producerId, epoch); - transactionManager.beginAbort(); + TransactionalRequestResult result = transactionManager.beginAbort(); runUntil(transactionManager::isReady); // neither produce request has been sent, so they should both be failed immediately assertProduceFutureFailed(authorizedTopicProduceFuture); assertProduceFutureFailed(unauthorizedTopicProduceFuture); assertFalse(transactionManager.hasPartitionsToAdd()); assertFalse(accumulator.hasIncomplete()); + assertTrue(result.isSuccessful()); + result.await(); // ensure we can now start a new transaction transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); FutureRecordMetadata nextTransactionFuture = appendToAccumulator(tp0); @@ -1387,8 +1463,7 @@ public void testRecoveryFromAbortableErrorProduceRequestInRetry() throws Excepti doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxn(tp0, Errors.NONE); Future authorizedTopicProduceFuture = appendToAccumulator(tp0); @@ -1400,8 +1475,7 @@ public void testRecoveryFromAbortableErrorProduceRequestInRetry() throws Excepti assertFalse(authorizedTopicProduceFuture.isDone()); assertTrue(accumulator.hasIncomplete()); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(unauthorizedPartition); + transactionManager.maybeAddPartition(unauthorizedPartition); Future unauthorizedTopicProduceFuture = appendToAccumulator(unauthorizedPartition); prepareAddPartitionsToTxn(singletonMap(unauthorizedPartition, Errors.TOPIC_AUTHORIZATION_FAILED)); runUntil(transactionManager::hasAbortableError); @@ -1417,18 +1491,19 @@ public void testRecoveryFromAbortableErrorProduceRequestInRetry() throws Excepti assertTrue(authorizedTopicProduceFuture.isDone()); prepareEndTxnResponse(Errors.NONE, TransactionResult.ABORT, producerId, epoch); - transactionManager.beginAbort(); + TransactionalRequestResult abortResult = transactionManager.beginAbort(); runUntil(transactionManager::isReady); // neither produce request has been sent, so they should both be failed immediately assertTrue(transactionManager.isReady()); assertFalse(transactionManager.hasPartitionsToAdd()); assertFalse(accumulator.hasIncomplete()); + assertTrue(abortResult.isSuccessful()); + abortResult.await(); // ensure we can now start a new transaction transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); FutureRecordMetadata nextTransactionFuture = appendToAccumulator(tp0); @@ -1452,8 +1527,7 @@ public void testTransactionalIdAuthorizationFailureInAddPartitions() { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp); + transactionManager.maybeAddPartition(tp); prepareAddPartitionsToTxn(tp, Errors.TRANSACTIONAL_ID_AUTHORIZATION_FAILED); runUntil(transactionManager::hasError); @@ -1467,8 +1541,7 @@ public void testFlushPendingPartitionsOnCommit() throws InterruptedException { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1506,8 +1579,7 @@ public void testMultipleAddPartitionsPerForOneProduce() throws InterruptedExcept transactionManager.beginTransaction(); // User does one producer.send - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1520,8 +1592,7 @@ public void testMultipleAddPartitionsPerForOneProduce() throws InterruptedExcept runUntil(() -> transactionManager.transactionContainsPartition(tp0)); // In the mean time, the user does a second produce to a different partition - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp1); + transactionManager.maybeAddPartition(tp1); Future secondResponseFuture = appendToAccumulator(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp1, epoch, producerId); @@ -1563,7 +1634,7 @@ private void verifyProducerFencedForInitProducerId(Errors error) { runUntil(transactionManager::hasError); - assertEquals(ProducerFencedException.class, result.error().getClass()); + assertThrows(ProducerFencedException.class, result::await); assertThrows(ProducerFencedException.class, () -> transactionManager.beginTransaction()); assertThrows(ProducerFencedException.class, () -> transactionManager.beginCommit()); @@ -1586,8 +1657,7 @@ private void verifyProducerFencedForAddPartitionsToTxn(Errors error) throws Inte doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1611,7 +1681,6 @@ private void verifyProducerFencedForAddOffsetsToTxn(Errors error) throws Interru doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); transactionManager.sendOffsetsToTransaction(Collections.emptyMap(), new ConsumerGroupMetadata(consumerGroupId)); Future responseFuture = appendToAccumulator(tp0); @@ -1647,8 +1716,7 @@ public void testInvalidProducerEpochConvertToProducerFencedInEndTxn() throws Int doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); TransactionalRequestResult commitResult = transactionManager.beginCommit(); Future responseFuture = appendToAccumulator(tp0); @@ -1661,6 +1729,10 @@ public void testInvalidProducerEpochConvertToProducerFencedInEndTxn() throws Int runUntil(commitResult::isCompleted); runUntil(responseFuture::isDone); + assertThrows(KafkaException.class, commitResult::await); + assertFalse(commitResult.isSuccessful()); + assertTrue(commitResult.isAcked()); + // make sure the exception was thrown directly from the follow-up calls. assertThrows(KafkaException.class, () -> transactionManager.beginTransaction()); assertThrows(KafkaException.class, () -> transactionManager.beginCommit()); @@ -1674,8 +1746,7 @@ public void testInvalidProducerEpochFromProduce() throws InterruptedException { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1709,8 +1780,7 @@ public void testDisallowCommitOnProduceFailure() throws InterruptedException { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1721,19 +1791,8 @@ public void testDisallowCommitOnProduceFailure() throws InterruptedException { runUntil(commitResult::isCompleted); // commit should be cancelled with exception without being sent. - try { - commitResult.await(); - fail(); // the get() must throw an exception. - } catch (KafkaException e) { - // Expected - } - - try { - responseFuture.get(); - fail("Expected produce future to raise an exception"); - } catch (ExecutionException e) { - assertTrue(e.getCause() instanceof OutOfOrderSequenceException); - } + assertThrows(KafkaException.class, commitResult::await); + TestUtils.assertFutureThrows(responseFuture, OutOfOrderSequenceException.class); // Commit is not allowed, so let's abort and try again. TransactionalRequestResult abortResult = transactionManager.beginAbort(); @@ -1749,8 +1808,7 @@ public void testAllowAbortOnProduceFailure() throws InterruptedException { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1773,8 +1831,7 @@ public void testAbortableErrorWhileAbortInProgress() throws InterruptedException doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1804,8 +1861,7 @@ public void testCommitTransactionWithUnsentProduceRequest() throws Exception { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1848,8 +1904,7 @@ public void testCommitTransactionWithInFlightProduceRequest() throws Exception { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1891,8 +1946,7 @@ public void testFindCoordinatorAllowedInAbortableErrorState() throws Interrupted doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1914,8 +1968,7 @@ public void testCancelUnsentAddPartitionsAndProduceOnAbort() throws InterruptedE doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -1928,12 +1981,7 @@ public void testCancelUnsentAddPartitionsAndProduceOnAbort() throws InterruptedE assertTrue(abortResult.isSuccessful()); assertTrue(transactionManager.isReady()); // make sure we are ready for a transaction now. - try { - responseFuture.get(); - fail("Expected produce future to raise an exception"); - } catch (ExecutionException e) { - assertTrue(e.getCause() instanceof KafkaException); - } + TestUtils.assertFutureThrows(responseFuture, KafkaException.class); } @Test @@ -1941,8 +1989,7 @@ public void testAbortResendsAddPartitionErrorIfRetried() throws InterruptedExcep doInitTransactions(producerId, epoch); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.UNKNOWN_TOPIC_OR_PARTITION, tp0, epoch, producerId); Future responseFuture = appendToAccumulator(tp0); @@ -1960,12 +2007,7 @@ public void testAbortResendsAddPartitionErrorIfRetried() throws InterruptedExcep assertTrue(abortResult.isSuccessful()); assertTrue(transactionManager.isReady()); // make sure we are ready for a transaction now. - try { - responseFuture.get(); - fail("Expected produce future to raise an exception"); - } catch (ExecutionException e) { - assertTrue(e.getCause() instanceof KafkaException); - } + TestUtils.assertFutureThrows(responseFuture, KafkaException.class); } @Test @@ -1973,8 +2015,7 @@ public void testAbortResendsProduceRequestIfRetried() throws Exception { doInitTransactions(producerId, epoch); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, epoch, producerId); prepareProduceResponse(Errors.REQUEST_TIMED_OUT, producerId, epoch); @@ -2002,8 +2043,7 @@ public void testHandlingOfUnknownTopicPartitionErrorOnAddPartitions() throws Int doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -2116,8 +2156,7 @@ public void shouldNotAddPartitionsToTransactionWhenTopicAuthorizationFailed() th doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); assertFalse(responseFuture.isDone()); @@ -2131,8 +2170,7 @@ public void shouldNotSendAbortTxnRequestWhenOnlyAddPartitionsRequestFailed() { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.TOPIC_AUTHORIZATION_FAILED, tp0, epoch, producerId); runUntil(() -> !client.hasPendingResponses()); @@ -2241,7 +2279,6 @@ private TransactionalRequestResult prepareGroupMetadataCommit(Runnable prepareTx assertFalse(addOffsetsResult.isCompleted()); // The request should complete only after the TxnOffsetCommit completes prepareFindCoordinatorResponse(Errors.NONE, false, CoordinatorType.GROUP, consumerGroupId); -// prepareTxnOffsetCommitResponse(consumerGroupId, producerId, epoch, groupInstanceId, memberId, generationId, txnOffsetCommitResponse); prepareTxnCommitResponse.run(); assertNull(transactionManager.coordinator(CoordinatorType.GROUP)); @@ -2256,11 +2293,9 @@ private TransactionalRequestResult prepareGroupMetadataCommit(Runnable prepareTx public void testNoDrainWhenPartitionsPending() throws InterruptedException { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); appendToAccumulator(tp0); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp1); + transactionManager.maybeAddPartition(tp1); appendToAccumulator(tp1); assertFalse(transactionManager.isSendToPartitionAllowed(tp0)); @@ -2291,13 +2326,11 @@ public void testNoDrainWhenPartitionsPending() throws InterruptedException { public void testAllowDrainInAbortableErrorState() throws InterruptedException { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp1); + transactionManager.maybeAddPartition(tp1); prepareAddPartitionsToTxn(tp1, Errors.NONE); runUntil(() -> transactionManager.transactionContainsPartition(tp1)); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxn(tp0, Errors.TOPIC_AUTHORIZATION_FAILED); runUntil(transactionManager::hasAbortableError); assertTrue(transactionManager.isSendToPartitionAllowed(tp1)); @@ -2345,8 +2378,7 @@ public void resendFailedProduceRequestAfterAbortableError() throws Exception { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -2367,8 +2399,7 @@ public void testTransitionToAbortableErrorOnBatchExpiry() throws InterruptedExce doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -2408,10 +2439,8 @@ public void testTransitionToAbortableErrorOnMultipleBatchExpiry() throws Interru doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp1); + transactionManager.maybeAddPartition(tp0); + transactionManager.maybeAddPartition(tp1); Future firstBatchResponse = appendToAccumulator(tp0); Future secondBatchResponse = appendToAccumulator(tp1); @@ -2468,8 +2497,7 @@ public void testDropCommitOnBatchExpiry() throws InterruptedException { doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -2504,6 +2532,7 @@ public void testDropCommitOnBatchExpiry() throws InterruptedException { } runUntil(commitResult::isCompleted); // the commit shouldn't be completed without being sent since the produce request failed. assertFalse(commitResult.isSuccessful()); // the commit shouldn't succeed since the produce request failed. + assertThrows(TimeoutException.class, commitResult::await); assertTrue(transactionManager.hasAbortableError()); assertTrue(transactionManager.hasOngoingTransaction()); @@ -2535,8 +2564,7 @@ public void testTransitionToFatalErrorWhenRetriedBatchIsExpired() throws Interru doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -2735,8 +2763,7 @@ public void testAbortTransactionAndReuseSequenceNumberOnError() throws Interrupt doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture0 = appendToAccumulator(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, epoch, producerId); @@ -2758,11 +2785,11 @@ public void testAbortTransactionAndReuseSequenceNumberOnError() throws Interrupt prepareEndTxnResponse(Errors.NONE, TransactionResult.ABORT, producerId, epoch); runUntil(abortResult::isCompleted); assertTrue(abortResult.isSuccessful()); + abortResult.await(); assertTrue(transactionManager.isReady()); // make sure we are ready for a transaction now. transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, epoch, producerId); runUntil(() -> transactionManager.isPartitionAdded(tp0)); // Send AddPartitionsRequest @@ -2788,16 +2815,15 @@ public void testAbortTransactionAndResetSequenceNumberOnUnknownProducerId() thro doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp1); + transactionManager.maybeAddPartition(tp1); Future successPartitionResponseFuture = appendToAccumulator(tp1); prepareAddPartitionsToTxnResponse(Errors.NONE, tp1, epoch, producerId); prepareProduceResponse(Errors.NONE, producerId, epoch, tp1); runUntil(successPartitionResponseFuture::isDone); assertTrue(transactionManager.isPartitionAdded(tp1)); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture0 = appendToAccumulator(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, epoch, producerId); prepareProduceResponse(Errors.NONE, producerId, epoch); @@ -2819,11 +2845,11 @@ public void testAbortTransactionAndResetSequenceNumberOnUnknownProducerId() thro prepareEndTxnResponse(Errors.NONE, TransactionResult.ABORT, producerId, epoch); runUntil(abortResult::isCompleted); assertTrue(abortResult.isSuccessful()); + abortResult.await(); assertTrue(transactionManager.isReady()); // make sure we are ready for a transaction now. transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, epoch, producerId); runUntil(() -> transactionManager.isPartitionAdded(tp0)); @@ -2840,8 +2866,7 @@ public void testBumpTransactionalEpochOnAbortableError() throws InterruptedExcep doInitTransactions(producerId, initialEpoch); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, initialEpoch, producerId); runUntil(() -> transactionManager.isPartitionAdded(tp0)); @@ -2867,11 +2892,11 @@ public void testBumpTransactionalEpochOnAbortableError() throws InterruptedExcep assertTrue(abortResult.isCompleted()); assertTrue(abortResult.isSuccessful()); + abortResult.await(); assertTrue(transactionManager.isReady()); // make sure we are ready for a transaction now. transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, bumpedEpoch, producerId); runUntil(() -> transactionManager.isPartitionAdded(tp0)); @@ -2887,8 +2912,7 @@ public void testBumpTransactionalEpochOnUnknownProducerIdError() throws Interrup doInitTransactions(producerId, initialEpoch); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, initialEpoch, producerId); runUntil(() -> transactionManager.isPartitionAdded(tp0)); @@ -2915,11 +2939,11 @@ public void testBumpTransactionalEpochOnUnknownProducerIdError() throws Interrup assertTrue(abortResult.isCompleted()); assertTrue(abortResult.isSuccessful()); + abortResult.await(); assertTrue(transactionManager.isReady()); // make sure we are ready for a transaction now. transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, bumpedEpoch, producerId); runUntil(() -> transactionManager.isPartitionAdded(tp0)); @@ -2935,8 +2959,7 @@ public void testBumpTransactionalEpochOnTimeout() throws InterruptedException { doInitTransactions(producerId, initialEpoch); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, initialEpoch, producerId); runUntil(() -> transactionManager.isPartitionAdded(tp0)); @@ -2975,11 +2998,11 @@ public void testBumpTransactionalEpochOnTimeout() throws InterruptedException { assertTrue(abortResult.isCompleted()); assertTrue(abortResult.isSuccessful()); + abortResult.await(); assertTrue(transactionManager.isReady()); // make sure we are ready for a transaction now. transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.NONE, tp0, bumpedEpoch, producerId); runUntil(() -> transactionManager.isPartitionAdded(tp0)); @@ -2995,8 +3018,7 @@ public void testBumpTransactionalEpochOnRecoverableAddPartitionRequestError() { doInitTransactions(producerId, initialEpoch); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); prepareAddPartitionsToTxnResponse(Errors.INVALID_PRODUCER_ID_MAPPING, tp0, initialEpoch, producerId); runUntil(transactionManager::hasAbortableError); TransactionalRequestResult abortResult = transactionManager.beginAbort(); @@ -3016,8 +3038,7 @@ public void testBumpTransactionalEpochOnRecoverableAddOffsetsRequestError() thro doInitTransactions(producerId, initialEpoch); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); Future responseFuture = appendToAccumulator(tp0); @@ -3148,12 +3169,12 @@ public void testRetryCommitTransaction() throws InterruptedException { @Test public void testRetryAbortTransactionAfterCommitTimeout() { - assertThrows(KafkaException.class, () -> verifyCommitOrAbortTransactionRetriable(TransactionResult.COMMIT, TransactionResult.ABORT)); + assertThrows(IllegalStateException.class, () -> verifyCommitOrAbortTransactionRetriable(TransactionResult.COMMIT, TransactionResult.ABORT)); } @Test public void testRetryCommitTransactionAfterAbortTimeout() { - assertThrows(KafkaException.class, () -> verifyCommitOrAbortTransactionRetriable(TransactionResult.ABORT, TransactionResult.COMMIT)); + assertThrows(IllegalStateException.class, () -> verifyCommitOrAbortTransactionRetriable(TransactionResult.ABORT, TransactionResult.COMMIT)); } @Test @@ -3270,8 +3291,7 @@ private void verifyCommitOrAbortTransactionRetriable(TransactionResult firstTran doInitTransactions(); transactionManager.beginTransaction(); - transactionManager.failIfNotReadyForSend(); - transactionManager.maybeAddPartitionToTransaction(tp0); + transactionManager.maybeAddPartition(tp0); appendToAccumulator(tp0); @@ -3284,12 +3304,7 @@ private void verifyCommitOrAbortTransactionRetriable(TransactionResult firstTran prepareEndTxnResponse(Errors.NONE, firstTransactionResult, producerId, epoch, true); runUntil(() -> !client.hasPendingResponses()); assertFalse(result.isCompleted()); - - try { - result.await(MAX_BLOCK_TIMEOUT, TimeUnit.MILLISECONDS); - fail("Should have raised TimeoutException"); - } catch (TimeoutException ignored) { - } + assertThrows(TimeoutException.class, () -> result.await(MAX_BLOCK_TIMEOUT, TimeUnit.MILLISECONDS)); prepareFindCoordinatorResponse(Errors.NONE, false, CoordinatorType.TRANSACTION, transactionalId); runUntil(() -> !client.hasPendingResponses()); @@ -3522,13 +3537,17 @@ private void doInitTransactions() { } private void doInitTransactions(long producerId, short epoch) { - transactionManager.initializeTransactions(); + TransactionalRequestResult result = transactionManager.initializeTransactions(); prepareFindCoordinatorResponse(Errors.NONE, false, CoordinatorType.TRANSACTION, transactionalId); runUntil(() -> transactionManager.coordinator(CoordinatorType.TRANSACTION) != null); assertEquals(brokerNode, transactionManager.coordinator(CoordinatorType.TRANSACTION)); prepareInitPidResponse(Errors.NONE, false, producerId, epoch); runUntil(transactionManager::hasProducerId); + + result.await(); + assertTrue(result.isSuccessful()); + assertTrue(result.isAcked()); } private void assertAbortableError(Class cause) { diff --git a/core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala b/core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala index 6efb860675d96..1da4fcd587edd 100644 --- a/core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala +++ b/core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala @@ -58,7 +58,7 @@ import org.apache.kafka.common.resource.{PatternType, Resource, ResourcePattern, import org.apache.kafka.common.security.auth.{AuthenticationContext, KafkaPrincipal, SecurityProtocol} import org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder import org.apache.kafka.common.utils.Utils -import org.apache.kafka.common.{ElectionType, IsolationLevel, Node, TopicPartition, Uuid, requests} +import org.apache.kafka.common.{ElectionType, IsolationLevel, KafkaException, Node, TopicPartition, Uuid, requests} import org.apache.kafka.test.{TestUtils => JTestUtils} import org.junit.jupiter.api.Assertions._ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test, TestInfo} @@ -1883,31 +1883,38 @@ class AuthorizerIntegrationTest extends BaseRequestTest { def testIdempotentProducerNoIdempotentWriteAclInInitProducerId(): Unit = { createTopic(topic) addAndVerifyAcls(Set(new AccessControlEntry(clientPrincipalString, WildcardHost, READ, ALLOW)), topicResource) - shouldIdempotentProducerFailInInitProducerId(true) + assertIdempotentSendAuthorizationFailure() } - def shouldIdempotentProducerFailInInitProducerId(expectAuthException: Boolean): Unit = { + private def assertIdempotentSendSuccess(): Unit = { val producer = buildIdempotentProducer() - try { + producer.send(new ProducerRecord[Array[Byte], Array[Byte]](topic, "hi".getBytes)).get() + } + + private def assertIdempotentSendAuthorizationFailure(): Unit = { + val producer = buildIdempotentProducer() + + def assertClusterAuthFailure(): Unit = { // the InitProducerId is sent asynchronously, so we expect the error either in the callback // or raised from send itself - producer.send(new ProducerRecord[Array[Byte], Array[Byte]](topic, "hi".getBytes)).get() - if (expectAuthException) - fail("Should have raised ClusterAuthorizationException") - } catch { - case e: ExecutionException => - assertTrue(e.getCause.isInstanceOf[ClusterAuthorizationException]) - } - try { - // the second time, the call to send itself should fail (the producer becomes unusable - // if no producerId can be obtained) - producer.send(new ProducerRecord[Array[Byte], Array[Byte]](topic, "hi".getBytes)).get() - if (expectAuthException) - fail("Should have raised ClusterAuthorizationException") - } catch { - case e: ExecutionException => - assertTrue(e.getCause.isInstanceOf[ClusterAuthorizationException]) + val exception = assertThrows(classOf[Exception], () => { + val future = producer.send(new ProducerRecord[Array[Byte], Array[Byte]](topic, "hi".getBytes)) + future.get() + }) + + exception match { + case e@ (_: KafkaException | _: ExecutionException) => + assertTrue(exception.getCause.isInstanceOf[ClusterAuthorizationException]) + case _ => + fail(s"Unexpected exception type raised from send: ${exception.getClass}") + } } + + assertClusterAuthFailure() + + // the second time, the call to send itself should fail (the producer becomes unusable + // if no producerId can be obtained) + assertClusterAuthFailure() } @Test @@ -2119,14 +2126,14 @@ class AuthorizerIntegrationTest extends BaseRequestTest { for (_ <- 1 to 3) { addAndVerifyAcls(Set(new AccessControlEntry(clientPrincipalString, WildcardHost, DESCRIBE, ALLOW)), topicResource) - shouldIdempotentProducerFailInInitProducerId(true) + assertIdempotentSendAuthorizationFailure() addAndVerifyAcls(Set(new AccessControlEntry(clientPrincipalString, WildcardHost, WRITE, ALLOW)), topicResource) - shouldIdempotentProducerFailInInitProducerId(false) + assertIdempotentSendSuccess() removeAllClientAcls() addAndVerifyAcls(Set(new AccessControlEntry(clientPrincipalString, WildcardHost, DESCRIBE, ALLOW)), topicResource) - shouldIdempotentProducerFailInInitProducerId(true) + assertIdempotentSendAuthorizationFailure() } } @@ -2149,7 +2156,7 @@ class AuthorizerIntegrationTest extends BaseRequestTest { addAndVerifyAcls(Set(acl1, acl4, acl5), topicResource) addAndVerifyAcls(Set(acl2, acl3), unrelatedTopicResource) addAndVerifyAcls(Set(acl2, acl3), unrelatedGroupResource) - shouldIdempotentProducerFailInInitProducerId(false) + assertIdempotentSendSuccess() } @Test @@ -2157,11 +2164,11 @@ class AuthorizerIntegrationTest extends BaseRequestTest { createTopic(topic) val allowWriteAce = new AccessControlEntry(clientPrincipalString, WildcardHost, WRITE, ALLOW) addAndVerifyAcls(Set(allowWriteAce), topicResource) - shouldIdempotentProducerFailInInitProducerId(false) + assertIdempotentSendSuccess() val denyWriteAce = new AccessControlEntry(clientPrincipalString, WildcardHost, WRITE, DENY) addAndVerifyAcls(Set(denyWriteAce), topicResource) - shouldIdempotentProducerFailInInitProducerId(true) + assertIdempotentSendAuthorizationFailure() } @Test @@ -2175,10 +2182,10 @@ class AuthorizerIntegrationTest extends BaseRequestTest { addAndVerifyAcls(Set(allowWriteAce), prefixed) addAndVerifyAcls(Set(allowWriteAce), literal) - shouldIdempotentProducerFailInInitProducerId(false) + assertIdempotentSendSuccess() addAndVerifyAcls(Set(denyWriteAce), wildcard) - shouldIdempotentProducerFailInInitProducerId(true) + assertIdempotentSendAuthorizationFailure() } @Test @@ -2191,7 +2198,7 @@ class AuthorizerIntegrationTest extends BaseRequestTest { addAndVerifyAcls(Set(denyWriteAce), prefixed) addAndVerifyAcls(Set(allowWriteAce), literal) - shouldIdempotentProducerFailInInitProducerId(true) + assertIdempotentSendAuthorizationFailure() } @Test diff --git a/core/src/test/scala/integration/kafka/api/TransactionsTest.scala b/core/src/test/scala/integration/kafka/api/TransactionsTest.scala index 1fbba9e7ee394..2d8689fa64d9f 100644 --- a/core/src/test/scala/integration/kafka/api/TransactionsTest.scala +++ b/core/src/test/scala/integration/kafka/api/TransactionsTest.scala @@ -30,7 +30,7 @@ import kafka.utils.TestUtils.consumeRecords import org.apache.kafka.clients.consumer.{ConsumerConfig, ConsumerGroupMetadata, KafkaConsumer, OffsetAndMetadata} import org.apache.kafka.clients.producer.{KafkaProducer, ProducerRecord} import org.apache.kafka.common.errors.{InvalidProducerEpochException, ProducerFencedException, TimeoutException} -import org.apache.kafka.common.{KafkaException, TopicPartition} +import org.apache.kafka.common.TopicPartition import org.junit.jupiter.api.Assertions._ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test, TestInfo} @@ -604,7 +604,7 @@ class TransactionsTest extends KafkaServerTestHarness { val producer = createTransactionalProducer(transactionalId = "normalProducer") producer.initTransactions() - assertThrows(classOf[KafkaException], () => producer.initTransactions()) + assertThrows(classOf[IllegalStateException], () => producer.initTransactions()) } @Test From 035f4181d951b5d13faa048ba83ac40d01869807 Mon Sep 17 00:00:00 2001 From: David Jacot Date: Mon, 24 Jan 2022 17:08:03 +0100 Subject: [PATCH 07/51] MINOR: Upgrade netty to 4.1.73.Final (#11706) Changelog: https://github.com/netty/netty/issues?q=is%3Aclosed+milestone%3A4.1.73.Final Reviewers: Manikumar Reddy --- gradle/dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index fa3998b7b5993..05d97ec3db059 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -102,7 +102,7 @@ versions += [ mavenArtifact: "3.8.1", metrics: "2.2.0", mockito: "3.12.4", - netty: "4.1.68.Final", + netty: "4.1.73.Final", powermock: "2.0.9", reflections: "0.9.12", rocksDB: "6.22.1.1", From 56bc731bfe1d6fc9bc5bc555a2e3a94bea4528bf Mon Sep 17 00:00:00 2001 From: David Jacot Date: Wed, 26 Jan 2022 08:14:00 +0100 Subject: [PATCH 08/51] KAFKA-13585; Fix flaky test `ReplicaManagerTest.testReplicaAlterLogDirsWithAndWithoutIds` (#11665) The issue was quite subtile. It was due to a race for the `partitionMapLock` lock. `assertFetcherHasTopicId` would only succeed if it can acquire it before `processFetchRequest`. This PR refactors the test in order to make it more stable. Reviewers: Justine Olshan , Jason Gustafson , --- .../kafka/server/ReplicaManagerTest.scala | 100 ++++++++++-------- 1 file changed, 58 insertions(+), 42 deletions(-) diff --git a/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala b/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala index 2b1a397db20d3..07a43519a33ac 100644 --- a/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala +++ b/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala @@ -2009,7 +2009,8 @@ class ReplicaManagerTest { brokerId: Int = 0, aliveBrokerIds: Seq[Int] = Seq(0, 1), propsModifier: Properties => Unit = _ => {}, - mockReplicaFetcherManager: Option[ReplicaFetcherManager] = None + mockReplicaFetcherManager: Option[ReplicaFetcherManager] = None, + mockReplicaAlterLogDirsManager: Option[ReplicaAlterLogDirsManager] = None ): ReplicaManager = { val props = TestUtils.createBrokerConfig(brokerId, TestUtils.MockZkConnect) props.put("log.dirs", TestUtils.tempRelativeDir("data").getAbsolutePath + "," + TestUtils.tempRelativeDir("data2").getAbsolutePath) @@ -2064,6 +2065,18 @@ class ReplicaManagerTest { ) } } + + override def createReplicaAlterLogDirsManager( + quotaManager: ReplicationQuotaManager, + brokerTopicStats: BrokerTopicStats + ): ReplicaAlterLogDirsManager = { + mockReplicaAlterLogDirsManager.getOrElse { + super.createReplicaAlterLogDirsManager( + quotaManager, + brokerTopicStats + ) + } + } } } @@ -2814,7 +2827,7 @@ class ReplicaManagerTest { // Delete the data directory to trigger a storage exception Utils.delete(dataDir) - val request = leaderAndIsrRequest( + val request = makeLeaderAndIsrRequest( topicId = Uuid.randomUuid(), topicPartition = topicPartition, replicas = Seq(0, 1), @@ -2830,7 +2843,7 @@ class ReplicaManagerTest { } } - private def leaderAndIsrRequest( + private def makeLeaderAndIsrRequest( topicId: Uuid, topicPartition: TopicPartition, replicas: Seq[Int], @@ -2887,7 +2900,7 @@ class ReplicaManagerTest { // This API is supported by both leaders and followers val barPartition = new TopicPartition("bar", 0) - val barLeaderAndIsrRequest = leaderAndIsrRequest( + val barLeaderAndIsrRequest = makeLeaderAndIsrRequest( topicId = Uuid.randomUuid(), topicPartition = barPartition, replicas = Seq(brokerId), @@ -2899,7 +2912,7 @@ class ReplicaManagerTest { val otherBrokerId = 1 val bazPartition = new TopicPartition("baz", 0) - val bazLeaderAndIsrRequest = leaderAndIsrRequest( + val bazLeaderAndIsrRequest = makeLeaderAndIsrRequest( topicId = Uuid.randomUuid(), topicPartition = bazPartition, replicas = Seq(brokerId, otherBrokerId), @@ -3515,7 +3528,7 @@ class ReplicaManagerTest { // or does not start with a topic ID in the PartitionFetchState and adds one on the next request (!startsWithTopicId) val startingId = if (startsWithTopicId) topicId else Uuid.ZERO_UUID val startingIdOpt = if (startsWithTopicId) Some(topicId) else None - val leaderAndIsrRequest1 = leaderAndIsrRequest(startingId, tp, aliveBrokersIds, leaderAndIsr) + val leaderAndIsrRequest1 = makeLeaderAndIsrRequest(startingId, tp, aliveBrokersIds, leaderAndIsr) val leaderAndIsrResponse1 = replicaManager.becomeLeaderOrFollower(0, leaderAndIsrRequest1, (_, _) => ()) assertEquals(Errors.NONE, leaderAndIsrResponse1.error) @@ -3523,7 +3536,7 @@ class ReplicaManagerTest { val endingId = if (!startsWithTopicId) topicId else Uuid.ZERO_UUID val endingIdOpt = if (!startsWithTopicId) Some(topicId) else None - val leaderAndIsrRequest2 = leaderAndIsrRequest(endingId, tp, aliveBrokersIds, leaderAndIsr) + val leaderAndIsrRequest2 = makeLeaderAndIsrRequest(endingId, tp, aliveBrokersIds, leaderAndIsr) val leaderAndIsrResponse2 = replicaManager.becomeLeaderOrFollower(0, leaderAndIsrRequest2, (_, _) => ()) assertEquals(Errors.NONE, leaderAndIsrResponse2.error) @@ -3536,48 +3549,51 @@ class ReplicaManagerTest { @ParameterizedTest @ValueSource(booleans = Array(true, false)) def testReplicaAlterLogDirsWithAndWithoutIds(usesTopicIds: Boolean): Unit = { + val tp = new TopicPartition(topic, 0) val version = if (usesTopicIds) LeaderAndIsrRequestData.HIGHEST_SUPPORTED_VERSION else 4.toShort val topicId = if (usesTopicIds) this.topicId else Uuid.ZERO_UUID val topicIdOpt = if (usesTopicIds) Some(topicId) else None - val replicaManager = setupReplicaManagerWithMockedPurgatories(new MockTimer(time)) - try { - val topicPartition = new TopicPartition(topic, 0) - val aliveBrokersIds = Seq(0, 1) - replicaManager.createPartition(topicPartition) - .createLogIfNotExists(isNew = false, isFutureReplica = false, - new LazyOffsetCheckpoints(replicaManager.highWatermarkCheckpoints), None) - val tp = new TopicPartition(topic, 0) - val leaderAndIsr = new LeaderAndIsr(0, 0, aliveBrokersIds.toList, 0) - - val leaderAndIsrRequest1 = leaderAndIsrRequest(topicId, tp, aliveBrokersIds, leaderAndIsr, version = version) - replicaManager.becomeLeaderOrFollower(0, leaderAndIsrRequest1, (_, _) => ()) - val partition = replicaManager.getPartitionOrException(tp) - assertEquals(1, replicaManager.logManager.liveLogDirs.filterNot(_ == partition.log.get.dir.getParentFile).size) - - // Append a couple of messages. - for (i <- 1 to 500) { - val records = TestUtils.singletonRecords(s"message $i".getBytes) - appendRecords(replicaManager, tp, records).onFire { response => - assertEquals(Errors.NONE, response.error) - } - } - // Find the live and different folder. - val newReplicaFolder = replicaManager.logManager.liveLogDirs.filterNot(_ == partition.log.get.dir.getParentFile).head - assertEquals(0, replicaManager.replicaAlterLogDirsManager.fetcherThreadMap.size) - replicaManager.alterReplicaLogDirs(Map(topicPartition -> newReplicaFolder.getAbsolutePath)) + val mockReplicaAlterLogDirsManager = Mockito.mock(classOf[ReplicaAlterLogDirsManager]) + val replicaManager = setupReplicaManagerWithMockedPurgatories( + timer = new MockTimer(time), + mockReplicaAlterLogDirsManager = Some(mockReplicaAlterLogDirsManager) + ) - assertFetcherHasTopicId(replicaManager.replicaAlterLogDirsManager, partition.topicPartition, topicIdOpt) + try { + replicaManager.createPartition(tp).createLogIfNotExists( + isNew = false, + isFutureReplica = false, + offsetCheckpoints = new LazyOffsetCheckpoints(replicaManager.highWatermarkCheckpoints), + topicId = None + ) - // Make sure the future log is created. - replicaManager.futureLocalLogOrException(topicPartition) - assertEquals(1, replicaManager.replicaAlterLogDirsManager.fetcherThreadMap.size) + val leaderAndIsrRequest = makeLeaderAndIsrRequest( + topicId = topicId, + topicPartition = tp, + replicas = Seq(0, 1), + leaderAndIsr = LeaderAndIsr(0, List(0, 1)), + version = version + ) + replicaManager.becomeLeaderOrFollower(0, leaderAndIsrRequest, (_, _) => ()) - // Wait for the ReplicaAlterLogDirsThread to complete. - TestUtils.waitUntilTrue(() => { - replicaManager.replicaAlterLogDirsManager.shutdownIdleFetcherThreads() - replicaManager.replicaAlterLogDirsManager.fetcherThreadMap.isEmpty - }, s"ReplicaAlterLogDirsThread should be gone") + // Move the replica to the second log directory. + val partition = replicaManager.getPartitionOrException(tp) + val newReplicaFolder = replicaManager.logManager.liveLogDirs.filterNot(_ == partition.log.get.dir.getParentFile).head + replicaManager.alterReplicaLogDirs(Map(tp -> newReplicaFolder.getAbsolutePath)) + + // Make sure the future log is created with the correct topic ID. + val futureLog = replicaManager.futureLocalLogOrException(tp) + assertEquals(topicIdOpt, futureLog.topicId) + + // Verify that addFetcherForPartitions was called with the correct topic ID. + Mockito.verify(mockReplicaAlterLogDirsManager, Mockito.times(1)) + .addFetcherForPartitions(Map(tp -> InitialFetchState( + topicId = topicIdOpt, + leader = BrokerEndPoint(0, "localhost", -1), + currentLeaderEpoch = 0, + initOffset = 0 + ))) } finally { replicaManager.shutdown(checkpointHW = false) } From 0a3c26e1de237dd68ee1d855d093232621f118aa Mon Sep 17 00:00:00 2001 From: Philip Nee Date: Wed, 2 Feb 2022 16:03:32 -0800 Subject: [PATCH 09/51] KAFKA-12841: Fix producer callback handling when partition is missing (#11689) Sometimes, the Kafka producer encounters an error prior to selecting a topic partition. In this case, we would like to acknowledge the failure in the producer interceptors, if any are configured. We should also pass a non-null Metadata object to the producer callback, if there is one. This PR implements that behavior. It also updates the JavaDoc to clarify that if a partition cannot be selected, we will pass back a partition id of -1 in the metadata. This is in keeping with KAFKA-3303. Co-authors: Kirk True Reviewers: Colin P. McCabe --- .../kafka/clients/producer/Callback.java | 5 ++- .../kafka/clients/producer/KafkaProducer.java | 13 +++++- .../internals/ProducerInterceptors.java | 7 ++- .../clients/producer/KafkaProducerTest.java | 43 +++++++++++++++++++ 4 files changed, 62 insertions(+), 6 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/Callback.java b/clients/src/main/java/org/apache/kafka/clients/producer/Callback.java index ee0610ebe4722..236d04b0709d1 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/Callback.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/Callback.java @@ -25,10 +25,11 @@ public interface Callback { /** * A callback method the user can implement to provide asynchronous handling of request completion. This method will * be called when the record sent to the server has been acknowledged. When exception is not null in the callback, - * metadata will contain the special -1 value for all fields except for topicPartition, which will be valid. + * metadata will contain the special -1 value for all fields. If topicPartition cannot be + * choosen, a -1 value will be assigned. * * @param metadata The metadata for the record that was sent (i.e. the partition and offset). An empty metadata - * with -1 value for all fields except for topicPartition will be returned if an error occurred. + * with -1 value for all fields will be returned if an error occurred. * @param exception The exception thrown during processing of this record. Null if no error occurred. * Possible thrown exceptions include: * diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java index a7f9389219add..2a68d0f496e5b 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java @@ -996,8 +996,17 @@ private Future doSend(ProducerRecord record, Callback call // for other exceptions throw directly } catch (ApiException e) { log.debug("Exception occurred during message send:", e); - if (callback != null) - callback.onCompletion(null, e); + // producer callback will make sure to call both 'callback' and interceptor callback + if (tp == null) { + // set topicPartition to -1 when null + tp = ProducerInterceptors.extractTopicPartition(record); + } + + Callback interceptCallback = new InterceptorCallback<>(callback, this.interceptors, tp); + + // The onCompletion callback does expect a non-null metadata, but one will be created inside + // the interceptor's onCompletion implementation before the user's callback is invoked. + interceptCallback.onCompletion(null, e); this.errors.record(); this.interceptors.onSendError(record, tp, e); return new FutureFailure(e); diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerInterceptors.java b/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerInterceptors.java index ceec552e26d80..dd72409770f6c 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerInterceptors.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/internals/ProducerInterceptors.java @@ -110,8 +110,7 @@ public void onSendError(ProducerRecord record, TopicPartition interceptTop interceptor.onAcknowledgement(null, exception); } else { if (interceptTopicPartition == null) { - interceptTopicPartition = new TopicPartition(record.topic(), - record.partition() == null ? RecordMetadata.UNKNOWN_PARTITION : record.partition()); + interceptTopicPartition = extractTopicPartition(record); } interceptor.onAcknowledgement(new RecordMetadata(interceptTopicPartition, -1, -1, RecordBatch.NO_TIMESTAMP, -1, -1), exception); @@ -123,6 +122,10 @@ public void onSendError(ProducerRecord record, TopicPartition interceptTop } } + public static TopicPartition extractTopicPartition(ProducerRecord record) { + return new TopicPartition(record.topic(), record.partition() == null ? RecordMetadata.UNKNOWN_PARTITION : record.partition()); + } + /** * Closes every interceptor in a container. */ diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java index 96a30341b842f..7c521fde68d9a 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java @@ -50,6 +50,7 @@ import org.apache.kafka.common.network.Selectable; import org.apache.kafka.common.protocol.ApiKeys; import org.apache.kafka.common.protocol.Errors; +import org.apache.kafka.common.record.RecordBatch; import org.apache.kafka.common.requests.AddOffsetsToTxnResponse; import org.apache.kafka.common.requests.EndTxnResponse; import org.apache.kafka.common.requests.FindCoordinatorRequest; @@ -57,6 +58,7 @@ import org.apache.kafka.common.requests.InitProducerIdResponse; import org.apache.kafka.common.requests.JoinGroupRequest; import org.apache.kafka.common.requests.MetadataResponse; +import org.apache.kafka.common.requests.ProduceResponse; import org.apache.kafka.common.requests.RequestTestUtils; import org.apache.kafka.common.requests.TxnOffsetCommitRequest; import org.apache.kafka.common.requests.TxnOffsetCommitResponse; @@ -1548,6 +1550,47 @@ public void testNullTopicName() { "key".getBytes(StandardCharsets.UTF_8), "value".getBytes(StandardCharsets.UTF_8))); } + @Test + public void testCallbackHandlesError() throws Exception { + Map configs = new HashMap<>(); + configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9000"); + configs.put(ProducerConfig.MAX_BLOCK_MS_CONFIG, "1000"); + + Time time = new MockTime(); + ProducerMetadata producerMetadata = newMetadata(0, Long.MAX_VALUE); + MockClient client = new MockClient(time, producerMetadata); + + String invalidTopicName = "topic abc"; // Invalid topic name due to space + + try (Producer producer = kafkaProducer(configs, new StringSerializer(), new StringSerializer(), + producerMetadata, client, null, time)) { + ProducerRecord record = new ProducerRecord<>(invalidTopicName, "HelloKafka"); + + // Here's the important piece of the test. Let's make sure that the RecordMetadata we get + // is non-null and adheres to the onCompletion contract. + Callback callBack = (recordMetadata, exception) -> { + assertNotNull(exception); + assertNotNull(recordMetadata); + + assertNotNull(recordMetadata.topic(), "Topic name should be valid even on send failure"); + assertEquals(invalidTopicName, recordMetadata.topic()); + assertNotNull(recordMetadata.partition(), "Partition should be valid even on send failure"); + + assertFalse(recordMetadata.hasOffset()); + assertEquals(ProduceResponse.INVALID_OFFSET, recordMetadata.offset()); + + assertFalse(recordMetadata.hasTimestamp()); + assertEquals(RecordBatch.NO_TIMESTAMP, recordMetadata.timestamp()); + + assertEquals(-1, recordMetadata.serializedKeySize()); + assertEquals(-1, recordMetadata.serializedValueSize()); + assertEquals(-1, recordMetadata.partition()); + }; + + producer.send(record, callBack); + } + } + private static final List CLIENT_IDS = new ArrayList<>(); public static class SerializerForClientId implements Serializer { From ae09d9c32793777fa3d3fc90ce67eb504bec9b2e Mon Sep 17 00:00:00 2001 From: dengziming Date: Thu, 3 Feb 2022 17:32:25 +0800 Subject: [PATCH 10/51] KAFKA-13637: Use default.api.timeout.ms as default timeout value for KafkaConsumer.endOffsets (#11726) We introduced `default.api.timeout.ms` in https://github.com/apache/kafka/commit/53ca52f855e903907378188d29224b3f9cefa6cb but we missed updating `KafkaConsumer.endOffsets` which still use `request.timeout.ms`. This patch fixes this. Reviewers: David Jacot --- .../kafka/clients/consumer/KafkaConsumer.java | 4 +- .../clients/consumer/KafkaConsumerTest.java | 58 ++++++++++++++++++- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java b/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java index 286f84b40d9f2..5e58bee1e1c66 100644 --- a/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java +++ b/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java @@ -2186,11 +2186,11 @@ public Map beginningOffsets(Collection par * @throws org.apache.kafka.common.errors.AuthenticationException if authentication fails. See the exception for more details * @throws org.apache.kafka.common.errors.AuthorizationException if not authorized to the topic(s). See the exception for more details * @throws org.apache.kafka.common.errors.TimeoutException if the offset metadata could not be fetched before - * the amount of time allocated by {@code request.timeout.ms} expires + * the amount of time allocated by {@code default.api.timeout.ms} expires */ @Override public Map endOffsets(Collection partitions) { - return endOffsets(partitions, Duration.ofMillis(requestTimeoutMs)); + return endOffsets(partitions, Duration.ofMillis(defaultApiTimeoutMs)); } /** diff --git a/clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java b/clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java index 28729834c1d34..9b79473d89e4a 100644 --- a/clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java @@ -164,6 +164,8 @@ public class KafkaConsumerTest { private final TopicPartition t3p0 = new TopicPartition(topic3, 0); private final int sessionTimeoutMs = 10000; + private final int defaultApiTimeoutMs = 60000; + private final int requestTimeoutMs = defaultApiTimeoutMs / 2; private final int heartbeatIntervalMs = 1000; // Set auto commit interval lower than heartbeat so we don't need to deal with @@ -2618,8 +2620,6 @@ private KafkaConsumer newConsumer(Time time, String clientId = "mock-consumer"; String metricGroupPrefix = "consumer"; long retryBackoffMs = 100; - int requestTimeoutMs = 30000; - int defaultApiTimeoutMs = 30000; int minBytes = 1; int maxBytes = Integer.MAX_VALUE; int maxWaitMs = 500; @@ -2948,6 +2948,60 @@ public void testAssignorNameConflict() { () -> new KafkaConsumer<>(configs, new StringDeserializer(), new StringDeserializer())); } + @Test + public void testOffsetsForTimesTimeout() { + final KafkaConsumer consumer = consumerForCheckingTimeoutException(); + assertEquals( + "Failed to get offsets by times in 60000ms", + assertThrows(org.apache.kafka.common.errors.TimeoutException.class, () -> consumer.offsetsForTimes(singletonMap(tp0, 0L))).getMessage() + ); + } + + @Test + public void testBeginningOffsetsTimeout() { + final KafkaConsumer consumer = consumerForCheckingTimeoutException(); + assertEquals( + "Failed to get offsets by times in 60000ms", + assertThrows(org.apache.kafka.common.errors.TimeoutException.class, () -> consumer.beginningOffsets(singletonList(tp0))).getMessage() + ); + } + + @Test + public void testEndOffsetsTimeout() { + final KafkaConsumer consumer = consumerForCheckingTimeoutException(); + assertEquals( + "Failed to get offsets by times in 60000ms", + assertThrows(org.apache.kafka.common.errors.TimeoutException.class, () -> consumer.endOffsets(singletonList(tp0))).getMessage() + ); + } + + private KafkaConsumer consumerForCheckingTimeoutException() { + final Time time = new MockTime(); + SubscriptionState subscription = new SubscriptionState(new LogContext(), OffsetResetStrategy.EARLIEST); + ConsumerMetadata metadata = createMetadata(subscription); + MockClient client = new MockClient(time, metadata); + + initMetadata(client, singletonMap(topic, 1)); + + ConsumerPartitionAssignor assignor = new RangeAssignor(); + + final KafkaConsumer consumer = newConsumer(time, client, subscription, metadata, assignor, false, groupInstanceId); + + for (int i = 0; i < 10; i++) { + client.prepareResponse( + request -> { + time.sleep(defaultApiTimeoutMs / 10); + return request instanceof ListOffsetsRequest; + }, + listOffsetsResponse( + Collections.emptyMap(), + Collections.singletonMap(tp0, Errors.UNKNOWN_TOPIC_OR_PARTITION) + )); + } + + return consumer; + } + private static final List CLIENT_IDS = new ArrayList<>(); public static class DeserializerForClientId implements Deserializer { @Override From 9a7e975f661a80d17f6fb0693ae5387081606873 Mon Sep 17 00:00:00 2001 From: Kvicii <42023367+Kvicii@users.noreply.github.com> Date: Thu, 3 Feb 2022 17:59:12 +0800 Subject: [PATCH 11/51] KAFKA-13583; Fix FetchRequestBetweenDifferentIbpTest flaky tests (#11699) Co-authored-by: Kvicii Reviewers: David Jacot --- .../kafka/server/FetchRequestBetweenDifferentIbpTest.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/test/scala/integration/kafka/server/FetchRequestBetweenDifferentIbpTest.scala b/core/src/test/scala/integration/kafka/server/FetchRequestBetweenDifferentIbpTest.scala index 37ac4a29ea93d..d10bafde69a9a 100644 --- a/core/src/test/scala/integration/kafka/server/FetchRequestBetweenDifferentIbpTest.scala +++ b/core/src/test/scala/integration/kafka/server/FetchRequestBetweenDifferentIbpTest.scala @@ -73,7 +73,7 @@ class FetchRequestBetweenDifferentIbpTest extends BaseRequestTest { producer.send(record2) consumer.assign(asList(new TopicPartition(topic, 0), new TopicPartition(topic, 1))) - val count = consumer.poll(Duration.ofMillis(1500)).count() + consumer.poll(Duration.ofMillis(1500)).count() + val count = consumer.poll(Duration.ofMillis(5000)).count() + consumer.poll(Duration.ofMillis(5000)).count() assertEquals(2, count) } @@ -109,7 +109,7 @@ class FetchRequestBetweenDifferentIbpTest extends BaseRequestTest { consumer.assign(asList(new TopicPartition(topic, 0), new TopicPartition(topic, 1))) - val count = consumer.poll(Duration.ofMillis(1500)).count() + consumer.poll(Duration.ofMillis(1500)).count() + val count = consumer.poll(Duration.ofMillis(5000)).count() + consumer.poll(Duration.ofMillis(5000)).count() assertEquals(2, count) // Make controller version2 @@ -128,7 +128,7 @@ class FetchRequestBetweenDifferentIbpTest extends BaseRequestTest { // Assign this new topic in addition to the old topics. consumer.assign(asList(new TopicPartition(topic, 0), new TopicPartition(topic, 1), new TopicPartition(topic2, 0))) - val count2 = consumer.poll(Duration.ofMillis(1500)).count() + consumer.poll(Duration.ofMillis(1500)).count() + val count2 = consumer.poll(Duration.ofMillis(5000)).count() + consumer.poll(Duration.ofMillis(5000)).count() assertEquals(2, count2) } From 5227478fb18e4589d1ef8044bf4a26780400cedb Mon Sep 17 00:00:00 2001 From: Luke Chen Date: Sun, 6 Feb 2022 02:53:27 +0800 Subject: [PATCH 12/51] KAFKA-13598: enable idempotence producer by default and validate the configs (#11691) In v3.0, we changed the default value for `enable.idempotence` to true, but we didn't adjust the validator and the `idempotence` enabled check method. So if a user didn't explicitly enable idempotence, this feature won't be turned on. This patch addresses the problem, cleans up associated logic, and fixes tests that broke as a result of properly applying the new default. Specifically it does the following: 1. fix the `ProducerConfig#idempotenceEnabled` method, to make it correctly detect if `idempotence` is enabled or not 2. remove some unnecessary config overridden and checks due to we already default `acks`, `retries` and `enable.idempotence` configs. 3. move the config validator for the idempotent producer from `KafkaProducer` into `ProducerConfig`. The config validation should be the responsibility of `ProducerConfig` class. 4. add an `AbstractConfig#hasKeyInOriginals` method, to avoid `originals` configs get copied and only want to check the existence of the key. 5. fix many broken tests. As mentioned, we didn't actually enable idempotence in v3.0. After this PR, there are some tests broken due to some different behavior between idempotent and non-idempotent producer. 6. add additional tests to validate configuration behavior Reviewers: Kirk True , Ismael Juma , Mickael Maison , Jason Gustafson --- .../kafka/clients/CommonClientConfigs.java | 5 +- .../kafka/clients/producer/KafkaProducer.java | 34 +-- .../clients/producer/ProducerConfig.java | 47 ++-- .../clients/producer/KafkaProducerTest.java | 233 +++++++++++++++--- .../storage/KafkaStatusBackingStore.java | 1 + .../scala/kafka/tools/ConsoleProducer.scala | 2 +- .../kafka/api/AuthorizerIntegrationTest.scala | 2 + ...thLegacyMessageFormatIntegrationTest.scala | 6 +- .../kafka/api/EndToEndAuthorizationTest.scala | 52 +++- .../integration/kafka/api/MetricsTest.scala | 5 +- ...aslClientsWithInvalidCredentialsTest.scala | 23 +- .../DynamicBrokerReconfigurationTest.scala | 2 + .../unit/kafka/server/LogDirFailureTest.scala | 2 + docs/upgrade.html | 8 + .../log4jappender/KafkaLog4jAppenderTest.java | 4 +- 15 files changed, 309 insertions(+), 117 deletions(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java b/clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java index 58075d628927e..5371a73ece192 100644 --- a/clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java +++ b/clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java @@ -194,8 +194,9 @@ public class CommonClientConfigs { public static Map postProcessReconnectBackoffConfigs(AbstractConfig config, Map parsedValues) { HashMap rval = new HashMap<>(); - if ((!config.originals().containsKey(RECONNECT_BACKOFF_MAX_MS_CONFIG)) && - config.originals().containsKey(RECONNECT_BACKOFF_MS_CONFIG)) { + Map originalConfig = config.originals(); + if ((!originalConfig.containsKey(RECONNECT_BACKOFF_MAX_MS_CONFIG)) && + originalConfig.containsKey(RECONNECT_BACKOFF_MS_CONFIG)) { log.debug("Disabling exponential reconnect backoff because {} is set, but {} is not.", RECONNECT_BACKOFF_MS_CONFIG, RECONNECT_BACKOFF_MAX_MS_CONFIG); rval.put(RECONNECT_BACKOFF_MAX_MS_CONFIG, parsedValues.get(RECONNECT_BACKOFF_MS_CONFIG)); diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java index 2a68d0f496e5b..ac3343e427a3f 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java @@ -445,7 +445,7 @@ public KafkaProducer(Properties properties, Serializer keySerializer, Seriali // visible for testing Sender newSender(LogContext logContext, KafkaClient kafkaClient, ProducerMetadata metadata) { - int maxInflightRequests = configureInflightRequests(producerConfig); + int maxInflightRequests = producerConfig.getInt(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION); int requestTimeoutMs = producerConfig.getInt(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG); ChannelBuilder channelBuilder = ClientUtils.createChannelBuilder(producerConfig, time, logContext); ProducerMetrics metricsRegistry = new ProducerMetrics(this.metrics); @@ -468,7 +468,8 @@ Sender newSender(LogContext logContext, KafkaClient kafkaClient, ProducerMetadat apiVersions, throttleTimeSensor, logContext); - short acks = configureAcks(producerConfig, log); + + short acks = Short.parseShort(producerConfig.getString(ProducerConfig.ACKS_CONFIG)); return new Sender(logContext, client, metadata, @@ -514,15 +515,8 @@ private static int configureDeliveryTimeout(ProducerConfig config, Logger log) { private TransactionManager configureTransactionState(ProducerConfig config, LogContext logContext) { - TransactionManager transactionManager = null; - final boolean userConfiguredIdempotence = config.originals().containsKey(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG); - final boolean userConfiguredTransactions = config.originals().containsKey(ProducerConfig.TRANSACTIONAL_ID_CONFIG); - if (userConfiguredTransactions && !userConfiguredIdempotence) - log.info("Overriding the default {} to true since {} is specified.", ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, - ProducerConfig.TRANSACTIONAL_ID_CONFIG); - if (config.idempotenceEnabled()) { final String transactionalId = config.getString(ProducerConfig.TRANSACTIONAL_ID_CONFIG); final int transactionTimeoutMs = config.getInt(ProducerConfig.TRANSACTION_TIMEOUT_CONFIG); @@ -543,28 +537,6 @@ private TransactionManager configureTransactionState(ProducerConfig config, return transactionManager; } - private static int configureInflightRequests(ProducerConfig config) { - if (config.idempotenceEnabled() && 5 < config.getInt(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION)) { - throw new ConfigException("Must set " + ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION + " to at most 5" + - " to use the idempotent producer."); - } - return config.getInt(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION); - } - - private static short configureAcks(ProducerConfig config, Logger log) { - boolean userConfiguredAcks = config.originals().containsKey(ProducerConfig.ACKS_CONFIG); - short acks = Short.parseShort(config.getString(ProducerConfig.ACKS_CONFIG)); - - if (config.idempotenceEnabled()) { - if (!userConfiguredAcks) - log.info("Overriding the default {} to all since idempotence is enabled.", ProducerConfig.ACKS_CONFIG); - else if (acks != -1) - throw new ConfigException("Must set " + ProducerConfig.ACKS_CONFIG + " to all in order to use the idempotent " + - "producer. Otherwise we cannot guarantee idempotence."); - } - return acks; - } - /** * Needs to be called before any other methods when the transactional.id is set in the configuration. * diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java b/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java index fbd3449f29c8f..9a698b1ecef3c 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java @@ -207,6 +207,8 @@ public class ProducerConfig extends AbstractConfig { private static final String MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION_DOC = "The maximum number of unacknowledged requests the client will send on a single connection before blocking." + " Note that if this config is set to be greater than 1 and enable.idempotence is set to false, there is a risk of" + " message re-ordering after a failed send due to retries (i.e., if retries are enabled)."; + // max.in.flight.requests.per.connection should be less than or equal to 5 when idempotence producer enabled to ensure message ordering + private static final int MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION_FOR_IDEMPOTENCE = 5; /** retries */ public static final String RETRIES_CONFIG = CommonClientConfigs.RETRIES_CONFIG; @@ -269,8 +271,8 @@ public class ProducerConfig extends AbstractConfig { public static final String ENABLE_IDEMPOTENCE_CONFIG = "enable.idempotence"; public static final String ENABLE_IDEMPOTENCE_DOC = "When set to 'true', the producer will ensure that exactly one copy of each message is written in the stream. If 'false', producer " + "retries due to broker failures, etc., may write duplicates of the retried message in the stream. " - + "Note that enabling idempotence requires " + MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION + " to be less than or equal to 5 " - + "(with message ordering preserved for any allowable value), " + RETRIES_CONFIG + " to be greater than 0, and " + + "Note that enabling idempotence requires " + MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION + " to be less than or equal to " + MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION_FOR_IDEMPOTENCE + + " (with message ordering preserved for any allowable value), " + RETRIES_CONFIG + " to be greater than 0, and " + ACKS_CONFIG + " must be 'all'. If these values are not explicitly set by the user, suitable values will be chosen. If incompatible " + "values are set, a ConfigException will be thrown."; @@ -438,9 +440,8 @@ public class ProducerConfig extends AbstractConfig { @Override protected Map postProcessParsedConfig(final Map parsedValues) { Map refinedConfigs = CommonClientConfigs.postProcessReconnectBackoffConfigs(this, parsedValues); - maybeOverrideEnableIdempotence(refinedConfigs); + postProcessAndValidateIdempotenceConfigs(refinedConfigs); maybeOverrideClientId(refinedConfigs); - maybeOverrideAcksAndRetries(refinedConfigs); return refinedConfigs; } @@ -456,33 +457,30 @@ private void maybeOverrideClientId(final Map configs) { configs.put(CLIENT_ID_CONFIG, refinedClientId); } - private void maybeOverrideEnableIdempotence(final Map configs) { - boolean userConfiguredIdempotence = this.originals().containsKey(ENABLE_IDEMPOTENCE_CONFIG); - boolean userConfiguredTransactions = this.originals().containsKey(TRANSACTIONAL_ID_CONFIG); - - if (userConfiguredTransactions && !userConfiguredIdempotence) { - configs.put(ENABLE_IDEMPOTENCE_CONFIG, true); - } - } - - private void maybeOverrideAcksAndRetries(final Map configs) { + private void postProcessAndValidateIdempotenceConfigs(final Map configs) { + final Map originalConfigs = this.originals(); final String acksStr = parseAcks(this.getString(ACKS_CONFIG)); configs.put(ACKS_CONFIG, acksStr); - // For idempotence producers, values for `RETRIES_CONFIG` and `ACKS_CONFIG` might need to be overridden. + + // For idempotence producers, values for `RETRIES_CONFIG` and `ACKS_CONFIG` need validation if (idempotenceEnabled()) { - boolean userConfiguredRetries = this.originals().containsKey(RETRIES_CONFIG); - if (this.getInt(RETRIES_CONFIG) == 0) { + boolean userConfiguredRetries = originalConfigs.containsKey(RETRIES_CONFIG); + if (userConfiguredRetries && this.getInt(RETRIES_CONFIG) == 0) { throw new ConfigException("Must set " + ProducerConfig.RETRIES_CONFIG + " to non-zero when using the idempotent producer."); } - configs.put(RETRIES_CONFIG, userConfiguredRetries ? this.getInt(RETRIES_CONFIG) : Integer.MAX_VALUE); - boolean userConfiguredAcks = this.originals().containsKey(ACKS_CONFIG); + boolean userConfiguredAcks = originalConfigs.containsKey(ACKS_CONFIG); final short acks = Short.valueOf(acksStr); if (userConfiguredAcks && acks != (short) -1) { throw new ConfigException("Must set " + ACKS_CONFIG + " to all in order to use the idempotent " + "producer. Otherwise we cannot guarantee idempotence."); } - configs.put(ACKS_CONFIG, "-1"); + + boolean userConfiguredInflightRequests = originalConfigs.containsKey(MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION); + if (userConfiguredInflightRequests && MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION_FOR_IDEMPOTENCE < this.getInt(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION)) { + throw new ConfigException("Must set " + ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION + " to at most 5" + + " to use the idempotent producer."); + } } } @@ -514,13 +512,12 @@ public ProducerConfig(Map props) { } boolean idempotenceEnabled() { - boolean userConfiguredIdempotence = this.originals().containsKey(ENABLE_IDEMPOTENCE_CONFIG); boolean userConfiguredTransactions = this.originals().containsKey(TRANSACTIONAL_ID_CONFIG); - boolean idempotenceEnabled = userConfiguredIdempotence && this.getBoolean(ENABLE_IDEMPOTENCE_CONFIG); - - if (!idempotenceEnabled && userConfiguredIdempotence && userConfiguredTransactions) + boolean idempotenceEnabled = this.getBoolean(ENABLE_IDEMPOTENCE_CONFIG); + if (!idempotenceEnabled && userConfiguredTransactions) throw new ConfigException("Cannot set a " + ProducerConfig.TRANSACTIONAL_ID_CONFIG + " without also enabling idempotence."); - return userConfiguredTransactions || idempotenceEnabled; + + return idempotenceEnabled; } ProducerConfig(Map props, boolean doLog) { diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java index 7c521fde68d9a..b2ec2e4e7d7f7 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java @@ -74,6 +74,8 @@ import org.apache.kafka.test.MockSerializer; import org.apache.kafka.test.TestUtils; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import javax.management.MBeanServer; import javax.management.ObjectName; @@ -124,8 +126,7 @@ public class KafkaProducerTest { private final String topic = "topic"; - private final Node host1 = new Node(0, "host1", 1000); - private final Collection nodes = Collections.singletonList(host1); + private final Collection nodes = Collections.singletonList(NODE); private final Cluster emptyCluster = new Cluster( null, nodes, @@ -148,6 +149,7 @@ public class KafkaProducerTest { Collections.emptySet(), Collections.emptySet()); private static final int DEFAULT_METADATA_IDLE_MS = 5 * 60 * 1000; + private static final Node NODE = new Node(0, "host1", 1000); private static KafkaProducer kafkaProducer(Map configs, @@ -242,6 +244,7 @@ public void testAcksAndIdempotenceForIdempotentProducers() { Properties invalidProps2 = new Properties() {{ putAll(baseProps); setProperty(ProducerConfig.ACKS_CONFIG, "1"); + // explicitly enabling idempotence should still throw exception setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "true"); }}; assertThrows( @@ -252,12 +255,149 @@ public void testAcksAndIdempotenceForIdempotentProducers() { Properties invalidProps3 = new Properties() {{ putAll(baseProps); setProperty(ProducerConfig.ACKS_CONFIG, "0"); - setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "true"); }}; assertThrows( ConfigException.class, () -> new ProducerConfig(invalidProps3), "Must set acks to all in order to use the idempotent producer"); + + Properties invalidProps4 = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.ACKS_CONFIG, "0"); + setProperty(ProducerConfig.TRANSACTIONAL_ID_CONFIG, "transactionalId"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps4), + "Must set retries to non-zero when using the idempotent producer."); + } + + @Test + public void testRetriesAndIdempotenceForIdempotentProducers() { + Properties baseProps = new Properties() {{ + setProperty(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); + setProperty(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); + setProperty(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); + }}; + + Properties validProps = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.RETRIES_CONFIG, "0"); + setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false"); + }}; + ProducerConfig config = new ProducerConfig(validProps); + assertFalse( + config.getBoolean(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG), + "idempotence should be overwritten"); + assertEquals( + 0, + config.getInt(ProducerConfig.RETRIES_CONFIG), + "retries should be overwritten"); + + Properties invalidProps = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.RETRIES_CONFIG, "0"); + setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false"); + setProperty(ProducerConfig.TRANSACTIONAL_ID_CONFIG, "transactionalId"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps), + "Cannot set a transactional.id without also enabling idempotence"); + + Properties invalidProps2 = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.RETRIES_CONFIG, "0"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps2), + "Must set retries to non-zero when using the idempotent producer."); + + Properties invalidProps3 = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.RETRIES_CONFIG, "0"); + // explicitly enabling idempotence should still throw exception + setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "true"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps3), + "Must set retries to non-zero when using the idempotent producer."); + + Properties invalidProps4 = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.RETRIES_CONFIG, "0"); + setProperty(ProducerConfig.TRANSACTIONAL_ID_CONFIG, "transactionalId"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps4), + "Must set retries to non-zero when using the idempotent producer."); + } + + @Test + public void testInflightRequestsAndIdempotenceForIdempotentProducers() { + Properties baseProps = new Properties() {{ + setProperty(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); + setProperty(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); + setProperty(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); + }}; + + Properties validProps = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION, "10"); + setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false"); + }}; + ProducerConfig config = new ProducerConfig(validProps); + assertFalse( + config.getBoolean(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG), + "idempotence should be overwritten"); + assertEquals( + 10, + config.getInt(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION), + "max.in.flight.requests.per.connection should be overwritten"); + + Properties invalidProps = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION, "5"); + setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false"); + setProperty(ProducerConfig.TRANSACTIONAL_ID_CONFIG, "transactionalId"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps), + "Cannot set a transactional.id without also enabling idempotence"); + + Properties invalidProps2 = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION, "10"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps2), + "Must set max.in.flight.requests.per.connection to at most 5 when using the idempotent producer."); + + Properties invalidProps3 = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION, "10"); + // explicitly enabling idempotence should still throw exception + setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "true"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps3), + "Must set max.in.flight.requests.per.connection to at most 5 when using the idempotent producer."); + + Properties invalidProps4 = new Properties() {{ + putAll(baseProps); + setProperty(ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION, "10"); + setProperty(ProducerConfig.TRANSACTIONAL_ID_CONFIG, "transactionalId"); + }}; + assertThrows( + ConfigException.class, + () -> new ProducerConfig(invalidProps4), + "Must set retries to non-zero when using the idempotent producer."); } @Test @@ -469,9 +609,17 @@ private static KafkaProducer producerWithOverrideNewSender(Map producerWithOverrideNewSender(Map configs, ProducerMetadata metadata, Time timer) { + // let mockClient#leastLoadedNode return the node directly so that we can isolate Metadata calls from KafkaProducer for idempotent producer + MockClient mockClient = new MockClient(Time.SYSTEM, metadata) { + @Override + public Node leastLoadedNode(long now) { + return NODE; + } + }; + return new KafkaProducer( new ProducerConfig(ProducerConfig.appendSerializerToConfig(configs, new StringSerializer(), new StringSerializer())), - new StringSerializer(), new StringSerializer(), metadata, new MockClient(Time.SYSTEM, metadata), null, timer) { + new StringSerializer(), new StringSerializer(), metadata, mockClient, null, timer) { @Override Sender newSender(LogContext logContext, KafkaClient kafkaClient, ProducerMetadata metadata) { // give Sender its own Metadata instance so that we can isolate Metadata calls from KafkaProducer @@ -480,10 +628,13 @@ Sender newSender(LogContext logContext, KafkaClient kafkaClient, ProducerMetadat }; } - @Test - public void testMetadataFetch() throws InterruptedException { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testMetadataFetch(boolean isIdempotenceEnabled) throws InterruptedException { Map configs = new HashMap<>(); configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); + configs.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, isIdempotenceEnabled); + ProducerMetadata metadata = mock(ProducerMetadata.class); // Return empty cluster 4 times and cluster from then on @@ -513,18 +664,14 @@ public void testMetadataFetch() throws InterruptedException { producer.close(Duration.ofMillis(0)); } - @Test - public void testMetadataExpiry() throws InterruptedException { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testMetadataExpiry(boolean isIdempotenceEnabled) throws InterruptedException { Map configs = new HashMap<>(); configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); + configs.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, isIdempotenceEnabled); ProducerMetadata metadata = mock(ProducerMetadata.class); - Cluster emptyCluster = new Cluster( - "dummy", - Collections.singletonList(host1), - Collections.emptySet(), - Collections.emptySet(), - Collections.emptySet()); when(metadata.fetch()).thenReturn(onePartitionCluster, emptyCluster, onePartitionCluster); KafkaProducer producer = producerWithOverrideNewSender(configs, metadata); @@ -545,11 +692,13 @@ public void testMetadataExpiry() throws InterruptedException { producer.close(Duration.ofMillis(0)); } - @Test - public void testMetadataTimeoutWithMissingTopic() throws Exception { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testMetadataTimeoutWithMissingTopic(boolean isIdempotenceEnabled) throws Exception { Map configs = new HashMap<>(); configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); configs.put(ProducerConfig.MAX_BLOCK_MS_CONFIG, 60000); + configs.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, isIdempotenceEnabled); // Create a record with a partition higher than the initial (outdated) partition range ProducerRecord record = new ProducerRecord<>(topic, 2, null, "value"); @@ -570,6 +719,7 @@ public void testMetadataTimeoutWithMissingTopic() throws Exception { // Four request updates where the topic isn't present, at which point the timeout expires and a // TimeoutException is thrown + // For idempotence enabled case, the first metadata.fetch will be called in Sender#maybeSendAndPollTransactionalRequest Future future = producer.send(record); verify(metadata, times(4)).requestUpdateForTopic(topic); verify(metadata, times(4)).awaitUpdate(anyInt(), anyLong()); @@ -583,11 +733,13 @@ public void testMetadataTimeoutWithMissingTopic() throws Exception { } } - @Test - public void testMetadataWithPartitionOutOfRange() throws Exception { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testMetadataWithPartitionOutOfRange(boolean isIdempotenceEnabled) throws Exception { Map configs = new HashMap<>(); configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); configs.put(ProducerConfig.MAX_BLOCK_MS_CONFIG, 60000); + configs.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, isIdempotenceEnabled); // Create a record with a partition higher than the initial (outdated) partition range ProducerRecord record = new ProducerRecord<>(topic, 2, null, "value"); @@ -607,11 +759,13 @@ public void testMetadataWithPartitionOutOfRange() throws Exception { producer.close(Duration.ofMillis(0)); } - @Test - public void testMetadataTimeoutWithPartitionOutOfRange() throws Exception { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testMetadataTimeoutWithPartitionOutOfRange(boolean isIdempotenceEnabled) throws Exception { Map configs = new HashMap<>(); configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); configs.put(ProducerConfig.MAX_BLOCK_MS_CONFIG, 60000); + configs.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, isIdempotenceEnabled); // Create a record with a partition higher than the initial (outdated) partition range ProducerRecord record = new ProducerRecord<>(topic, 2, null, "value"); @@ -632,7 +786,10 @@ public void testMetadataTimeoutWithPartitionOutOfRange() throws Exception { // Four request updates where the requested partition is out of range, at which point the timeout expires // and a TimeoutException is thrown + // For idempotence enabled case, the first and last metadata.fetch will be called in Sender#maybeSendAndPollTransactionalRequest, + // before the producer#send and after it finished Future future = producer.send(record); + verify(metadata, times(4)).requestUpdateForTopic(topic); verify(metadata, times(4)).awaitUpdate(anyInt(), anyLong()); verify(metadata, times(5)).fetch(); @@ -650,6 +807,8 @@ public void testTopicRefreshInMetadata() throws InterruptedException { Map configs = new HashMap<>(); configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); configs.put(ProducerConfig.MAX_BLOCK_MS_CONFIG, "600000"); + // test under normal producer for simplicity + configs.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, false); long refreshBackoffMs = 500L; long metadataExpireMs = 60000L; long metadataIdleMs = 60000L; @@ -797,6 +956,9 @@ public void closeWithNegativeTimestampShouldThrow() { public void testFlushCompleteSendOfInflightBatches() { Map configs = new HashMap<>(); configs.put(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9000"); + // only test in idempotence disabled producer for simplicity + // flush operation acts the same for idempotence enabled and disabled cases + configs.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, false); Time time = new MockTime(1); MetadataResponse initialUpdateResponse = RequestTestUtils.metadataUpdateWith(1, singletonMap("topic", 1)); @@ -812,6 +974,7 @@ public void testFlushCompleteSendOfInflightBatches() { Future response = producer.send(new ProducerRecord<>("topic", "value" + i)); futureResponses.add(response); } + futureResponses.forEach(res -> assertFalse(res.isDone())); producer.flush(); futureResponses.forEach(res -> assertTrue(res.isDone())); @@ -929,7 +1092,7 @@ public void testInitTransactionsResponseAfterTimeout() throws Exception { client.prepareResponse( request -> request instanceof FindCoordinatorRequest && ((FindCoordinatorRequest) request).data().keyType() == FindCoordinatorRequest.CoordinatorType.TRANSACTION.id(), - FindCoordinatorResponse.prepareResponse(Errors.NONE, "bad-transaction", host1)); + FindCoordinatorResponse.prepareResponse(Errors.NONE, "bad-transaction", NODE)); Future future = executor.submit(producer::initTransactions); TestUtils.waitForCondition(client::hasInFlightRequests, @@ -966,14 +1129,14 @@ public void testInitTransactionTimeout() { client.prepareResponse( request -> request instanceof FindCoordinatorRequest && ((FindCoordinatorRequest) request).data().keyType() == FindCoordinatorRequest.CoordinatorType.TRANSACTION.id(), - FindCoordinatorResponse.prepareResponse(Errors.NONE, "bad-transaction", host1)); + FindCoordinatorResponse.prepareResponse(Errors.NONE, "bad-transaction", NODE)); assertThrows(TimeoutException.class, producer::initTransactions); client.prepareResponse( request -> request instanceof FindCoordinatorRequest && ((FindCoordinatorRequest) request).data().keyType() == FindCoordinatorRequest.CoordinatorType.TRANSACTION.id(), - FindCoordinatorResponse.prepareResponse(Errors.NONE, "bad-transaction", host1)); + FindCoordinatorResponse.prepareResponse(Errors.NONE, "bad-transaction", NODE)); client.prepareResponse(initProducerIdResponse(1L, (short) 5, Errors.NONE)); @@ -999,7 +1162,7 @@ public void testInitTransactionWhileThrottled() { Node node = metadata.fetch().nodes().get(0); client.throttle(node, 5000); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); client.prepareResponse(initProducerIdResponse(1L, (short) 5, Errors.NONE)); try (Producer producer = kafkaProducer(configs, new StringSerializer(), @@ -1021,7 +1184,7 @@ public void testAbortTransaction() { MockClient client = new MockClient(time, metadata); client.updateMetadata(initialUpdateResponse); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); client.prepareResponse(initProducerIdResponse(1L, (short) 5, Errors.NONE)); client.prepareResponse(endTxnResponse(Errors.NONE)); @@ -1043,7 +1206,7 @@ public void testMeasureAbortTransactionDuration() { ProducerMetadata metadata = newMetadata(0, Long.MAX_VALUE); MockClient client = new MockClient(time, metadata); client.updateMetadata(initialUpdateResponse); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); client.prepareResponse(initProducerIdResponse(1L, (short) 5, Errors.NONE)); try (KafkaProducer producer = kafkaProducer(configs, new StringSerializer(), @@ -1080,10 +1243,10 @@ public void testSendTxnOffsetsWithGroupId() { Node node = metadata.fetch().nodes().get(0); client.throttle(node, 5000); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); client.prepareResponse(initProducerIdResponse(1L, (short) 5, Errors.NONE)); client.prepareResponse(addOffsetsToTxnResponse(Errors.NONE)); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); String groupId = "group"; client.prepareResponse(request -> ((TxnOffsetCommitRequest) request).data().groupId().equals(groupId), @@ -1123,7 +1286,7 @@ public void testMeasureTransactionDurations() { MockClient client = new MockClient(time, metadata); client.updateMetadata(initialUpdateResponse); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); client.prepareResponse(initProducerIdResponse(1L, (short) 5, Errors.NONE)); try (KafkaProducer producer = kafkaProducer(configs, new StringSerializer(), @@ -1132,7 +1295,7 @@ public void testMeasureTransactionDurations() { assertDurationAtLeast(producer, "txn-init-time-ns-total", tick.toNanos()); client.prepareResponse(addOffsetsToTxnResponse(Errors.NONE)); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); client.prepareResponse(txnOffsetsCommitResponse(Collections.singletonMap( new TopicPartition("topic", 0), Errors.NONE))); client.prepareResponse(endTxnResponse(Errors.NONE)); @@ -1181,10 +1344,10 @@ public void testSendTxnOffsetsWithGroupMetadata() { Node node = metadata.fetch().nodes().get(0); client.throttle(node, 5000); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); client.prepareResponse(initProducerIdResponse(1L, (short) 5, Errors.NONE)); client.prepareResponse(addOffsetsToTxnResponse(Errors.NONE)); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); String groupId = "group"; String memberId = "member"; int generationId = 5; @@ -1237,7 +1400,7 @@ private void verifyInvalidGroupMetadata(ConsumerGroupMetadata groupMetadata) { Node node = metadata.fetch().nodes().get(0); client.throttle(node, 5000); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "some.id", NODE)); client.prepareResponse(initProducerIdResponse(1L, (short) 5, Errors.NONE)); try (Producer producer = kafkaProducer(configs, new StringSerializer(), @@ -1448,7 +1611,7 @@ public void testCloseIsForcedOnPendingInitProducerId() throws InterruptedExcepti ExecutorService executorService = Executors.newSingleThreadExecutor(); CountDownLatch assertionDoneLatch = new CountDownLatch(1); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "this-is-a-transactional-id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "this-is-a-transactional-id", NODE)); executorService.submit(() -> { assertThrows(KafkaException.class, producer::initTransactions); assertionDoneLatch.countDown(); @@ -1477,7 +1640,7 @@ public void testCloseIsForcedOnPendingAddOffsetRequest() throws InterruptedExcep ExecutorService executorService = Executors.newSingleThreadExecutor(); CountDownLatch assertionDoneLatch = new CountDownLatch(1); - client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "this-is-a-transactional-id", host1)); + client.prepareResponse(FindCoordinatorResponse.prepareResponse(Errors.NONE, "this-is-a-transactional-id", NODE)); executorService.submit(() -> { assertThrows(KafkaException.class, producer::initTransactions); assertionDoneLatch.countDown(); diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaStatusBackingStore.java b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaStatusBackingStore.java index 44902c0df1abc..eb28102e1001c 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaStatusBackingStore.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaStatusBackingStore.java @@ -170,6 +170,7 @@ public void configure(final WorkerConfig config) { producerProps.put(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); producerProps.put(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, ByteArraySerializer.class.getName()); producerProps.put(ProducerConfig.RETRIES_CONFIG, 0); // we handle retries in this class + producerProps.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, false); // disable idempotence since retries is force to 0 ConnectUtils.addMetricsContextProperties(producerProps, config, clusterId); Map consumerProps = new HashMap<>(originals); diff --git a/core/src/main/scala/kafka/tools/ConsoleProducer.scala b/core/src/main/scala/kafka/tools/ConsoleProducer.scala index 7c221baf69a2b..4a6f7315eeeb1 100644 --- a/core/src/main/scala/kafka/tools/ConsoleProducer.scala +++ b/core/src/main/scala/kafka/tools/ConsoleProducer.scala @@ -164,7 +164,7 @@ object ConsoleProducer { .withRequiredArg .describedAs("request required acks") .ofType(classOf[java.lang.String]) - .defaultsTo("1") + .defaultsTo("-1") val requestTimeoutMsOpt = parser.accepts("request-timeout-ms", "The ack timeout of the producer requests. Value must be non-negative and non-zero") .withRequiredArg .describedAs("request timeout ms") diff --git a/core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala b/core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala index 1da4fcd587edd..0b215ff7200bb 100644 --- a/core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala +++ b/core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala @@ -143,6 +143,7 @@ class AuthorizerIntegrationTest extends BaseRequestTest { val adminClients = Buffer[Admin]() producerConfig.setProperty(ProducerConfig.ACKS_CONFIG, "1") + producerConfig.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false") producerConfig.setProperty(ProducerConfig.MAX_BLOCK_MS_CONFIG, "50000") consumerConfig.setProperty(ConsumerConfig.GROUP_ID_CONFIG, group) @@ -2383,6 +2384,7 @@ class AuthorizerIntegrationTest extends BaseRequestTest { private def buildTransactionalProducer(): KafkaProducer[Array[Byte], Array[Byte]] = { producerConfig.setProperty(ProducerConfig.TRANSACTIONAL_ID_CONFIG, transactionalId) + producerConfig.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "true") producerConfig.setProperty(ProducerConfig.ACKS_CONFIG, "all") createProducer() } diff --git a/core/src/test/scala/integration/kafka/api/ConsumerWithLegacyMessageFormatIntegrationTest.scala b/core/src/test/scala/integration/kafka/api/ConsumerWithLegacyMessageFormatIntegrationTest.scala index e8c451ec3aba2..0ce4004c64e2b 100644 --- a/core/src/test/scala/integration/kafka/api/ConsumerWithLegacyMessageFormatIntegrationTest.scala +++ b/core/src/test/scala/integration/kafka/api/ConsumerWithLegacyMessageFormatIntegrationTest.scala @@ -18,6 +18,7 @@ package kafka.api import kafka.log.LogConfig import kafka.server.KafkaConfig +import org.apache.kafka.clients.producer.ProducerConfig import org.apache.kafka.common.TopicPartition import org.junit.jupiter.api.Assertions.{assertEquals, assertNull, assertThrows} import org.junit.jupiter.api.Test @@ -101,7 +102,10 @@ class ConsumerWithLegacyMessageFormatIntegrationTest extends AbstractConsumerTes def testEarliestOrLatestOffsets(): Unit = { val topic0 = "topicWithNewMessageFormat" val topic1 = "topicWithOldMessageFormat" - val producer = createProducer() + val prop = new Properties() + // idempotence producer doesn't support old version of messages + prop.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false") + val producer = createProducer(configOverrides = prop) createTopicAndSendRecords(producer, topicName = topic0, numPartitions = 2, recordsPerPartition = 100) val props = new Properties() props.setProperty(LogConfig.MessageFormatVersionProp, "0.9.0") diff --git a/core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala b/core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala index cbb536b37b3ad..fb8431253f087 100644 --- a/core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala +++ b/core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala @@ -20,7 +20,7 @@ package kafka.api import com.yammer.metrics.core.Gauge import java.io.File -import java.util.Collections +import java.util.{Collections, Properties} import java.util.concurrent.ExecutionException import kafka.admin.AclCommand import kafka.metrics.KafkaYammerMetrics @@ -30,7 +30,7 @@ import kafka.server._ import kafka.utils._ import org.apache.kafka.clients.admin.Admin import org.apache.kafka.clients.consumer.{Consumer, ConsumerConfig, ConsumerRecords} -import org.apache.kafka.clients.producer.{KafkaProducer, ProducerRecord} +import org.apache.kafka.clients.producer.{KafkaProducer, ProducerConfig, ProducerRecord} import org.apache.kafka.common.acl._ import org.apache.kafka.common.acl.AclOperation._ import org.apache.kafka.common.acl.AclPermissionType._ @@ -42,6 +42,8 @@ import org.apache.kafka.common.resource.PatternType.{LITERAL, PREFIXED} import org.apache.kafka.common.security.auth.KafkaPrincipal import org.junit.jupiter.api.Assertions._ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test, TestInfo} +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource import scala.jdk.CollectionConverters._ @@ -334,13 +336,18 @@ abstract class EndToEndAuthorizationTest extends IntegrationTestHarness with Sas * messages and describe topics respectively when the describe ACL isn't set. * Also verifies that subsequent publish, consume and describe to authorized topic succeeds. */ - @Test - def testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl(): Unit = { + @ParameterizedTest + @ValueSource(booleans = Array(true, false)) + def testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl(isIdempotenceEnabled: Boolean): Unit = { // Set consumer group acls since we are testing topic authorization setConsumerGroupAcls() // Verify produce/consume/describe throw TopicAuthorizationException - val producer = createProducer() + + val prop = new Properties() + prop.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, isIdempotenceEnabled.toString) + val producer = createProducer(configOverrides = prop) + assertThrows(classOf[TopicAuthorizationException], () => sendRecords(producer, numRecords, tp)) val consumer = createConsumer() consumer.assign(List(tp).asJava) @@ -352,12 +359,21 @@ abstract class EndToEndAuthorizationTest extends IntegrationTestHarness with Sas // Verify successful produce/consume/describe on another topic using the same producer, consumer and adminClient val topic2 = "topic2" val tp2 = new TopicPartition(topic2, 0) + setReadAndWriteAcls(tp2) - sendRecords(producer, numRecords, tp2) + // in idempotence producer, we need to create another producer because the previous one is in FATAL_ERROR state (due to authorization error) + // If the transaction state in FATAL_ERROR, it'll never transit to other state. check TransactionManager#isTransitionValid for detail + val producer2 = if (isIdempotenceEnabled) + createProducer(configOverrides = prop) + else + producer + + sendRecords(producer2, numRecords, tp2) consumer.assign(List(tp2).asJava) consumeRecords(consumer, numRecords, topic = topic2) val describeResults = adminClient.describeTopics(Set(topic, topic2).asJava).topicNameValues() assertEquals(1, describeResults.get(topic2).get().partitions().size()) + val e2 = assertThrows(classOf[ExecutionException], () => adminClient.describeTopics(Set(topic).asJava).allTopicNames().get()) assertTrue(e2.getCause.isInstanceOf[TopicAuthorizationException], "Unexpected exception " + e2.getCause) @@ -365,7 +381,7 @@ abstract class EndToEndAuthorizationTest extends IntegrationTestHarness with Sas // from the unauthorized topic and throw; since we can now return data during the time we are updating // metadata / fetching positions, it is possible that the authorized topic record is returned during this time. consumer.assign(List(tp, tp2).asJava) - sendRecords(producer, numRecords, tp2) + sendRecords(producer2, numRecords, tp2) var topic2RecordConsumed = false def verifyNoRecords(records: ConsumerRecords[Array[Byte], Array[Byte]]): Boolean = { assertEquals(Collections.singleton(tp2), records.partitions(), "Consumed records with unexpected partitions: " + records) @@ -380,22 +396,32 @@ abstract class EndToEndAuthorizationTest extends IntegrationTestHarness with Sas if (!topic2RecordConsumed) { consumeRecordsIgnoreOneAuthorizationException(consumer, numRecords, startingOffset = 1, topic2) } - sendRecords(producer, numRecords, tp) + sendRecords(producer2, numRecords, tp) consumeRecordsIgnoreOneAuthorizationException(consumer, numRecords, startingOffset = 0, topic) val describeResults2 = adminClient.describeTopics(Set(topic, topic2).asJava).topicNameValues assertEquals(1, describeResults2.get(topic).get().partitions().size()) assertEquals(1, describeResults2.get(topic2).get().partitions().size()) } - @Test - def testNoProduceWithDescribeAcl(): Unit = { + @ParameterizedTest + @ValueSource(booleans = Array(true, false)) + def testNoProduceWithDescribeAcl(isIdempotenceEnabled: Boolean): Unit = { AclCommand.main(describeAclArgs) servers.foreach { s => TestUtils.waitAndVerifyAcls(TopicDescribeAcl, s.dataPlaneRequestProcessor.authorizer.get, topicResource) } - val producer = createProducer() - val e = assertThrows(classOf[TopicAuthorizationException], () => sendRecords(producer, numRecords, tp)) - assertEquals(Set(topic).asJava, e.unauthorizedTopics()) + + val prop = new Properties() + prop.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, isIdempotenceEnabled.toString) + val producer = createProducer(configOverrides = prop) + + if (isIdempotenceEnabled) { + // in idempotent producer, it'll fail at InitProducerId request + assertThrows(classOf[KafkaException], () => sendRecords(producer, numRecords, tp)) + } else { + val e = assertThrows(classOf[TopicAuthorizationException], () => sendRecords(producer, numRecords, tp)) + assertEquals(Set(topic).asJava, e.unauthorizedTopics()) + } confirmReauthenticationMetrics() } diff --git a/core/src/test/scala/integration/kafka/api/MetricsTest.scala b/core/src/test/scala/integration/kafka/api/MetricsTest.scala index 850ac89365af2..151bd0dac3358 100644 --- a/core/src/test/scala/integration/kafka/api/MetricsTest.scala +++ b/core/src/test/scala/integration/kafka/api/MetricsTest.scala @@ -82,7 +82,10 @@ class MetricsTest extends IntegrationTestHarness with SaslSetup { // Produce and consume some records val numRecords = 10 val recordSize = 100000 - val producer = createProducer() + val prop = new Properties() + // idempotence producer doesn't support old version of messages + prop.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false") + val producer = createProducer(configOverrides = prop) sendRecords(producer, numRecords, recordSize, tp) val consumer = createConsumer() diff --git a/core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala b/core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala index a9f2c6c4535b8..c9be59e6b41fc 100644 --- a/core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala +++ b/core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala @@ -14,9 +14,8 @@ package kafka.api import java.nio.file.Files import java.time.Duration -import java.util.Collections +import java.util.{Collections, Properties} import java.util.concurrent.{ExecutionException, TimeUnit} - import scala.jdk.CollectionConverters._ import org.apache.kafka.clients.admin.{Admin, AdminClientConfig} import org.apache.kafka.clients.consumer.{ConsumerConfig, KafkaConsumer} @@ -30,6 +29,8 @@ import kafka.server.KafkaConfig import kafka.utils.{JaasTestUtils, TestUtils} import kafka.zk.ConfigEntityChangeNotificationZNode import org.apache.kafka.common.security.auth.SecurityProtocol +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource class SaslClientsWithInvalidCredentialsTest extends IntegrationTestHarness with SaslSetup { private val kafkaClientSaslMechanism = "SCRAM-SHA-256" @@ -76,14 +77,24 @@ class SaslClientsWithInvalidCredentialsTest extends IntegrationTestHarness with closeSasl() } - @Test - def testProducerWithAuthenticationFailure(): Unit = { - val producer = createProducer() + @ParameterizedTest + @ValueSource(booleans = Array(true, false)) + def testProducerWithAuthenticationFailure(isIdempotenceEnabled: Boolean): Unit = { + val prop = new Properties() + prop.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, isIdempotenceEnabled.toString) + val producer = createProducer(configOverrides = prop) + verifyAuthenticationException(sendOneRecord(producer, maxWaitMs = 10000)) verifyAuthenticationException(producer.partitionsFor(topic)) createClientCredential() - verifyWithRetry(sendOneRecord(producer)) + // in idempotence producer, we need to create another producer because the previous one is in FATEL_ERROR state (due to authentication error) + // If the transaction state in FATAL_ERROR, it'll never transit to other state. check TransactionManager#isTransitionValid for detail + val producer2 = if (isIdempotenceEnabled) + createProducer(configOverrides = prop) + else + producer + verifyWithRetry(sendOneRecord(producer2)) } @Test diff --git a/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala b/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala index 687cd9e8b5685..9502d2ef24c85 100644 --- a/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala +++ b/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala @@ -1670,6 +1670,8 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup producerProps.put(ProducerConfig.RETRIES_CONFIG, _retries.toString) producerProps.put(ProducerConfig.DELIVERY_TIMEOUT_MS_CONFIG, _deliveryTimeoutMs.toString) producerProps.put(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG, _requestTimeoutMs.toString) + // disable the idempotence since some tests want to test the cases when retries=0, and these tests are not testing producers + producerProps.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false") val producer = new KafkaProducer[String, String](producerProps, new StringSerializer, new StringSerializer) producers += producer diff --git a/core/src/test/scala/unit/kafka/server/LogDirFailureTest.scala b/core/src/test/scala/unit/kafka/server/LogDirFailureTest.scala index 1025d7ae2e3bd..bfbb14e1aaae7 100644 --- a/core/src/test/scala/unit/kafka/server/LogDirFailureTest.scala +++ b/core/src/test/scala/unit/kafka/server/LogDirFailureTest.scala @@ -109,6 +109,7 @@ class LogDirFailureTest extends IntegrationTestHarness { @Test def testReplicaFetcherThreadAfterLogDirFailureOnFollower(): Unit = { this.producerConfig.setProperty(ProducerConfig.RETRIES_CONFIG, "0") + this.producerConfig.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false") val producer = createProducer() val partition = new TopicPartition(topic, 0) @@ -140,6 +141,7 @@ class LogDirFailureTest extends IntegrationTestHarness { def testProduceErrorsFromLogDirFailureOnLeader(failureType: LogDirFailureType): Unit = { // Disable retries to allow exception to bubble up for validation this.producerConfig.setProperty(ProducerConfig.RETRIES_CONFIG, "0") + this.producerConfig.setProperty(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, "false") val producer = createProducer() val partition = new TopicPartition(topic, 0) diff --git a/docs/upgrade.html b/docs/upgrade.html index 73ebe2970a7da..71ded0cd22281 100644 --- a/docs/upgrade.html +++ b/docs/upgrade.html @@ -19,6 +19,14 @@