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/06/17 18:22:02 UTC
[GitHub] [kafka] andymg3 opened a new pull request, #12305: MINOR: Add __cluster_metadata topic to list of internal topics
andymg3 opened a new pull request, #12305:
URL: https://github.com/apache/kafka/pull/12305
### Details
- Adds the new KRaft `__cluster_metadata` topic to `INTERNAL_TOPICS` which tracks internal topics
### 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r901666678
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
Please correct me if I am wrong here Andy but this is true only for kraft mode. I haven't tried it but in the zookeeper mode, it should be possible to create a topic with this name as a non-internal topic.
--
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] andymg3 commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
andymg3 commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r933670311
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
@showuon @divijvaidya @hachikuji I took a little into adding a conditional check. One thing that is tricky is the `isInternal` method (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/internals/Topic.java#L59) is used client side. For example, it’s used in TopicCommand.scala (https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/TopicCommand.scala#L427). I don’t think it makes sense for the client to be in KRaft or ZK mode. During an upgrade its possible there’s a mix or KRaft and ZK brokers, for example. Furthermore, I can’t image we’d expect the client to provide metadata for whether we’re in KRaft or ZK mode so I don’t know how we could know what mode we’re even in.
Thus, I’m not sure how we can correctly handle sometimes including the the __cluster_metadata topic and sometimes not. As I mentioned above, it seems possible we will have this issue again in the future. Doesn’t seem unlikely new internal topics will be created.
My feeling is we should include __cluster_metadata as an internal topic and document it as a breaking change as such. Other solutions that start involving the client feel overly complex. Open to thoughts and/or any other better ideas folks might have though.
--
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] hachikuji commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
hachikuji commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r912151174
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
I think the change makes sense. Eventually zk clusters will have to upgrade. On the other hand, for pre-existing topics which match the same name, I am not sure what we should do. Probably we have to catch that case in the upgrade.
--
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] andymg3 commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
andymg3 commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r913275951
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
Apologies for the late reply @divijvaidya. I think it's a good point about the compatibility issue. If a user is relying on `isInternal` and they have a topic `__cluster_metadata` on an existing ZK cluster we do break them. We could wrap the code in a conditional check I think. @hachikuji thoughts? At first I thought this sounded a bit overkill but ultimately we dont know if folks are relying on this behavior and how critical it is to their applications. For upgrading from ZK to KRaft it seems like we'd have to handle the topic already existing somehow. But that issue is a bit separate from what @divijvaidya is concerned about (please correct me if I'm wrong) I think.
This makes me wonder how we would handle other topics that we want to mark as internal in the future. It feels like maybe Kafka could/should reserve topic names that start with two underscores? Obviously this could also break applications, and would have to be thought through a lot and done carefully.
--
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] andymg3 commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
andymg3 commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r933670311
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
@showuon @divijvaidya @hachikuji I took a look into adding a conditional check. One thing that is tricky is the `isInternal` method (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/internals/Topic.java#L59) is used client side. For example, it’s used in TopicCommand.scala (https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/TopicCommand.scala#L427). I don’t think it makes sense for the client to be in KRaft or ZK mode. During an upgrade its possible there’s a mix or KRaft and ZK brokers, for example. Furthermore, I can’t imagine we’d expect the client to provide metadata for whether we’re in KRaft or ZK mode so I don’t know how we could know what mode we’re even in.
Thus, I’m not sure how we can correctly handle sometimes including the the __cluster_metadata topic and sometimes not. As I mentioned above, it seems possible we will have this issue again in the future. Doesn’t seem unlikely new internal topics will be created.
My feeling is we should include __cluster_metadata as an internal topic and document it as a breaking change as such. Other solutions that start involving the client feel overly complex. Open to thoughts and/or any other better ideas folks might have though.
--
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] andymg3 commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
andymg3 commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r913275951
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
Apologies for the late reply @divijvaidya. I think it's a good point about the compatibility issue. If a user is relying on `isInternal` and they have a topic `__cluster_metadata` on an existing ZK cluster we do break them. We could wrap the code in a conditional check I think. @hachikuji thoughts? At first I thought this sounded a bit overkill but ultimately we dont know if folks are relying on this behavior and how critical it is to their applications. For upgrading from ZK to KRaft it seems like we'd have to handle the topic already existing somehow. But that issue is a bit separate from what @divijvaidya is concerned about (please correct me if I'm wrong) I think.
This makes me wonder how we would handle other topics that we want to mark as internal in the future. It feels like maybe Kafka could/should reserve topic names that start with two underscores? Obviously this could also break applications, and would have to be thought through a lot and done carefully. Not sure if this has been discussed before.
--
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] andymg3 commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
andymg3 commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r913275951
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
Apologies for the late reply @divijvaidya. I think it's a good point about the compatibility issue. If a customer is relying on `isInternal` and they have a topic `__cluster_metadata` on an existing ZK cluster we do break them. We could wrap the code in a conditional check I think. @hachikuji thoughts? At first I thought this sounded a bit overkill but ultimately we dont know if folks are relying on this behavior and how critical it is to their applications. For upgrading from ZK to KRaft it seems like we'd have to handle the topic already existing somehow. But that issue is a bit separate from what @divijvaidya is concerned about (please correct me if I'm wrong) I think.
This makes me wonder how we would handle other topics that we want to mark as internal in the future. It feels like maybe Kafka could/should reserve topic names that start with two underscores? Obviously this could also break applications, and would have to be thought through a lot and done carefully.
--
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] divijvaidya commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r912780444
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
> Eventually zk clusters will have to upgrade.
My concern is that if we add this change to the current or upcoming version (3.3.x), we would break the user who may have a zk cluster with the topic having this name. @hachikuji Is that breaking change something that we are willing to accept given that we support zk in these versions? In the interest of backward compatibility for zk (until it is completely removed), this change should go in a version where only kraft mode is possible.
We should probably wrap this change in a Raft conditional check, i.e. add this topic to internal topic list if kraft is enabled.
--
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] showuon commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r913346863
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
> We could wrap the code in a conditional check I think
+1, and we should log warning about in ZK mode if the `__cluster_metadata` topic exists about the topic will be seen as internal topic after upgrading to Kraft mode.
--
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] andymg3 commented on a diff in pull request #12305: MINOR: Add __cluster_metadata topic to list of internal topics
Posted by GitBox <gi...@apache.org>.
andymg3 commented on code in PR #12305:
URL: https://github.com/apache/kafka/pull/12305#discussion_r933670311
##########
clients/src/main/java/org/apache/kafka/common/internals/Topic.java:
##########
@@ -33,7 +33,7 @@ public class Topic {
public static final String LEGAL_CHARS = "[a-zA-Z0-9._-]";
private static final Set<String> INTERNAL_TOPICS = Collections.unmodifiableSet(
- Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME));
+ Utils.mkSet(GROUP_METADATA_TOPIC_NAME, TRANSACTION_STATE_TOPIC_NAME, METADATA_TOPIC_NAME));
Review Comment:
@showuon @divijvaidya @hachikuji I took a look into adding a conditional check. One thing that is tricky is the `isInternal` method (https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/internals/Topic.java#L59) is used client side. For example, it’s used in TopicCommand.scala (https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/TopicCommand.scala#L427). I don’t think it makes sense for the client to be in KRaft or ZK mode. During an upgrade its possible there’s a mix or KRaft and ZK brokers, for example. Furthermore, I can’t imagine we’d expect the client to provide metadata for whether we’re in KRaft or ZK mode so I don’t know how we could know what mode we’re even in.
Thus, I’m not sure how we can correctly handle sometimes including the __cluster_metadata topic and sometimes not. As I mentioned above, it seems possible we will have this issue again in the future. Doesn’t seem unlikely new internal topics will be created.
My feeling is we should include __cluster_metadata as an internal topic and document it as a breaking change as such. Other solutions that start involving the client feel overly complex. Open to thoughts and/or any other better ideas folks might have though.
--
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