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 2022/11/30 22:17:20 UTC
[GitHub] [kafka] mumrah opened a new pull request, #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
mumrah opened a new pull request, #12928:
URL: https://github.com/apache/kafka/pull/12928
This patch adds the schema changes from https://cwiki.apache.org/confluence/display/KAFKA/KIP-866+ZooKeeper+to+KRaft+Migration
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12928:
URL: https://github.com/apache/kafka/pull/12928#issuecomment-1332823758
More naming nitpicks:
`ZkMigrationRecord` -> `ZkMigrationStateRecord` ?
`MigrationRecoveryState` -> `ZkMigrationLeadershipState` ?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036500251
##########
metadata/src/main/java/org/apache/kafka/migration/MigrationRecoveryState.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.migration;
+
+import java.util.Objects;
+
+/**
+ * Persistent state needed to recover an ongoing migration. This data is stored in ZooKeeper under the "/migration"
+ * ZNode and is recovered by the active KRaft controller following an election. The absence of this data in ZK indicates
+ * that no migration has been started.
+ */
+public class MigrationRecoveryState {
+
+ public static final MigrationRecoveryState EMPTY = new MigrationRecoveryState(-1, -1, -1, -1, -1, -1, -1);
+
+ private final int kraftControllerId;
+
+ private final int kraftControllerEpoch;
+
+ private final long kraftMetadataOffset;
+
+ private final long kraftMetadataEpoch;
+
+ private final long lastUpdatedTimeMs;
+
+ private final int migrationZkVersion;
Review Comment:
this should be a `long` right?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036512516
##########
clients/src/main/resources/common/message/BrokerRegistrationRequest.json:
##########
@@ -50,6 +50,8 @@
]
},
{ "name": "Rack", "type": "string", "versions": "0+", "nullableVersions": "0+",
- "about": "The rack which this broker is in." }
+ "about": "The rack which this broker is in." },
+ { "name": "ZkMigrationReady", "type": "int8", "versions": "0+", "taggedVersions": "0+", "tag": 0, "ignorable": true,
Review Comment:
It would be nice if this name matched up with the field in the broker registration.
Maybe something like `IsMigratingZkBroker` ?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036508634
##########
metadata/src/main/resources/common/metadata/ZkMigrationRecord.json:
##########
@@ -0,0 +1,13 @@
+{
Review Comment:
can we have an associated enum for this? Earlier we sketched out
NONE = 0
PREMIGRATION = 1
MIGRATION = 2
POSTMIGRATION = 3
(0 being the default if no record is present)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12928:
URL: https://github.com/apache/kafka/pull/12928#issuecomment-1332821084
What do you think about renaming `org.apache.kafka.migration` to `org.apache.kafka.metadata.migration`? It seems more appropriate since this is all about migrating metadata
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036504699
##########
metadata/src/main/java/org/apache/kafka/migration/MigrationRecoveryState.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.migration;
+
+import java.util.Objects;
+
+/**
+ * Persistent state needed to recover an ongoing migration. This data is stored in ZooKeeper under the "/migration"
+ * ZNode and is recovered by the active KRaft controller following an election. The absence of this data in ZK indicates
+ * that no migration has been started.
+ */
+public class MigrationRecoveryState {
Review Comment:
Does it make sense to have a `ZkControllerState` object contained in this object?
seems like the fields here are a superset of the fields in `ZkControllerState`
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1037537646
##########
metadata/src/main/java/org/apache/kafka/migration/MigrationRecoveryState.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.migration;
+
+import java.util.Objects;
+
+/**
+ * Persistent state needed to recover an ongoing migration. This data is stored in ZooKeeper under the "/migration"
+ * ZNode and is recovered by the active KRaft controller following an election. The absence of this data in ZK indicates
+ * that no migration has been started.
+ */
+public class MigrationRecoveryState {
+
+ public static final MigrationRecoveryState EMPTY = new MigrationRecoveryState(-1, -1, -1, -1, -1, -1, -1);
+
+ private final int kraftControllerId;
+
+ private final int kraftControllerEpoch;
+
+ private final long kraftMetadataOffset;
+
+ private final long kraftMetadataEpoch;
+
+ private final long lastUpdatedTimeMs;
+
+ private final int migrationZkVersion;
Review Comment:
Hm, looks like ZK Stat#version is an int https://github.com/apache/zookeeper/blob/master/zookeeper-jute/src/main/resources/zookeeper.jute#L34
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036500654
##########
metadata/src/main/java/org/apache/kafka/migration/MigrationRecoveryState.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.migration;
+
+import java.util.Objects;
+
+/**
+ * Persistent state needed to recover an ongoing migration. This data is stored in ZooKeeper under the "/migration"
+ * ZNode and is recovered by the active KRaft controller following an election. The absence of this data in ZK indicates
+ * that no migration has been started.
+ */
+public class MigrationRecoveryState {
+
+ public static final MigrationRecoveryState EMPTY = new MigrationRecoveryState(-1, -1, -1, -1, -1, -1, -1);
+
+ private final int kraftControllerId;
+
+ private final int kraftControllerEpoch;
+
+ private final long kraftMetadataOffset;
+
+ private final long kraftMetadataEpoch;
+
+ private final long lastUpdatedTimeMs;
+
+ private final int migrationZkVersion;
+
+ private final int controllerZkVersion;
Review Comment:
this should be a `long` I believe
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah merged pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
mumrah merged PR #12928:
URL: https://github.com/apache/kafka/pull/12928
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1037541558
##########
metadata/src/main/java/org/apache/kafka/migration/MigrationRecoveryState.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.migration;
+
+import java.util.Objects;
+
+/**
+ * Persistent state needed to recover an ongoing migration. This data is stored in ZooKeeper under the "/migration"
+ * ZNode and is recovered by the active KRaft controller following an election. The absence of this data in ZK indicates
+ * that no migration has been started.
+ */
+public class MigrationRecoveryState {
Review Comment:
Hm, yea I think we can do without this separate `ZkControllerState ` class.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036499024
##########
clients/src/main/resources/common/message/StopReplicaRequest.json:
##########
@@ -24,11 +24,15 @@
// Version 2 is the first flexible version.
//
// Version 3 adds the leader epoch per partition (KIP-570).
- "validVersions": "0-3",
+ //
+ // Version 4 adds KRaft Controller ID field as part of KIP-866
+ "validVersions": "0-4",
"flexibleVersions": "2+",
"fields": [
{ "name": "ControllerId", "type": "int32", "versions": "0+", "entityType": "brokerId",
"about": "The controller id." },
+ { "name": "KRaftControllerId", "type": "int32", "versions": "4+", "entityType": "brokerId",
Review Comment:
can we add a default of -1? Also add KIP-866 ref
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036500418
##########
metadata/src/main/java/org/apache/kafka/migration/MigrationRecoveryState.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.migration;
+
+import java.util.Objects;
+
+/**
+ * Persistent state needed to recover an ongoing migration. This data is stored in ZooKeeper under the "/migration"
+ * ZNode and is recovered by the active KRaft controller following an election. The absence of this data in ZK indicates
+ * that no migration has been started.
+ */
+public class MigrationRecoveryState {
+
+ public static final MigrationRecoveryState EMPTY = new MigrationRecoveryState(-1, -1, -1, -1, -1, -1, -1);
+
+ private final int kraftControllerId;
+
+ private final int kraftControllerEpoch;
+
+ private final long kraftMetadataOffset;
+
+ private final long kraftMetadataEpoch;
Review Comment:
this should be an `int`
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036510729
##########
core/src/main/scala/kafka/zk/ZkData.scala:
##########
@@ -56,8 +57,8 @@ import scala.util.{Failure, Success, Try}
object ControllerZNode {
def path = "/controller"
- def encode(brokerId: Int, timestamp: Long): Array[Byte] = {
- Json.encodeAsBytes(Map("version" -> 1, "brokerid" -> brokerId, "timestamp" -> timestamp.toString).asJava)
+ def encode(brokerId: Int, timestamp: Long, isKRaft: Boolean = false): Array[Byte] = {
Review Comment:
One thing I was thinking is should we have the `kraftControllerEpoch` here instead of `isKraft`? Then `kraftControllerEpoch` == -1 (or not present) could mean ZK mode?
it might help with tracing what was going on...
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12928: KAFKA-14304 Add RPC changes, records, and config from KIP-866
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12928:
URL: https://github.com/apache/kafka/pull/12928#discussion_r1036498788
##########
clients/src/main/resources/common/message/LeaderAndIsrRequest.json:
##########
@@ -29,11 +29,15 @@
// Version 5 adds Topic ID and Type to the TopicStates, as described in KIP-516.
//
// Version 6 adds LeaderRecoveryState as described in KIP-704.
- "validVersions": "0-6",
+ //
+ // Version 7 adds KRaft Controller ID field as part of KIP-866
+ "validVersions": "0-7",
"flexibleVersions": "4+",
"fields": [
{ "name": "ControllerId", "type": "int32", "versions": "0+", "entityType": "brokerId",
"about": "The current controller ID." },
+ { "name": "KRaftControllerId", "type": "int32", "versions": "7+", "entityType": "brokerId",
Review Comment:
Can we add a reference to KIP-866 here?
Also, maybe good to set a default of -1 which will be used in the ZK case
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org