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/03/09 18:44:33 UTC
[GitHub] [kafka] jolshan opened a new pull request #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
jolshan opened a new pull request #10286:
URL: https://github.com/apache/kafka/pull/10286
Changes system tests to support both ZK and raft topic IDs. Clarifies the IBP check is for zk code.
Ran `upgrade_test.py` and `downgrade_test.py` as these both use the `all_nodes_support_topic_ids` and/or `topic_id` methods. Currently does not include tests using raft, but verified that the output for describe topics in raft code returns same output format and contains a valid topic ID.
### 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] rondagostino commented on a change in pull request #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#discussion_r590754821
##########
File path: tests/kafkatest/services/kafka/kafka.py
##########
@@ -1120,7 +1121,7 @@ def run_cli_tool(self, node, cmd):
def parse_describe_topic(self, topic_description):
"""Parse output of kafka-topics.sh --describe (or describe_topic() method above), which is a string of form
- PartitionCount:2\tReplicationFactor:2\tConfigs:
+ Topic:test_topic\tTopicId:<topic_id>\tPartitionCount:2\tReplicationFactor:2\tConfigs:
Review comment:
Add the spaces? (e.g. `Topic: test_topic\tTopicId: <topic_id>\tPartitionCount: 2\tReplicationFactor: 2\tConfigs:`
Also, in next line, `s/tPartition/ Partition/` I think.
##########
File path: tests/kafkatest/version.py
##########
@@ -101,7 +101,7 @@ def kafka_configs_command_uses_bootstrap_server_scram(self):
# User SCRAM Credentials (KIP-554)
return self >= V_2_7_0
- def supports_topic_ids(self):
+ def zk_supports_topic_ids(self):
Review comment:
Maybe `supports_topic_ids_when_using_zk` is a better name since it isn't ZooKeeper that is supporting topic IDs?
----------------------------------------------------------------
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] cmccabe commented on pull request #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#issuecomment-803001623
KIP-500 doesn't really run on IBP 2.7 since AlterIsr would be de-activated.
Well, technically you could run 2.7. internal version 2 to get it, but we don't test internal versions in ducktape (they don't appear in `version.py`) and I don't think they're much used in production either...
--
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] cmccabe commented on pull request #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#issuecomment-802993352
Hmm, I'm not sure why we want to rename `supports_topic_ids` to `supports_topic_ids_when_using_zk`. KIP-500 mode is only supported from 2.8 on, and does fully support topic ids.
--
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] cmccabe edited a comment on pull request #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
cmccabe edited a comment on pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#issuecomment-803001623
KIP-500 doesn't really run on IBP 2.7 or earlier since AlterIsr would be de-activated.
Well, technically you could run 2.7. internal version 2 to get it, but we don't test internal versions in ducktape (they don't appear in `version.py`) and I don't think they're much used in production either...
--
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 #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#discussion_r590800123
##########
File path: tests/kafkatest/services/kafka/kafka.py
##########
@@ -1120,7 +1121,7 @@ def run_cli_tool(self, node, cmd):
def parse_describe_topic(self, topic_description):
"""Parse output of kafka-topics.sh --describe (or describe_topic() method above), which is a string of form
- PartitionCount:2\tReplicationFactor:2\tConfigs:
+ Topic:test_topic\tTopicId:<topic_id>\tPartitionCount:2\tReplicationFactor:2\tConfigs:
Review comment:
Makes sense.
----------------------------------------------------------------
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] cmccabe commented on pull request #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#issuecomment-803011549
That's a fair point.
Can you add a comment to `supports_topic_ids_when_using_zk` explaining that self-managed clusters always support topic IDs?
LGTM after that.
--
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 #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#issuecomment-802995122
@cmccabe there's a small distinction in that kip-500 can run on ibp < 2.8, where zk IDs are created only when ibp is 2.8 or higher.
My understanding is that if we are using kip-500 mode, then we have topic IDs. This is the logic behind `all_nodes_support_topic_ids`
--
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 #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
jolshan commented on a change in pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#discussion_r590798792
##########
File path: tests/kafkatest/version.py
##########
@@ -101,7 +101,7 @@ def kafka_configs_command_uses_bootstrap_server_scram(self):
# User SCRAM Credentials (KIP-554)
return self >= V_2_7_0
- def supports_topic_ids(self):
+ def zk_supports_topic_ids(self):
Review comment:
I was having trouble naming this haha. This is a better name.
----------------------------------------------------------------
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] cmccabe merged pull request #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #10286:
URL: https://github.com/apache/kafka/pull/10286
--
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 #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#issuecomment-796963473
System tests `upgrade_test.py` and `downgrade_test.py` passed with latest code.
----------------------------------------------------------------
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 #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#issuecomment-803007546
I think ducktape uses `ApiVersion` and `assertEquals(KAFKA_2_7_IV2, ApiVersion("2.7"))` succeeds.
--
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 #10286: KAFKA-12318: system tests need to fetch Topic IDs via Admin Client instead of via ZooKeeper
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10286:
URL: https://github.com/apache/kafka/pull/10286#issuecomment-803005922
@cmccabe I think ducktape runs on the latest IBP for the release. So it would run 2.7-IV2
--
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