[GitHub] [kafka] cmccabe commented on a change in pull request #9103: Add redirection for (Incremental)AlterConfig, AlterClientQuota and CreateTopics

2020-08-24 Thread GitBox 


cmccabe commented on a change in pull request #9103:
URL: https://github.com/apache/kafka/pull/9103#discussion_r475725389



##
File path: core/src/main/scala/kafka/server/KafkaServer.scala
##
@@ -316,7 +316,10 @@ class KafkaServer(val config: KafkaConfig, time: Time = 
Time.SYSTEM, threadNameP
 kafkaController = new KafkaController(config, zkClient, time, metrics, 
brokerInfo, brokerEpoch, tokenManager, threadNamePrefix)
 kafkaController.startup()
 
-brokerToControllerChannelManager = new 
BrokerToControllerChannelManager(metadataCache, time, metrics, config, 
threadNamePrefix)
+if (config.redirectionEnabled) {
+  brokerToControllerChannelManager = new 
BrokerToControllerChannelManager(metadataCache, time, metrics, config, 
threadNamePrefix)

Review comment:
   just as a note the alter isr PR may also have an object like this.  so 
maybe we want a name which is more specific to redirection.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] cmccabe commented on a change in pull request #9103: Add redirection for (Incremental)AlterConfig, AlterClientQuota and CreateTopics

2020-08-24 Thread GitBox 


cmccabe commented on a change in pull request #9103:
URL: https://github.com/apache/kafka/pull/9103#discussion_r475714230



##
File path: 
clients/src/main/java/org/apache/kafka/common/requests/AlterConfigsRequest.java
##
@@ -87,6 +87,16 @@ public Builder(Map configs, boolean 
validateOnly) {
 public AlterConfigsRequest build(short version) {
 return new AlterConfigsRequest(data, version);
 }
+

Review comment:
   In general we don't define equals or hashCode on these builders.  Why 
are we defining it here?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] cmccabe commented on a change in pull request #9103: Add redirection for (Incremental)AlterConfig, AlterClientQuota and CreateTopics

2020-08-24 Thread GitBox 


cmccabe commented on a change in pull request #9103:
URL: https://github.com/apache/kafka/pull/9103#discussion_r475712573



##
File path: clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
##
@@ -325,7 +326,9 @@
 UNSTABLE_OFFSET_COMMIT(88, "There are unstable offsets that need to be 
cleared.", UnstableOffsetCommitException::new),
 THROTTLING_QUOTA_EXCEEDED(89, "The throttling quota has been exceeded.", 
ThrottlingQuotaExceededException::new),
 PRODUCER_FENCED(90, "There is a newer producer with the same 
transactionalId " +
-"which fences the current one.", ProducerFencedException::new);
+"which fences the current one.", ProducerFencedException::new),
+BROKER_AUTHORIZATION_FAILURE(91, "Authorization failed for the request 
during forwarding. " +

Review comment:
   How about: "A broker failed to authorize itself to another component of 
the system.  This indicates an internal error on the broker cluster security 
setup".
   
   This isn't specific to forwarding... there might be other reasons why a 
broker would need to authorize itself and fail





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kafka] cmccabe commented on a change in pull request #9103: Add redirection for (Incremental)AlterConfig, AlterClientQuota and CreateTopics

2020-08-24 Thread GitBox 


cmccabe commented on a change in pull request #9103:
URL: https://github.com/apache/kafka/pull/9103#discussion_r475710400



##
File path: 
clients/src/main/java/org/apache/kafka/common/errors/BrokerAuthorizationFailureException.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;
+
+/**
+ * Exception used to indicate a broker side authorization failure during 
request redirection.
+ */
+public class BrokerAuthorizationFailureException extends 
AuthorizationException {
+

Review comment:
   Need to include:
   ```
   private static final long serialVersionUID = 1L;
   ```





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org