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/01/28 23:57:06 UTC
[GitHub] [kafka] aloknnikhil opened a new pull request #9994: MINOR: Add BrokerHeartbeat/BrokerRegistration/DecommissionBroker RPCs for KIP-500
aloknnikhil opened a new pull request #9994:
URL: https://github.com/apache/kafka/pull/9994
### Committer Checklist (excluded from commit message)
- [x] Verify design and implementation
- [x] Verify test coverage and CI build status
- [x] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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] hachikuji merged pull request #9994: KAFKA-12248: Add BrokerHeartbeat/BrokerRegistration RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9994:
URL: https://github.com/apache/kafka/pull/9994
----------------------------------------------------------------
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] hachikuji commented on a change in pull request #9994: KAFKA-12248: Add BrokerHeartbeat/BrokerRegistration RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9994:
URL: https://github.com/apache/kafka/pull/9994#discussion_r566987833
##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -98,7 +98,9 @@
UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
ENVELOPE(ApiMessageType.ENVELOPE, true, RecordBatch.MAGIC_VALUE_V0, false, true),
FETCH_SNAPSHOT(ApiMessageType.FETCH_SNAPSHOT, false, RecordBatch.MAGIC_VALUE_V0, false, true),
- DESCRIBE_CLUSTER(ApiMessageType.DESCRIBE_CLUSTER);
+ DESCRIBE_CLUSTER(ApiMessageType.DESCRIBE_CLUSTER),
+ BROKER_REGISTRATION(ApiMessageType.BROKER_REGISTRATION, false, RecordBatch.MAGIC_VALUE_V0, false, true),
Review comment:
I think the second flag should be true for both of these. They do require CLUSTER_ACTION.
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -232,6 +232,9 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.DESCRIBE_QUORUM => requestHelper.closeConnection(request, util.Collections.emptyMap())
case ApiKeys.FETCH_SNAPSHOT => requestHelper.closeConnection(request, util.Collections.emptyMap())
+ // Handle requests that should have been sent to the KIP-500 controller.
Review comment:
nit: this applies to all of the Raft apis above 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] hachikuji merged pull request #9994: KAFKA-12248: Add BrokerHeartbeat/BrokerRegistration RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9994:
URL: https://github.com/apache/kafka/pull/9994
----------------------------------------------------------------
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] aloknnikhil commented on a change in pull request #9994: KAFKA-12248: Add BrokerHeartbeat/BrokerRegistration RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
aloknnikhil commented on a change in pull request #9994:
URL: https://github.com/apache/kafka/pull/9994#discussion_r568033646
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -232,6 +232,9 @@ class KafkaApis(val requestChannel: RequestChannel,
case ApiKeys.DESCRIBE_QUORUM => requestHelper.closeConnection(request, util.Collections.emptyMap())
case ApiKeys.FETCH_SNAPSHOT => requestHelper.closeConnection(request, util.Collections.emptyMap())
+ // Handle requests that should have been sent to the KIP-500 controller.
Review comment:
Ack.
----------------------------------------------------------------
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] aloknnikhil commented on pull request #9994: KAFKA-12248: Add BrokerHeartbeat/BrokerRegistration RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
aloknnikhil commented on pull request #9994:
URL: https://github.com/apache/kafka/pull/9994#issuecomment-771079401
> Left a couple comments. You will need to update RequestConvertToJson as well. Probably we should have some tests in RequestResponseTest as well.
Done.
----------------------------------------------------------------
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] hachikuji commented on pull request #9994: KAFKA-12248: Add BrokerHeartbeat/BrokerRegistration RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #9994:
URL: https://github.com/apache/kafka/pull/9994#issuecomment-771137129
@aloknnikhil Looks like we will need to fix some failures in `ApiKeysTest` before we can merge this. I suspect that the test does not like the throttle time fields. One way we could handle this is to ignore `controllerOnly` APIs in this test case. I guess we haven't really figured out the throttling picture for broker-to-controller APIs.
----------------------------------------------------------------
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] aloknnikhil commented on pull request #9994: KAFKA-12248: Add BrokerHeartbeat/BrokerRegistration RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
aloknnikhil commented on pull request #9994:
URL: https://github.com/apache/kafka/pull/9994#issuecomment-769516430
@hachikuji Fixed it up to merge only the Broker-Controller RPCs (Heartbeat/Registration)
----------------------------------------------------------------
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] hachikuji commented on pull request #9994: MINOR: Add BrokerHeartbeat/BrokerRegistration/DecommissionBroker RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #9994:
URL: https://github.com/apache/kafka/pull/9994#issuecomment-769479261
Thanks for the PR. Just to make this a little more manageable, could we split this into two PRs? One for the register/heartbeat controller APIs? And a separate one for decommission, which is a client API.
Also, it would be nice to have JIRAs since these are significant additions. Thanks!
----------------------------------------------------------------
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] aloknnikhil commented on a change in pull request #9994: KAFKA-12248: Add BrokerHeartbeat/BrokerRegistration RPCs for KIP-500
Posted by GitBox <gi...@apache.org>.
aloknnikhil commented on a change in pull request #9994:
URL: https://github.com/apache/kafka/pull/9994#discussion_r568033553
##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -98,7 +98,9 @@
UPDATE_FEATURES(ApiMessageType.UPDATE_FEATURES, false, true),
ENVELOPE(ApiMessageType.ENVELOPE, true, RecordBatch.MAGIC_VALUE_V0, false, true),
FETCH_SNAPSHOT(ApiMessageType.FETCH_SNAPSHOT, false, RecordBatch.MAGIC_VALUE_V0, false, true),
- DESCRIBE_CLUSTER(ApiMessageType.DESCRIBE_CLUSTER);
+ DESCRIBE_CLUSTER(ApiMessageType.DESCRIBE_CLUSTER),
+ BROKER_REGISTRATION(ApiMessageType.BROKER_REGISTRATION, false, RecordBatch.MAGIC_VALUE_V0, false, true),
Review comment:
Ack.
----------------------------------------------------------------
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