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/04/06 20:06:38 UTC
[GitHub] [kafka] jolshan opened a new pull request #10492: KAFKA-12457: Added sentinel ID to metadata topic
jolshan opened a new pull request #10492:
URL: https://github.com/apache/kafka/pull/10492
KIP-516 introduces topic IDs to topics, but there is a small issue with how the metadata topic will interact with topic IDs.
For example, https://github.com/apache/kafka/pull/9944 aims to replace topic names in the Fetch request with topic IDs. In order to get these IDs, brokers must fetch from the metadata topic. This leads to a sort of "chicken and the egg" problem concerning how we find out the metadata topic's topic ID.
One solution proposed in KIP-516 was to introduce a "sentinel ID" for the metadata topic. This is a reserved ID for the metadata topic only. This PR adds the sentinel ID when creating the metadata log.
More information can be found in the [JIRA](https://issues.apache.org/jira/browse/KAFKA-12457) and in [KIP-516](https://cwiki.apache.org/confluence/display/KAFKA/KIP-516%3A+Topic+Identifiers)
### 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 commented on a change in pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608881469
##########
File path: core/src/main/scala/kafka/tools/TestRaftServer.scala
##########
@@ -51,7 +51,8 @@ class TestRaftServer(
) extends Logging {
import kafka.tools.TestRaftServer._
- private val partition = new TopicPartition("__cluster_metadata", 0)
+ private val partition = new TopicPartition("__raft_performance_test", 0)
+ private val topicId = new Uuid(0L, 2L)
Review comment:
Could we add a comment here which explains that this value must be constant and that the value is chosen not to conflict with the topicId used for `@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#issuecomment-814417281
@hachikuji we defined it and have a check, but it is internal. I can add a public constant.
--
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 #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608187610
##########
File path: raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java
##########
@@ -113,6 +113,7 @@
static final int DEFAULT_ELECTION_TIMEOUT_MS = 10000;
private static final TopicPartition METADATA_PARTITION = new TopicPartition("metadata", 0);
+ private static final Uuid METADATA_TOPIC_ID = Uuid.METADATA_TOPIC_ID;
Review comment:
Do we need to redefine another constant here and in `RaftEventSimulationTest`?
--
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 #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608789181
##########
File path: clients/src/main/java/org/apache/kafka/common/Uuid.java
##########
@@ -27,6 +27,10 @@
*/
public class Uuid implements Comparable<Uuid> {
+ /**
+ * A UUID for the metadata topic in KRaft mode. Will never be returned by the randomUuid method.
+ */
+ public static final Uuid METADATA_TOPIC_ID = new Uuid(0L, 1L);
private static final java.util.UUID SENTINEL_ID_INTERNAL = new java.util.UUID(0L, 1L);
Review comment:
Is this used for anything else? Maybe we can choose a consistent name?
##########
File path: core/src/main/scala/kafka/tools/TestRaftServer.scala
##########
@@ -52,6 +52,7 @@ class TestRaftServer(
import kafka.tools.TestRaftServer._
private val partition = new TopicPartition("__cluster_metadata", 0)
+ private val topicId = Uuid.METADATA_TOPIC_ID
Review comment:
I'm a tad concerned about reusing the same topic id in a different context. Maybe instead we could hard-code a different sentinel and use it only here, say `new Uuid(0L, 2L)`. Then we can also change the topic name to `__raft_performance_test` or something like that. That should be enough to prevent any potential conflicts.
--
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] jolshan edited a comment on pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
jolshan edited a comment on pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#issuecomment-814417281
@hachikuji we defined it and have a check, but it is internal/private. I can add a public constant.
--
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] jolshan commented on a change in pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608861828
##########
File path: clients/src/main/java/org/apache/kafka/common/Uuid.java
##########
@@ -27,6 +27,10 @@
*/
public class Uuid implements Comparable<Uuid> {
+ /**
+ * A UUID for the metadata topic in KRaft mode. Will never be returned by the randomUuid method.
+ */
+ public static final Uuid METADATA_TOPIC_ID = new Uuid(0L, 1L);
private static final java.util.UUID SENTINEL_ID_INTERNAL = new java.util.UUID(0L, 1L);
Review comment:
referring to the private constant? I can change that 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] jolshan commented on a change in pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608188681
##########
File path: raft/src/test/java/org/apache/kafka/raft/RaftClientTestContext.java
##########
@@ -113,6 +113,7 @@
static final int DEFAULT_ELECTION_TIMEOUT_MS = 10000;
private static final TopicPartition METADATA_PARTITION = new TopicPartition("metadata", 0);
+ private static final Uuid METADATA_TOPIC_ID = Uuid.METADATA_TOPIC_ID;
Review comment:
good point. Will fix
--
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 #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608177108
##########
File path: clients/src/main/java/org/apache/kafka/common/Uuid.java
##########
@@ -27,6 +27,10 @@
*/
public class Uuid implements Comparable<Uuid> {
+ /**
+ * A UUID for the metadata topic in KRaft mode. Will never be returned by the randomUuid method.
+ */
+ public static final Uuid SENTINEL_ID = new Uuid(0L, 1L);
Review comment:
Could we be more explicit and call this `METADATA_TOPIC_ID` or something like that? Do we have other contexts where we use it?
--
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] jolshan commented on pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#issuecomment-814431608
@hachikuji I wasn't sure if I should mention the metadata topic/KRaft in the javadoc. I did for now, but let me know if I should change it.
--
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 #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608863873
##########
File path: core/src/main/scala/kafka/tools/TestRaftServer.scala
##########
@@ -52,6 +52,7 @@ class TestRaftServer(
import kafka.tools.TestRaftServer._
private val partition = new TopicPartition("__cluster_metadata", 0)
+ private val topicId = Uuid.METADATA_TOPIC_ID
Review comment:
Yes, we need a consistent value so that we can run the test raft server as a raft cluster.
--
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] jolshan commented on a change in pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608181552
##########
File path: clients/src/main/java/org/apache/kafka/common/Uuid.java
##########
@@ -27,6 +27,10 @@
*/
public class Uuid implements Comparable<Uuid> {
+ /**
+ * A UUID for the metadata topic in KRaft mode. Will never be returned by the randomUuid method.
+ */
+ public static final Uuid SENTINEL_ID = new Uuid(0L, 1L);
Review comment:
As of now, no other contexts. I'm good with changing it.
--
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] jolshan commented on a change in pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608181552
##########
File path: clients/src/main/java/org/apache/kafka/common/Uuid.java
##########
@@ -27,6 +27,10 @@
*/
public class Uuid implements Comparable<Uuid> {
+ /**
+ * A UUID for the metadata topic in KRaft mode. Will never be returned by the randomUuid method.
+ */
+ public static final Uuid SENTINEL_ID = new Uuid(0L, 1L);
Review comment:
As of now, no. I'm good with changing it.
--
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 #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #10492:
URL: https://github.com/apache/kafka/pull/10492
--
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] jolshan commented on a change in pull request #10492: KAFKA-12457: Add sentinel ID to metadata topic
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10492:
URL: https://github.com/apache/kafka/pull/10492#discussion_r608861438
##########
File path: core/src/main/scala/kafka/tools/TestRaftServer.scala
##########
@@ -52,6 +52,7 @@ class TestRaftServer(
import kafka.tools.TestRaftServer._
private val partition = new TopicPartition("__cluster_metadata", 0)
+ private val topicId = Uuid.METADATA_TOPIC_ID
Review comment:
I was wondering about this too. In another test, (for MockLog) I used a random Uuid. Is there something we get from the hardcoded one vs random?
--
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