You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/08 21:48:53 UTC
[GitHub] [kafka] cmccabe opened a new pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
cmccabe opened a new pull request #10084:
URL: https://github.com/apache/kafka/pull/10084
Rename DecommissionBrokers to UnregisterBrokers.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r573229531
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -217,6 +217,7 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.ENVELOPE => handleEnvelope(request)
case ApiKeys.DESCRIBE_CLUSTER => handleDescribeCluster(request)
case ApiKeys.DESCRIBE_PRODUCERS => handleDescribeProducersRequest(request)
+ case ApiKeys.UNREGISTER_BROKER => maybeForwardToController(request, handleUnregisterBrokerRequest)
Review comment:
The comment is not invalid. It just doesn't apply to UNREGISTER_BROKER. It does apply to VOTE, BEGIN_QUORUM_EPOCH, etc. etc.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r573823788
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/Admin.java
##########
@@ -1486,41 +1486,46 @@ default DescribeFeaturesResult describeFeatures() {
UpdateFeaturesResult updateFeatures(Map<String, FeatureUpdate> featureUpdates, UpdateFeaturesOptions options);
/**
- * Permanently remove a broker and reassign any partitions on the broker.
+ * Unregister a broker.
* <p>
- * This operation is supported only on self-managed Kafka clusters (i.e. brokers which do not rely on Zookeeper).
+ * This operation does not have any effect on partition assignments. It is supported
+ * only on Kafka clusters which use Raft to store metadata, rather than ZooKeeper.
*
- * @param brokerId the broker id to unregister.
+ * This is a convenience method for {@link #unregisterBroker(int, UnregisterBrokerOptions)}
*
- * <p>This is a convenience method for {@link #decommissionBroker(int, DecommissionBrokerOptions)}
+ * @param brokerId the broker id to unregister.
*
- * @return the {@link DecommissionBrokerResult} containing the result
+ * @return the {@link UnregisterBrokerResult} containing the result
*/
- default DecommissionBrokerResult decommissionBroker(int brokerId) {
- return decommissionBroker(brokerId, new DecommissionBrokerOptions());
+ @InterfaceStability.Unstable
+ default UnregisterBrokerResult unregisterBroker(int brokerId) {
+ return unregisterBroker(brokerId, new UnregisterBrokerOptions());
}
/**
- * Permanently remove a broker and reassign any partitions on the broker.
+ * Unregister a broker.
* <p>
- * This operation is supported only on self-managed Kafka clusters (i.e. brokers which do not rely on Zookeeper).
+ * This operation does not have any effect on partition assignments. It is supported
+ * only on Kafka clusters which use Raft to store metadata, rather than ZooKeeper.
*
* The following exceptions can be anticipated when calling {@code get()} on the future from the
* returned {@link DescribeFeaturesResult}:
Review comment:
"DescribeFeaturesResult" needs to be updated.
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -3390,6 +3390,11 @@ class KafkaApis(val requestChannel: RequestChannel,
new DescribeProducersResponse(response.setThrottleTimeMs(requestThrottleMs)))
}
+ def handleUnregisterBrokerRequest(request: RequestChannel.Request): Unit = {
+ throw new UnsupportedVersionException("The broker unregistration API is not available when using " +
+ "Apache ZooKeeper mode.")
Review comment:
This is a bit confusing since this method can be called when the built-in quorum mode is used too. Or am I missing something?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r572538990
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -217,6 +217,7 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.ENVELOPE => handleEnvelope(request)
case ApiKeys.DESCRIBE_CLUSTER => handleDescribeCluster(request)
case ApiKeys.DESCRIBE_PRODUCERS => handleDescribeProducersRequest(request)
+ case ApiKeys.UNREGISTER_BROKER => maybeForwardToController(request, handleUnregisterBrokerRequest)
Review comment:
The other APIs from KIP-500 is just closed. Maybe this API could follow same pattern?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r574006888
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -3390,6 +3390,11 @@ class KafkaApis(val requestChannel: RequestChannel,
new DescribeProducersResponse(response.setThrottleTimeMs(requestThrottleMs)))
}
+ def handleUnregisterBrokerRequest(request: RequestChannel.Request): Unit = {
+ throw new UnsupportedVersionException("The broker unregistration API is not available when using " +
+ "Apache ZooKeeper mode.")
Review comment:
This method can't be called in built-in quorum mode since the API is forwardable and we will always have a forwarding manager
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #10084:
URL: https://github.com/apache/kafka/pull/10084
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r572957476
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/Admin.java
##########
@@ -1486,41 +1486,40 @@ default DescribeFeaturesResult describeFeatures() {
UpdateFeaturesResult updateFeatures(Map<String, FeatureUpdate> featureUpdates, UpdateFeaturesOptions options);
/**
- * Permanently remove a broker and reassign any partitions on the broker.
+ * Unregister a broker.
* <p>
- * This operation is supported only on self-managed Kafka clusters (i.e. brokers which do not rely on Zookeeper).
+ * This is a convenience method for {@link #unregisterBroker(int, UnregisterBrokerOptions)}
*
* @param brokerId the broker id to unregister.
*
- * <p>This is a convenience method for {@link #decommissionBroker(int, DecommissionBrokerOptions)}
- *
- * @return the {@link DecommissionBrokerResult} containing the result
+ * @return the {@link UnregisterBrokerResult} containing the result
*/
- default DecommissionBrokerResult decommissionBroker(int brokerId) {
- return decommissionBroker(brokerId, new DecommissionBrokerOptions());
+ default UnregisterBrokerResult unregisterBroker(int brokerId) {
+ return unregisterBroker(brokerId, new UnregisterBrokerOptions());
}
/**
- * Permanently remove a broker and reassign any partitions on the broker.
+ * Unregister a broker.
* <p>
- * This operation is supported only on self-managed Kafka clusters (i.e. brokers which do not rely on Zookeeper).
+ * This operation does not have any effect on partition assignments. It is supported
+ * only on self-managed Kafka clusters (i.e. brokers which do not rely on Zookeeper).
*
* The following exceptions can be anticipated when calling {@code get()} on the future from the
* returned {@link DescribeFeaturesResult}:
* <ul>
* <li>{@link org.apache.kafka.common.errors.TimeoutException}
* If the request timed out before the describe operation could finish.</li>
* <li>{@link org.apache.kafka.common.errors.UnsupportedVersionException}
- * If the software is too old to support decommissioning.
+ * If the software is too old to support the unregistration API.
* </ul>
* <p>
*
* @param brokerId the broker id to unregister.
* @param options the options to use.
*
- * @return the {@link DecommissionBrokerResult} containing the result
+ * @return the {@link UnregisterBrokerResult} containing the result
*/
- DecommissionBrokerResult decommissionBroker(int brokerId, DecommissionBrokerOptions options);
+ UnregisterBrokerResult unregisterBroker(int brokerId, UnregisterBrokerOptions options);
Review comment:
@cmccabe We should mark all KIP-500 methods with `Unstable` and mention it in the documentation too.
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/Admin.java
##########
@@ -1486,41 +1486,40 @@ default DescribeFeaturesResult describeFeatures() {
UpdateFeaturesResult updateFeatures(Map<String, FeatureUpdate> featureUpdates, UpdateFeaturesOptions options);
/**
- * Permanently remove a broker and reassign any partitions on the broker.
+ * Unregister a broker.
* <p>
- * This operation is supported only on self-managed Kafka clusters (i.e. brokers which do not rely on Zookeeper).
+ * This is a convenience method for {@link #unregisterBroker(int, UnregisterBrokerOptions)}
*
* @param brokerId the broker id to unregister.
*
- * <p>This is a convenience method for {@link #decommissionBroker(int, DecommissionBrokerOptions)}
- *
- * @return the {@link DecommissionBrokerResult} containing the result
+ * @return the {@link UnregisterBrokerResult} containing the result
*/
- default DecommissionBrokerResult decommissionBroker(int brokerId) {
- return decommissionBroker(brokerId, new DecommissionBrokerOptions());
+ default UnregisterBrokerResult unregisterBroker(int brokerId) {
+ return unregisterBroker(brokerId, new UnregisterBrokerOptions());
}
/**
- * Permanently remove a broker and reassign any partitions on the broker.
+ * Unregister a broker.
* <p>
- * This operation is supported only on self-managed Kafka clusters (i.e. brokers which do not rely on Zookeeper).
+ * This operation does not have any effect on partition assignments. It is supported
+ * only on self-managed Kafka clusters (i.e. brokers which do not rely on Zookeeper).
*
* The following exceptions can be anticipated when calling {@code get()} on the future from the
* returned {@link DescribeFeaturesResult}:
* <ul>
* <li>{@link org.apache.kafka.common.errors.TimeoutException}
* If the request timed out before the describe operation could finish.</li>
* <li>{@link org.apache.kafka.common.errors.UnsupportedVersionException}
- * If the software is too old to support decommissioning.
+ * If the software is too old to support the unregistration API.
* </ul>
* <p>
*
* @param brokerId the broker id to unregister.
* @param options the options to use.
*
- * @return the {@link DecommissionBrokerResult} containing the result
+ * @return the {@link UnregisterBrokerResult} containing the result
*/
- DecommissionBrokerResult decommissionBroker(int brokerId, DecommissionBrokerOptions options);
+ UnregisterBrokerResult unregisterBroker(int brokerId, UnregisterBrokerOptions options);
Review comment:
@cmccabe We should mark all KIP-500 methods with `@Unstable` and mention it in the documentation too.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r574060464
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -3390,6 +3390,11 @@ class KafkaApis(val requestChannel: RequestChannel,
new DescribeProducersResponse(response.setThrottleTimeMs(requestThrottleMs)))
}
+ def handleUnregisterBrokerRequest(request: RequestChannel.Request): Unit = {
+ throw new UnsupportedVersionException("The broker unregistration API is not available when using " +
+ "Apache ZooKeeper mode.")
Review comment:
will do
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r574048714
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -3390,6 +3390,11 @@ class KafkaApis(val requestChannel: RequestChannel,
new DescribeProducersResponse(response.setThrottleTimeMs(requestThrottleMs)))
}
+ def handleUnregisterBrokerRequest(request: RequestChannel.Request): Unit = {
+ throw new UnsupportedVersionException("The broker unregistration API is not available when using " +
+ "Apache ZooKeeper mode.")
Review comment:
Can we add a comment explaining that?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r573229531
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -217,6 +217,7 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.ENVELOPE => handleEnvelope(request)
case ApiKeys.DESCRIBE_CLUSTER => handleDescribeCluster(request)
case ApiKeys.DESCRIBE_PRODUCERS => handleDescribeProducersRequest(request)
+ case ApiKeys.UNREGISTER_BROKER => maybeForwardToController(request, handleUnregisterBrokerRequest)
Review comment:
The comment is not invalid. It just doesn't apply to UNREGISTER_BROKER. It does apply to VOTE, BEGIN_QUORUM_EPOCH, etc. etc. I moved UNREGISTER_BROKER so it no longer appears underneath the comment.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r572633977
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -217,6 +217,7 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.ENVELOPE => handleEnvelope(request)
case ApiKeys.DESCRIBE_CLUSTER => handleDescribeCluster(request)
case ApiKeys.DESCRIBE_PRODUCERS => handleDescribeProducersRequest(request)
+ case ApiKeys.UNREGISTER_BROKER => maybeForwardToController(request, handleUnregisterBrokerRequest)
Review comment:
@cmccabe Thanks for explanation. It seems the [comment](https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaApis.scala#L221) gets invalid now.
```
Handle requests that should have been sent to the KIP-500 controlle
```
Could you revise the comment as well?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10084: MINOR: Rename DecommissionBrokers to UnregisterBrokers
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10084:
URL: https://github.com/apache/kafka/pull/10084#discussion_r572627716
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -217,6 +217,7 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.ENVELOPE => handleEnvelope(request)
case ApiKeys.DESCRIBE_CLUSTER => handleDescribeCluster(request)
case ApiKeys.DESCRIBE_PRODUCERS => handleDescribeProducersRequest(request)
+ case ApiKeys.UNREGISTER_BROKER => maybeForwardToController(request, handleUnregisterBrokerRequest)
Review comment:
This call needs to be forwarded to the KIP-500 controller. That is different from the other KIP-500 RPCs which are controller RPCs, and are not expected to be received on the broker at all.
----------------------------------------------------------------
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:
users@infra.apache.org