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 2020/08/05 15:23:48 UTC

[GitHub] [kafka] hachikuji opened a new pull request #9130: [DRAFT] Kafka Raft Implementation (KIP-595)

hachikuji opened a new pull request #9130:
URL: https://github.com/apache/kafka/pull/9130


   This is a draft of the changes needed for KIP-595. The draft tag will be removed in the upcoming weeks if/when the KIP is adopted. Note that there are still a few significant protocol differences from the documented proposal that we are still in the process of reconciling.
   
   Co-authored-by: Boyang Chen <bo...@confluent.io>
   Co-authored-by: Guozhang Wang <wa...@gmail.com> 
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] 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 #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9130:
URL: https://github.com/apache/kafka/pull/9130


   


----------------------------------------------------------------
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 #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9130:
URL: https://github.com/apache/kafka/pull/9130#discussion_r492211755



##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -182,10 +182,13 @@ class KafkaApis(val requestChannel: RequestChannel,
         case ApiKeys.ALTER_CLIENT_QUOTAS => handleAlterClientQuotasRequest(request)
         case ApiKeys.DESCRIBE_USER_SCRAM_CREDENTIALS => handleDescribeUserScramCredentialsRequest(request)
         case ApiKeys.ALTER_USER_SCRAM_CREDENTIALS => handleAlterUserScramCredentialsRequest(request)
-        case ApiKeys.VOTE => handleVote(request)
-        case ApiKeys.BEGIN_QUORUM_EPOCH => handleBeginEpoch(request)
-        case ApiKeys.END_QUORUM_EPOCH => handleEndEpoch(request)
-        case ApiKeys.DESCRIBE_QUORUM => handleDescribeQuorum(request)
+
+        // Until we are ready to integrate the Raft layer, these APIs are treated as
+        // unexpected and we just close the connection.
+        case ApiKeys.VOTE => closeConnection(request, util.Collections.emptyMap())

Review comment:
       Right. We should never reach here, but I thought we should make the `match` complete to avoid warnings.




----------------------------------------------------------------
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 #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9130:
URL: https://github.com/apache/kafka/pull/9130


   


----------------------------------------------------------------
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] guozhangwang commented on pull request #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #9130:
URL: https://github.com/apache/kafka/pull/9130#issuecomment-695511369


   Also browsed through the commit history and it matches my memory. Thanks for the awesome job @hachikuji 


----------------------------------------------------------------
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 #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9130:
URL: https://github.com/apache/kafka/pull/9130#discussion_r492211755



##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -182,10 +182,13 @@ class KafkaApis(val requestChannel: RequestChannel,
         case ApiKeys.ALTER_CLIENT_QUOTAS => handleAlterClientQuotasRequest(request)
         case ApiKeys.DESCRIBE_USER_SCRAM_CREDENTIALS => handleDescribeUserScramCredentialsRequest(request)
         case ApiKeys.ALTER_USER_SCRAM_CREDENTIALS => handleAlterUserScramCredentialsRequest(request)
-        case ApiKeys.VOTE => handleVote(request)
-        case ApiKeys.BEGIN_QUORUM_EPOCH => handleBeginEpoch(request)
-        case ApiKeys.END_QUORUM_EPOCH => handleEndEpoch(request)
-        case ApiKeys.DESCRIBE_QUORUM => handleDescribeQuorum(request)
+
+        // Until we are ready to integrate the Raft layer, these APIs are treated as
+        // unexpected and we just close the connection.
+        case ApiKeys.VOTE => closeConnection(request, util.Collections.emptyMap())

Review comment:
       Right. We should never reach here, but I thought we should make the `match` complete to avoid warnings.




----------------------------------------------------------------
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] guozhangwang commented on a change in pull request #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #9130:
URL: https://github.com/apache/kafka/pull/9130#discussion_r491745941



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -415,4 +444,10 @@ public void visit(Type field) {
         return hasBuffer.get();
     }
 
+    public static Set<ApiKeys> enabledApis() {

Review comment:
       nit: if this is only used for unit tests, better add a one-liner comment for that.

##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -182,10 +182,13 @@ class KafkaApis(val requestChannel: RequestChannel,
         case ApiKeys.ALTER_CLIENT_QUOTAS => handleAlterClientQuotasRequest(request)
         case ApiKeys.DESCRIBE_USER_SCRAM_CREDENTIALS => handleDescribeUserScramCredentialsRequest(request)
         case ApiKeys.ALTER_USER_SCRAM_CREDENTIALS => handleAlterUserScramCredentialsRequest(request)
-        case ApiKeys.VOTE => handleVote(request)
-        case ApiKeys.BEGIN_QUORUM_EPOCH => handleBeginEpoch(request)
-        case ApiKeys.END_QUORUM_EPOCH => handleEndEpoch(request)
-        case ApiKeys.DESCRIBE_QUORUM => handleDescribeQuorum(request)
+
+        // Until we are ready to integrate the Raft layer, these APIs are treated as
+        // unexpected and we just close the connection.
+        case ApiKeys.VOTE => closeConnection(request, util.Collections.emptyMap())

Review comment:
       I feel this is not necessary since we would throw in `processCompletedReceives` at SocketServer already as `processChannelException`, but no harm guarding them 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] guozhangwang commented on a change in pull request #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #9130:
URL: https://github.com/apache/kafka/pull/9130#discussion_r491745941



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -415,4 +444,10 @@ public void visit(Type field) {
         return hasBuffer.get();
     }
 
+    public static Set<ApiKeys> enabledApis() {

Review comment:
       nit: if this is only used for unit tests, better add a one-liner comment for that.

##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -182,10 +182,13 @@ class KafkaApis(val requestChannel: RequestChannel,
         case ApiKeys.ALTER_CLIENT_QUOTAS => handleAlterClientQuotasRequest(request)
         case ApiKeys.DESCRIBE_USER_SCRAM_CREDENTIALS => handleDescribeUserScramCredentialsRequest(request)
         case ApiKeys.ALTER_USER_SCRAM_CREDENTIALS => handleAlterUserScramCredentialsRequest(request)
-        case ApiKeys.VOTE => handleVote(request)
-        case ApiKeys.BEGIN_QUORUM_EPOCH => handleBeginEpoch(request)
-        case ApiKeys.END_QUORUM_EPOCH => handleEndEpoch(request)
-        case ApiKeys.DESCRIBE_QUORUM => handleDescribeQuorum(request)
+
+        // Until we are ready to integrate the Raft layer, these APIs are treated as
+        // unexpected and we just close the connection.
+        case ApiKeys.VOTE => closeConnection(request, util.Collections.emptyMap())

Review comment:
       I feel this is not necessary since we would throw in `processCompletedReceives` at SocketServer already as `processChannelException`, but no harm guarding them 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 commented on pull request #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #9130:
URL: https://github.com/apache/kafka/pull/9130#issuecomment-696301126


   As long as there are no problems with the build, I am planning to merge this within the next couple days. For a history of the review comments, please see: https://github.com/confluentinc/kafka/commits/kafka-raft. 
   
   Note that we have taken pains to ensure that the new APIs added for the Raft layer are not exposed so that we can continue iterating on them until they are ready to be integrated with the controller. There are also a few known performance issues which need to be sorted out. All of this will be tracked in https://issues.apache.org/jira/browse/KAFKA-9876 (in case anyone wants to help out). 


----------------------------------------------------------------
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 #9130: KAFKA-10492; Core Kafka Raft Implementation (KIP-595)

Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #9130:
URL: https://github.com/apache/kafka/pull/9130#issuecomment-696301126


   As long as there are no problems with the build, I am planning to merge this within the next couple days. For a history of the review comments, please see: https://github.com/confluentinc/kafka/commits/kafka-raft. 
   
   Note that we have taken pains to ensure that the new APIs added for the Raft layer are not exposed so that we can continue iterating on them until they are ready to be integrated with the controller. There are also a few known performance issues which need to be sorted out. All of this will be tracked in https://issues.apache.org/jira/browse/KAFKA-9876 (in case anyone wants to help out). 


----------------------------------------------------------------
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