kafka-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From j...@apache.org
Subject kafka git commit: KAFKA-5322; Add `OPERATION_NOT_ATTEMPTED` error code to resolve AddPartitionsToTxn inconsistency
Date Fri, 02 Jun 2017 21:23:37 GMT
Repository: kafka
Updated Branches:
  refs/heads/0.11.0 bf8378a84 -> 27a8b4fe5


KAFKA-5322; Add `OPERATION_NOT_ATTEMPTED` error code to resolve AddPartitionsToTxn inconsistency

In the `AddPartitionsToTxn` request handling, if even one partition fails authorization checks,
the entire request is essentially failed. However, the `AddPartitionsToTxnResponse` today
will only contain the error codes for the topics which failed authorization. It will have
no error code for the topics which succeeded, making it inconsistent with other APIs.

This patch adds a new error code `OPERATION_NOT_ATTEMPTED` which is returned for the successful
partitions to indicate that they were not added to the transaction.

Author: Apurva Mehta <apurva@confluent.io>

Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>

Closes #3204 from apurvam/KAFKA-5322-add-operation-not-attempted-for-add-partitions

(cherry picked from commit a318b1512900d29dc9ff0082f52f467999ba69b8)
Signed-off-by: Jason Gustafson <jason@confluent.io>


Project: http://git-wip-us.apache.org/repos/asf/kafka/repo
Commit: http://git-wip-us.apache.org/repos/asf/kafka/commit/27a8b4fe
Tree: http://git-wip-us.apache.org/repos/asf/kafka/tree/27a8b4fe
Diff: http://git-wip-us.apache.org/repos/asf/kafka/diff/27a8b4fe

Branch: refs/heads/0.11.0
Commit: 27a8b4fe540ac755986f8ea5fff9f496b5e6254e
Parents: bf8378a
Author: Apurva Mehta <apurva@confluent.io>
Authored: Fri Jun 2 14:18:54 2017 -0700
Committer: Jason Gustafson <jason@confluent.io>
Committed: Fri Jun 2 14:23:34 2017 -0700

----------------------------------------------------------------------
 .../producer/internals/TransactionManager.java  |  3 +
 .../errors/OperationNotAttemptedException.java  | 27 +++++++
 .../apache/kafka/common/protocol/Errors.java    | 13 +++-
 .../internals/TransactionManagerTest.java       | 26 ++++---
 .../src/main/scala/kafka/server/KafkaApis.scala | 11 +--
 .../server/AddPartitionsToTxnRequestTest.scala  | 76 ++++++++++++++++++++
 6 files changed, 141 insertions(+), 15 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kafka/blob/27a8b4fe/clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java
----------------------------------------------------------------------
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 0a69e02..aade83e 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
@@ -734,6 +734,9 @@ public class TransactionManager {
                     return;
                 } else if (error == Errors.TOPIC_AUTHORIZATION_FAILED) {
                     unauthorizedTopics.add(topicPartition.topic());
+                } else if (error == Errors.OPERATION_NOT_ATTEMPTED) {
+                    log.debug("{}Did not attempt to add partition {} to transaction because
other partitions in the batch had errors.", logPrefix, topicPartition);
+                    hasPartitionErrors = true;
                 } else {
                     log.error("{}Could not add partition {} due to unexpected error {}",
logPrefix, topicPartition, error);
                     hasPartitionErrors = true;

http://git-wip-us.apache.org/repos/asf/kafka/blob/27a8b4fe/clients/src/main/java/org/apache/kafka/common/errors/OperationNotAttemptedException.java
----------------------------------------------------------------------
diff --git a/clients/src/main/java/org/apache/kafka/common/errors/OperationNotAttemptedException.java
b/clients/src/main/java/org/apache/kafka/common/errors/OperationNotAttemptedException.java
new file mode 100644
index 0000000..96df321
--- /dev/null
+++ b/clients/src/main/java/org/apache/kafka/common/errors/OperationNotAttemptedException.java
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.errors;
+
+/**
+ * Indicates that the broker did not attempt to execute this operation. This may happen for
batched RPCs where some
+ * operations in the batch failed, causing the broker to respond without trying the rest.
+ */
+public class OperationNotAttemptedException extends ApiException {
+    public OperationNotAttemptedException(final String message) {
+        super(message);
+    }
+}

http://git-wip-us.apache.org/repos/asf/kafka/blob/27a8b4fe/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
----------------------------------------------------------------------
diff --git a/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java b/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
index dd02ff0..ae8d161 100644
--- a/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
+++ b/clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
@@ -53,6 +53,7 @@ import org.apache.kafka.common.errors.NotEnoughReplicasException;
 import org.apache.kafka.common.errors.NotLeaderForPartitionException;
 import org.apache.kafka.common.errors.OffsetMetadataTooLarge;
 import org.apache.kafka.common.errors.OffsetOutOfRangeException;
+import org.apache.kafka.common.errors.OperationNotAttemptedException;
 import org.apache.kafka.common.errors.OutOfOrderSequenceException;
 import org.apache.kafka.common.errors.PolicyViolationException;
 import org.apache.kafka.common.errors.ProducerFencedException;
@@ -486,8 +487,16 @@ public enum Errors {
         public ApiException build(String message) {
             return new SecurityDisabledException(message);
         }
-    });
-             
+    }),
+    OPERATION_NOT_ATTEMPTED(55, "The broker did not attempt to execute this operation. This
may happen for batched RPCs " +
+            "where some operations in the batch failed, causing the broker to respond without
trying the rest.",
+        new ApiExceptionBuilder() {
+            @Override
+            public ApiException build(String message) {
+                return new OperationNotAttemptedException(message);
+            }
+        });
+
     private interface ApiExceptionBuilder {
         ApiException build(String message);
     }

http://git-wip-us.apache.org/repos/asf/kafka/blob/27a8b4fe/clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java
----------------------------------------------------------------------
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 96eae8f..6e633ec 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
@@ -427,21 +427,26 @@ public class TransactionManagerTest {
     public void testTopicAuthorizationFailureInAddPartitions() {
         final long pid = 13131L;
         final short epoch = 1;
-        final TopicPartition tp = new TopicPartition("foo", 0);
+        final TopicPartition tp0 = new TopicPartition("foo", 0);
+        final TopicPartition tp1 = new TopicPartition("bar", 0);
 
         doInitTransactions(pid, epoch);
 
         transactionManager.beginTransaction();
-        transactionManager.maybeAddPartitionToTransaction(tp);
+        transactionManager.maybeAddPartitionToTransaction(tp0);
+        transactionManager.maybeAddPartitionToTransaction(tp1);
+        Map<TopicPartition, Errors> errors = new HashMap<>();
+        errors.put(tp0, Errors.TOPIC_AUTHORIZATION_FAILED);
+        errors.put(tp1, Errors.OPERATION_NOT_ATTEMPTED);
 
-        prepareAddPartitionsToTxn(tp, Errors.TOPIC_AUTHORIZATION_FAILED);
+        prepareAddPartitionsToTxn(errors);
         sender.run(time.milliseconds());
 
         assertTrue(transactionManager.hasError());
         assertTrue(transactionManager.lastError() instanceof TopicAuthorizationException);
 
         TopicAuthorizationException exception = (TopicAuthorizationException) transactionManager.lastError();
-        assertEquals(singleton(tp.topic()), exception.unauthorizedTopics());
+        assertEquals(singleton(tp0.topic()), exception.unauthorizedTopics());
 
         assertAbortableError(TopicAuthorizationException.class);
     }
@@ -1009,14 +1014,19 @@ public class TransactionManagerTest {
         assertFalse(transactionManager.transactionContainsPartition(tp0));
     }
 
-    private void prepareAddPartitionsToTxn(final TopicPartition tp, final Errors error) {
+    private void prepareAddPartitionsToTxn(final Map<TopicPartition, Errors> errors)
{
         client.prepareResponse(new MockClient.RequestMatcher() {
             @Override
             public boolean matches(AbstractRequest body) {
-                return body instanceof AddPartitionsToTxnRequest &&
-                        ((AddPartitionsToTxnRequest) body).partitions().contains(tp);
+                AddPartitionsToTxnRequest request = (AddPartitionsToTxnRequest) body;
+                assertEquals(new HashSet<>(request.partitions()), new HashSet<>(errors.keySet()));
+                return true;
             }
-        }, new AddPartitionsToTxnResponse(0, singletonMap(tp, error)));
+        }, new AddPartitionsToTxnResponse(0, errors));
+    }
+
+    private void prepareAddPartitionsToTxn(final TopicPartition tp, final Errors error) {
+        prepareAddPartitionsToTxn(Collections.singletonMap(tp, error));
     }
 
     private void prepareFindCoordinatorResponse(Errors error, boolean shouldDisconnect,

http://git-wip-us.apache.org/repos/asf/kafka/blob/27a8b4fe/core/src/main/scala/kafka/server/KafkaApis.scala
----------------------------------------------------------------------
diff --git a/core/src/main/scala/kafka/server/KafkaApis.scala b/core/src/main/scala/kafka/server/KafkaApis.scala
index 5ce590f..1d289b3 100644
--- a/core/src/main/scala/kafka/server/KafkaApis.scala
+++ b/core/src/main/scala/kafka/server/KafkaApis.scala
@@ -1598,7 +1598,7 @@ class KafkaApis(val requestChannel: RequestChannel,
           authorize(request.session, Describe, new Resource(Topic, tp.topic)) &&
metadataCache.contains(tp)
         }
 
-      val unauthorizedForWriteRequestInfo = existingAndAuthorizedForDescribeTopics.filterNot
{ tp =>
+      val (authorizedPartitions, unauthorizedForWriteRequestInfo) = existingAndAuthorizedForDescribeTopics.partition
{ tp =>
         authorize(request.session, Write, new Resource(Topic, tp.topic))
       }
 
@@ -1606,12 +1606,13 @@ class KafkaApis(val requestChannel: RequestChannel,
         || unauthorizedForWriteRequestInfo.nonEmpty
         || internalTopics.nonEmpty) {
 
-        // Any failed partition check causes the entire request to fail. We only send back
error responses
-        // for the partitions that failed to avoid needing to send an ambiguous error code
for the partitions
-        // which succeeded.
+        // Any failed partition check causes the entire request to fail. We send the appropriate
error codes for the
+        // partitions which failed, and an 'OPERATION_NOT_ATTEMPTED' error code for the partitions
which succeeded
+        // the authorization check to indicate that they were not added to the transaction.
         val partitionErrors = (unauthorizedForWriteRequestInfo.map(_ -> Errors.TOPIC_AUTHORIZATION_FAILED)
++
           nonExistingOrUnauthorizedForDescribeTopics.map(_ -> Errors.UNKNOWN_TOPIC_OR_PARTITION)
++
-          internalTopics.map(_ ->Errors.TOPIC_AUTHORIZATION_FAILED)).toMap
+          internalTopics.map(_ -> Errors.TOPIC_AUTHORIZATION_FAILED) ++
+          authorizedPartitions.map(_ -> Errors.OPERATION_NOT_ATTEMPTED)).toMap
 
         sendResponseMaybeThrottle(request, requestThrottleMs =>
           new AddPartitionsToTxnResponse(requestThrottleMs, partitionErrors.asJava))

http://git-wip-us.apache.org/repos/asf/kafka/blob/27a8b4fe/core/src/test/scala/unit/kafka/server/AddPartitionsToTxnRequestTest.scala
----------------------------------------------------------------------
diff --git a/core/src/test/scala/unit/kafka/server/AddPartitionsToTxnRequestTest.scala b/core/src/test/scala/unit/kafka/server/AddPartitionsToTxnRequestTest.scala
new file mode 100644
index 0000000..62dd4c4
--- /dev/null
+++ b/core/src/test/scala/unit/kafka/server/AddPartitionsToTxnRequestTest.scala
@@ -0,0 +1,76 @@
+/**
+  * Licensed to the Apache Software Foundation (ASF) under one or more
+  * contributor license agreements.  See the NOTICE file distributed with
+  * this work for additional information regarding copyright ownership.
+  * The ASF licenses this file to You under the Apache License, Version 2.0
+  * (the "License"); you may not use this file except in compliance with
+  * the License.  You may obtain a copy of the License at
+  *
+  *    http://www.apache.org/licenses/LICENSE-2.0
+  *
+  * Unless required by applicable law or agreed to in writing, software
+  * distributed under the License is distributed on an "AS IS" BASIS,
+  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  * See the License for the specific language governing permissions and
+  * limitations under the License.
+  */
+
+package kafka.server
+
+import java.util.Properties
+
+import kafka.utils.TestUtils
+import org.apache.kafka.common.TopicPartition
+import org.apache.kafka.common.protocol.{ApiKeys, Errors}
+import org.apache.kafka.common.requests.{AddPartitionsToTxnRequest, AddPartitionsToTxnResponse}
+import org.junit.{Before, Test}
+import org.junit.Assert._
+
+import scala.collection.JavaConversions._
+
+class AddPartitionsToTxnRequestTest extends BaseRequestTest {
+  private val topic1 = "foobartopic"
+  val numPartitions = 3
+
+  override def propertyOverrides(properties: Properties): Unit =
+    properties.put(KafkaConfig.AutoCreateTopicsEnableProp, false.toString)
+
+  @Before
+  override def setUp(): Unit = {
+    super.setUp()
+    TestUtils.createTopic(zkUtils, topic1, numPartitions, servers.size, servers, new Properties())
+  }
+
+  @Test
+  def shouldReceiveOperationNotAttemptedWhenOtherPartitionHasError(): Unit = {
+    // The basic idea is that we have one unknown topic and one created topic. We should
get the 'UNKNOWN_TOPIC_OR_PARTITION'
+    // error for the unknown topic and the 'OPERATION_NOT_ATTEMPTED' error for the known
and authorized topic.
+    val nonExistentTopic = new TopicPartition("unknownTopic", 0)
+    val createdTopicPartition = new TopicPartition(topic1, 0)
+
+    val request = createRequest(List(createdTopicPartition, nonExistentTopic))
+    val leaderId = servers.head.config.brokerId
+    val response = sendAddPartitionsRequest(leaderId, request)
+
+    assertEquals(2, response.errors.size)
+
+    assertTrue(response.errors.containsKey(createdTopicPartition))
+    assertEquals(Errors.OPERATION_NOT_ATTEMPTED, response.errors.get(createdTopicPartition))
+
+    assertTrue(response.errors.containsKey(nonExistentTopic))
+    assertEquals(Errors.UNKNOWN_TOPIC_OR_PARTITION, response.errors.get(nonExistentTopic))
+  }
+
+  private def sendAddPartitionsRequest(leaderId: Int, request: AddPartitionsToTxnRequest)
: AddPartitionsToTxnResponse = {
+    val response = connectAndSend(request, ApiKeys.ADD_PARTITIONS_TO_TXN, destination = brokerSocketServer(leaderId))
+    AddPartitionsToTxnResponse.parse(response, request.version)
+  }
+
+  private def createRequest(partitions: List[TopicPartition]): AddPartitionsToTxnRequest
= {
+    val transactionalId = "foobar"
+    val producerId = 1000L
+    val producerEpoch: Short = 0
+    val builder = new AddPartitionsToTxnRequest.Builder(transactionalId, producerId, producerEpoch,
partitions)
+    builder.build()
+  }
+}


Mime
View raw message