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/06/03 12:15:03 UTC
[GitHub] [kafka] showuon opened a new pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
showuon opened a new pull request #10811:
URL: https://github.com/apache/kafka/pull/10811
Checked the documentation, we must use `--zookeeper` option in 2 places:
1. [config SCRAM Credentials](https://kafka.apache.org/documentation/#security_sasl_scram_credentials)
2. [Update password config before broker started](https://kafka.apache.org/documentation/#dynamicbrokerconfigs)
So, after this PR, we only support `--zookeeper` on `users` and `brokers` entity. Add some argument parse rules and tests.
### 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] ijuma commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661514537
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,39 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "ips",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusAlterUserQuotaWithoutEntityName(): Unit = {
Review comment:
But the CLI can verify if you are trying to update SCRAM credentials vs updating quotas. We should not allow the latter. Is your point that describe should continue to work?
--
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] rajinisivaram commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-869699660
@ijuma Yes, those two cases would still require ZooKeeper for bootstrapping broker credentials/broker configs. I think there was some discussion around bootstrapping cases, @rondagostino would probably know what the plan is.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661411622
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,39 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "ips",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusAlterUserQuotaWithoutEntityName(): Unit = {
Review comment:
> I was thinking this test should be exactly like the clients entity case, why did we make it different?
Do you mean this test case?
```java
@Test
def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
assertNonZeroStatusExit(Array(
"--zookeeper", zkConnect,
"--entity-type", "clients",
"--describe"))
}
```
If so, it's because `users` entity can still accept to use ZK because we need it to create/update the SCRAM credentials.
If this is not what you meant, please kindly let me know. Thank you. :)
--
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] rondagostino commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-872258415
> require zk-tls-config-file option when zookeeper is provided
I think this may be too much of a constraint. While it is true that these operations should be using TLS, it is possible that some installations may depend upon network segmentation exclusively for security.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661098484
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -68,7 +68,7 @@ object ConfigCommand extends Config {
val BrokerDefaultEntityName = ""
val BrokerLoggerConfigType = "broker-loggers"
val BrokerSupportedConfigTypes = ConfigType.all :+ BrokerLoggerConfigType
- val ZkSupportedConfigTypes = ConfigType.all
+ val ZkSupportedConfigTypes = Seq(ConfigType.User, ConfigType.Broker)
Review comment:
If you're saying this comment:
> // Dynamic broker configs can only be updated using the new AdminClient once brokers have started
// so that configs may be fully validated. Prior to starting brokers, updates may be performed using
// ZooKeeper for bootstrapping. This allows all password configs to be stored encrypted in ZK,
// avoiding clear passwords in server.properties. For consistency with older versions, quota-related
// broker configs can still be updated using ZooKeeper at any time.
I checked and confirmed that is saying these 3 configs of "broker" type (not "quota" type):
```
val BrokerConfigsUpdatableUsingZooKeeperWhileBrokerRunning = Set(
DynamicConfig.Broker.LeaderReplicationThrottledRateProp,
DynamicConfig.Broker.FollowerReplicationThrottledRateProp,
DynamicConfig.Broker.ReplicaAlterLogDirsIoMaxBytesPerSecondProp)
```
--
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] ijuma edited a comment on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
ijuma edited a comment on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-885004599
@showuon I'm seeing the following in the system test `upgrade_test` (from_kafka_version=0.9.0.1.to_message_format_version=0.9.0.1.compression_types=.none):
> INFO - 2021-07-22 08:16:06,976 - runner_client - log - lineno:241]: RunnerClient: kafkatest.tests.core.upgrade_test.TestUpgrade.test_upgrade.from_kafka_version=0.9.0.1.to_message_format_version=0.9.0.1.compression_types=.none: FAIL: Rem
> oteCommandError({'ssh_config': {'host': 'ducker11', 'hostname': 'ducker11', 'user': 'ducker', 'port': 22, 'password': '', 'identityfile': '/home/ducker/.ssh/id_rsa'}, 'hostname': 'ducker11', 'ssh_hostname': 'ducker11', 'user': 'ducker', '
> externally_routable_ip': 'ducker11', '_logger': <Logger kafkatest.tests.core.upgrade_test.TestUpgrade.test_upgrade.from_kafka_version=0.9.0.1.to_message_format_version=0.9.0.1.compression_types=.none-15 (DEBUG)>, 'os': 'linux', '_ssh_clie
> nt': <paramiko.client.SSHClient object at 0x7f7a45174f28>, '_sftp_client': <paramiko.sftp_client.SFTPClient object at 0x7f7a441069b0>}, '/opt/kafka-dev/bin/kafka-run-class.sh kafka.admin.ConfigCommand --zookeeper ducker11:2181 --describe
> --topic test_topic', 1, b'')
> Traceback (most recent call last):
> File "/usr/local/lib/python3.7/dist-packages/ducktape/tests/runner_client.py", line 133, in run
> data = self.run_test()
> File "/usr/local/lib/python3.7/dist-packages/ducktape/tests/runner_client.py", line 190, in run_test
> return self.test_context.function(self.test)
> File "/usr/local/lib/python3.7/dist-packages/ducktape/mark/_mark.py", line 429, in wrapper
> return functools.partial(f, *args, **kwargs)(*w_args, **w_kwargs)
> File "/opt/kafka-dev/tests/kafkatest/tests/core/upgrade_test.py", line 199, in test_upgrade
> cluster_id = self.kafka.cluster_id()
> File "/opt/kafka-dev/tests/kafkatest/tests/produce_consume_validate.py", line 105, in run_produce_consume_validate
> core_test_action(*args)
> File "/opt/kafka-dev/tests/kafkatest/tests/core/upgrade_test.py", line 200, in <lambda>
> assert cluster_id is not None
> File "/opt/kafka-dev/tests/kafkatest/tests/core/upgrade_test.py", line 57, in perform_upgrade
> self.zk.describe(self.topic)
> File "/opt/kafka-dev/tests/kafkatest/services/zookeeper.py", line 234, in describe
> output = self.nodes[0].account.ssh_output(cmd)
> File "/usr/local/lib/python3.7/dist-packages/ducktape/cluster/remoteaccount.py", line 370, in ssh_output
> raise RemoteCommandError(self, cmd, exit_status, stderr.read())
> ducktape.cluster.remoteaccount.RemoteCommandError: ducker@ducker11: Command '/opt/kafka-dev/bin/kafka-run-class.sh kafka.admin.ConfigCommand --zookeeper ducker11:2181 --describe --topic test_topic' returned non-zero exit status 1.
Is this related to this PR?
--
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 pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882996715
@ijuma , thanks for the update. It looks better now! Also, thank you and @rondagostino for your patiently review!
For your question:
> it looks to me that we don't have test coverage in ConfigCommandTest for the case where we try to update broker configs via zk when the brokers are up, is that right?
I actually added a test for it:
`shouldNotAllowAddBrokerQuotaConfigWhileBrokerUpUsingZookeeper` [here](https://github.com/apache/kafka/blob/da252c633d88ecec6f68200081ad94a3081e5f35/core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala#L905)
And this test is to test update multiple broker configs using zookeeper (when no brokers up)
`testDynamicBrokerConfigUpdateUsingZooKeeper` [here](https://github.com/apache/kafka/blob/da252c633d88ecec6f68200081ad94a3081e5f35/core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala#L1245)
Do you think we should add more tests for 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.
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r644741927
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -731,10 +731,11 @@ object ConfigCommand extends Config {
class ConfigCommandOptions(args: Array[String]) extends CommandDefaultOptions(args) {
val zkConnectOpt = parser.accepts("zookeeper", "DEPRECATED. The connection string for the zookeeper connection in the form host:port. " +
- "Multiple URLS can be given to allow fail-over. Replaced by --bootstrap-server, REQUIRED unless --bootstrap-server is given.")
- .withRequiredArg
- .describedAs("urls")
- .ofType(classOf[String])
+ "Multiple URLS can be given to allow fail-over. Replaced by --bootstrap-server except configuring SCRAM credentials for user or " +
+ "password configs before starting brokers. REQUIRED unless --bootstrap-server is given.")
Review comment:
update the description for `zookeeper` option.
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -876,6 +877,10 @@ object ConfigCommand extends Config {
throw new IllegalArgumentException(s"--bootstrap-server must be specified for --all")
}
+ if (options.has(zkTlsConfigFile) && options.has(bootstrapServerOpt)) {
+ throw new IllegalArgumentException(s"--zookeeper must be specified for --zk-tls-config-file")
Review comment:
This check is missed before. Added.
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -876,6 +877,10 @@ object ConfigCommand extends Config {
throw new IllegalArgumentException(s"--bootstrap-server must be specified for --all")
}
+ if (options.has(zkTlsConfigFile) && options.has(bootstrapServerOpt)) {
+ throw new IllegalArgumentException(s"--zookeeper must be specified for --zk-tls-config-file")
Review comment:
This check is missed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-885022501
Yeah, it looks like we call `self.zk.describe()` in 3 system tests; the one above and also in `zookeeper_tls_encrypt_only_test.py` and `zookeeper_tls_test.py`. The latter two are simply confirming that the `--zk-tls-config-file` parameter will work with `kafka-configs.sh` (i.e. that it can talk to TLS-enabled ZooKeeper nodes), so we could easily change those to do something with SCRAM configs.
The one mentioned here is simply trying to surface an error as early as possible as per the comment:
```
# Confirm we have a successful ZooKeeper upgrade by describing the topic.
# Not trying to detect a problem here leads to failure in the ensuing Kafka roll, which would be a less
# intuitive failure than seeing a problem here, so detect ZooKeeper upgrade problems before involving Kafka.
self.zk.describe(self.topic)
```
So we could either just get rid of it or maybe list ACLs/do something with SCRAM.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r644742376
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -876,6 +877,10 @@ object ConfigCommand extends Config {
throw new IllegalArgumentException(s"--bootstrap-server must be specified for --all")
}
+ if (options.has(zkTlsConfigFile) && options.has(bootstrapServerOpt)) {
+ throw new IllegalArgumentException(s"--zookeeper must be specified for --zk-tls-config-file")
Review comment:
This check is missed before. Added.
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -876,6 +877,10 @@ object ConfigCommand extends Config {
throw new IllegalArgumentException(s"--bootstrap-server must be specified for --all")
}
+ if (options.has(zkTlsConfigFile) && options.has(bootstrapServerOpt)) {
+ throw new IllegalArgumentException(s"--zookeeper must be specified for --zk-tls-config-file")
Review comment:
This check is missed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882171897
@ijuma , thanks for help update the comment and messages. :)
Failed tests are unrelated:
```
Build / ARM / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / ARM / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()
Build / JDK 11 and Scala 2.13 / kafka.server.LogDirFailureTest.testProduceErrorFromFailureOnCheckpoint()
Build / JDK 16 and Scala 2.13 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 16 and Scala 2.13 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
```
The `testResolveDnsLookup` failed should be because of infra's issue. It didn't fail in the previous and older PR build, and the latest update is only comment and message update. Besides, it also failed in another PR build that triggered 7 hours ago [here](https://github.com/apache/kafka/pull/11077/checks?check_run_id=3098846634).
Thank you.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661161241
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -68,14 +68,13 @@ object ConfigCommand extends Config {
val BrokerDefaultEntityName = ""
val BrokerLoggerConfigType = "broker-loggers"
val BrokerSupportedConfigTypes = ConfigType.all :+ BrokerLoggerConfigType
- val ZkSupportedConfigTypes = ConfigType.all
+ val ZkSupportedConfigTypes = Seq(ConfigType.User, ConfigType.Broker)
val DefaultScramIterations = 4096
// Dynamic broker configs can only be updated using the new AdminClient once brokers have started
// so that configs may be fully validated. Prior to starting brokers, updates may be performed using
// ZooKeeper for bootstrapping. This allows all password configs to be stored encrypted in ZK,
// avoiding clear passwords in server.properties. For consistency with older versions, quota-related
- // broker configs can still be updated using ZooKeeper at any time. ConfigCommand will be migrated
- // to the new AdminClient later for these configs (KIP-248).
Review comment:
I've checked and confirmed that KIP-248 is rejected now. It won't be implemented. So, remove 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.
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 edited a comment on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-877736694
@rondagostino , thanks for comments and many good catch! I've updated the PR.
> The PR as currently written restricts describing configs via --zookeeper to user and broker configs. Unlike with altering, it does not perform a check to see if the broker is not started. Do we wish to add such a check?
Good suggestion! Make sense to me. I added the check for broker type describe via `--zookeeper`, and add tests.
Thank you.
--
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] ijuma commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661276110
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -68,14 +68,13 @@ object ConfigCommand extends Config {
val BrokerDefaultEntityName = ""
val BrokerLoggerConfigType = "broker-loggers"
val BrokerSupportedConfigTypes = ConfigType.all :+ BrokerLoggerConfigType
- val ZkSupportedConfigTypes = ConfigType.all
+ val ZkSupportedConfigTypes = Seq(ConfigType.User, ConfigType.Broker)
val DefaultScramIterations = 4096
// Dynamic broker configs can only be updated using the new AdminClient once brokers have started
// so that configs may be fully validated. Prior to starting brokers, updates may be performed using
// ZooKeeper for bootstrapping. This allows all password configs to be stored encrypted in ZK,
// avoiding clear passwords in server.properties. For consistency with older versions, quota-related
- // broker configs can still be updated using ZooKeeper at any time. ConfigCommand will be migrated
- // to the new AdminClient later for these configs (KIP-248).
Review comment:
Hmm, the comment says that quota related broker configs can still be updated using ZooKeeper at any time. Why is that? I think we should remove this.
--
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] ijuma commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-861513235
@showuon I'll take a look this week.
--
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] showuon commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r660562175
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "Ips",
Review comment:
Nice catch! Will update. Thanks.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661098484
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -68,7 +68,7 @@ object ConfigCommand extends Config {
val BrokerDefaultEntityName = ""
val BrokerLoggerConfigType = "broker-loggers"
val BrokerSupportedConfigTypes = ConfigType.all :+ BrokerLoggerConfigType
- val ZkSupportedConfigTypes = ConfigType.all
+ val ZkSupportedConfigTypes = Seq(ConfigType.User, ConfigType.Broker)
Review comment:
If you're saying this comment:
> // Dynamic broker configs can only be updated using the new AdminClient once brokers have started
// so that configs may be fully validated. Prior to starting brokers, updates may be performed using
// ZooKeeper for bootstrapping. This allows all password configs to be stored encrypted in ZK,
// avoiding clear passwords in server.properties. For consistency with older versions, quota-related
// broker configs can still be updated using ZooKeeper at any time.
I checked and confirmed that it is saying these 3 configs of "broker" type (not "quota" type):
```
val BrokerConfigsUpdatableUsingZooKeeperWhileBrokerRunning = Set(
DynamicConfig.Broker.LeaderReplicationThrottledRateProp,
DynamicConfig.Broker.FollowerReplicationThrottledRateProp,
DynamicConfig.Broker.ReplicaAlterLogDirsIoMaxBytesPerSecondProp)
```
--
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] ijuma merged pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #10811:
URL: https://github.com/apache/kafka/pull/10811
--
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] ijuma commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-883408956
@showuon Thanks for the clarification. Maybe we just need one like `shouldNotAllowAddBrokerQuotaConfigWhileBrokerUpUsingZookeeper` for the entity-default 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
[GitHub] [kafka] ijuma commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-885055345
@showuon can you please file a blocker Jira for 3.0? Do you have cycles to fix this?
--
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] ijuma commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882984058
--
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-877736694
@rondagostino , thanks for comments and many good catch! I've updated the PR.
> The PR as currently written restricts describing configs via --zookeeper to user and broker configs. Unlike with altering, it does not perform a check to see if the broker is not started. Do we wish to add such a check?
Good suggestion! Make sense to me. I added the check for broker describe via `--zookeeper`, and add tests.
Thank you.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r667417274
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -236,9 +240,17 @@ object ConfigCommand extends Config {
}
}
- private def describeConfigWithZk(zkClient: KafkaZkClient, opts: ConfigCommandOptions, adminZkClient: AdminZkClient): Unit = {
+ private[admin] def describeConfigWithZk(zkClient: KafkaZkClient, opts: ConfigCommandOptions, adminZkClient: AdminZkClient): Unit = {
val configEntity = parseEntity(opts)
- val describeAllUsers = configEntity.root.entityType == ConfigType.User && !configEntity.root.sanitizedName.isDefined && !configEntity.child.isDefined
+ val entityType = configEntity.root.entityType
+ val describeAllUsers = entityType == ConfigType.User && !configEntity.root.sanitizedName.isDefined && !configEntity.child.isDefined
+ val entityName = configEntity.fullSanitizedName
+ val errorMessage = s"--bootstrap-server option must be specified to describe $entityType"
+ if (entityType == ConfigType.Broker) {
+ // Dynamic broker configs can be described using ZooKeeper only if the corresponding broker is not running.
+ validateBrokersNotRunning(entityName, adminZkClient, zkClient, errorMessage)
Review comment:
Make sure brokers not running for describe via --zookeeper
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r662866236
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -132,17 +122,22 @@ object ConfigCommand extends Config {
val entity = parseEntity(opts)
val entityType = entity.root.entityType
val entityName = entity.fullSanitizedName
-
- if (entityType == ConfigType.User)
+ val errorMessage = s"--bootstrap-server option must be specified to update $entityType configs $configsToBeAdded $configsToBeDeleted."
+
+ if (entityType == ConfigType.User) {
+ if (!configsToBeAdded.isEmpty || !configsToBeDeleted.isEmpty) {
+ val info = "User configuration updates using ZooKeeper are only supported for SCRAM credential updates."
+ // make sure every added/deleted configs are SCRAM related, other configs are not supported using zookeeper
+ require(configsToBeAdded.keys().asScala.forall(propertyKey => ScramMechanism.values().map(_.mechanismName()).contains(propertyKey.toString)),
+ s"$errorMessage $info")
+ require(configsToBeDeleted.forall(propertyKey => ScramMechanism.values().map(_.mechanismName()).contains(propertyKey)),
+ s"$errorMessage $info")
Review comment:
Add a `user` type check to only allow SCRAM credential updates.
--
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] rondagostino commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-870738717
Regarding the bootstrap use cases, these were enumerated in [KIP-515: Enable ZK client to use the new TLS supported authentication](https://cwiki.apache.org/confluence/display/KAFKA/KIP-515%3A+Enable+ZK+client+to+use+the+new+TLS+supported+authentication) when we added ZooKeeper TLS, and while that was a while ago, yes, it is still necessary to be able to bootstrap configs prior to starting Kafka.
--
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-872830334
@rondagostino, thanks for the comments. I've removed the constraint for the `zk-tls-config-file`. Also, I added some constraint to only allow `user` type update SCRAM related configs (and add tests). Thank you.
--
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] ijuma commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882989575
Merged to master and cherry-picking to 3.0.
--
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 pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882996715
@ijuma , thanks for the update. It looks better now! Also, thank you and @rondagostino for your patiently review!
For your question:
> it looks to me that we don't have test coverage in ConfigCommandTest for the case where we try to update broker configs via zk when the brokers are up, is that right?
I actually added a test for it:
`shouldNotAllowAddBrokerQuotaConfigWhileBrokerUpUsingZookeeper` [here](https://github.com/apache/kafka/blob/da252c633d88ecec6f68200081ad94a3081e5f35/core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala#L905)
And this test is to test update multiple broker configs using zookeeper (when no brokers up)
`testDynamicBrokerConfigUpdateUsingZooKeeper` [here](https://github.com/apache/kafka/blob/da252c633d88ecec6f68200081ad94a3081e5f35/core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala#L1245)
Do you think we should add more tests for 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.
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r644741927
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -731,10 +731,11 @@ object ConfigCommand extends Config {
class ConfigCommandOptions(args: Array[String]) extends CommandDefaultOptions(args) {
val zkConnectOpt = parser.accepts("zookeeper", "DEPRECATED. The connection string for the zookeeper connection in the form host:port. " +
- "Multiple URLS can be given to allow fail-over. Replaced by --bootstrap-server, REQUIRED unless --bootstrap-server is given.")
- .withRequiredArg
- .describedAs("urls")
- .ofType(classOf[String])
+ "Multiple URLS can be given to allow fail-over. Replaced by --bootstrap-server except configuring SCRAM credentials for user or " +
+ "password configs before starting brokers. REQUIRED unless --bootstrap-server is given.")
Review comment:
update the description for `zookeeper` option.
--
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] showuon commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r667417221
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -236,9 +240,17 @@ object ConfigCommand extends Config {
}
}
- private def describeConfigWithZk(zkClient: KafkaZkClient, opts: ConfigCommandOptions, adminZkClient: AdminZkClient): Unit = {
+ private[admin] def describeConfigWithZk(zkClient: KafkaZkClient, opts: ConfigCommandOptions, adminZkClient: AdminZkClient): Unit = {
Review comment:
visible for testing
--
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 edited a comment on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-877717089
@ijuma , I've uploaded the PR. Please take a look again. Thanks.
Failed tests are unrelated.
```
Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()
Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()
Build / JDK 16 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()
Build / JDK 16 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()
Build / JDK 8 and Scala 2.12 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()
Build / JDK 8 and Scala 2.12 / kafka.api.TransactionsTest.testSendOffsetsToTransactionTimeout()
Build / JDK 8 and Scala 2.12 / kafka.api.TransactionsTest.testSendOffsetsToTransactionTimeout()
```
--
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-860173898
@ijuma , please help review. Thanks.
--
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] showuon commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r660562020
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
Review comment:
Will do. Thanks.
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "Ips",
Review comment:
Nice catch! Will update. Thanks.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r667309248
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -876,6 +872,11 @@ object ConfigCommand extends Config {
throw new IllegalArgumentException(s"--bootstrap-server must be specified for --all")
}
+ if (options.has(zkTlsConfigFile) && options.has(bootstrapServerOpt)) {
+ throw new IllegalArgumentException("--bootstrap-server doesn't support --zk-tls-config-file option. " +
+ "Please use --zookeeper option instead")
Review comment:
Good suggestion! Updated the message to:
`--bootstrap-server doesn't support --zk-tls-config-file option. If you are trying to update the ZooKeeper TLS configuration, please use --zookeeper option instead. Otherwise, Please remove --zk-tls-config-file option.`
--
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] rondagostino commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r667389229
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -133,13 +183,13 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
}
@Test
- def shouldParseArgumentsForIpEntityType(): Unit = {
- testArgumentParse("ips", zkConfig = false)
+ def shouldParseArgumentsForIpEntityTypeUsingZookeeper(): Unit = {
Review comment:
s/shouldParse/shouldFailParse/
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -118,7 +168,7 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
}
@Test
- def shouldParseArgumentsForBrokersEntityTypeUsingZookeeper(): Unit = {
+ def shouldFailParseArgumentsForBrokersEntityTypeUsingZookeeper(): Unit = {
Review comment:
I think this name change is incorrect -- it still parses correctly
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -1399,6 +1454,11 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
ConfigCommand.alterConfigWithZk(null, del512, CredentialChange("userB", Set(), 4096))
}
+ @Test
+ def testQuotaConfigEntityUsingZookeeper(): Unit = {
Review comment:
s/testQuotaConfigEntityUsingZookeeper/testQuotaConfigEntityUsingZookeeperNotAllowed/
##########
File path: docs/upgrade.html
##########
@@ -46,8 +46,10 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
<code>AclBindingFilter</code>.</li>
<li>The <code>Admin.electedPreferredLeaders()</code> methods were removed. Please use <code>Admin.electLeaders</code> instead.</li>
<li>The <code>kafka-preferred-replica-election</code> command line tool was removed. Please use <code>kafka-leader-election</code> instead.</li>
- <li>The <code>--zookeeper</code> option was removed from the <code>bin/kafka-topics.sh</code> and <code>bin/kafka-reassign-partitions.sh</code> command line tools.
+ <li>The <code>--zookeeper</code> option was removed from the <code>kafka-topics</code> and <code>kafka-reassign-partitions</code> command line tools.
Please use <code>--bootstrap-server</code> instead.</li>
+ <li>In <code>kafka-configs</code> command line tool, the <code>--zookeeper</code> option only supported for <a href="#security_sasl_scram_credentials">SCRAM Credentials configuration</a>
+ and <a href="#dynamicbrokerconfigs">update password config before broker started</a>.</li>
Review comment:
```suggestion
and <a href="#dynamicbrokerconfigs">updating dynamic broker configs before brokers are started</a>.</li>
```
##########
File path: docs/upgrade.html
##########
@@ -46,8 +46,10 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
<code>AclBindingFilter</code>.</li>
<li>The <code>Admin.electedPreferredLeaders()</code> methods were removed. Please use <code>Admin.electLeaders</code> instead.</li>
<li>The <code>kafka-preferred-replica-election</code> command line tool was removed. Please use <code>kafka-leader-election</code> instead.</li>
- <li>The <code>--zookeeper</code> option was removed from the <code>bin/kafka-topics.sh</code> and <code>bin/kafka-reassign-partitions.sh</code> command line tools.
+ <li>The <code>--zookeeper</code> option was removed from the <code>kafka-topics</code> and <code>kafka-reassign-partitions</code> command line tools.
Please use <code>--bootstrap-server</code> instead.</li>
+ <li>In <code>kafka-configs</code> command line tool, the <code>--zookeeper</code> option only supported for <a href="#security_sasl_scram_credentials">SCRAM Credentials configuration</a>
Review comment:
s/In/In the/
s/option only supported/option is only supported/
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -731,10 +726,11 @@ object ConfigCommand extends Config {
class ConfigCommandOptions(args: Array[String]) extends CommandDefaultOptions(args) {
val zkConnectOpt = parser.accepts("zookeeper", "DEPRECATED. The connection string for the zookeeper connection in the form host:port. " +
- "Multiple URLS can be given to allow fail-over. Replaced by --bootstrap-server, REQUIRED unless --bootstrap-server is given.")
- .withRequiredArg
- .describedAs("urls")
- .ofType(classOf[String])
+ "Multiple URLS can be given to allow fail-over. Replaced by --bootstrap-server except configuring SCRAM credentials for user or " +
Review comment:
s/except configuring SCRAM credentials for user or /except for when configuring SCRAM credentials for users or /
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -109,7 +159,7 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
@Test
def shouldParseArgumentsForTopicsEntityTypeUsingZookeeper(): Unit = {
Review comment:
s/shouldParse/shouldFailParse/
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -1474,13 +1534,13 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
}
@Test
- def testQuotaConfigEntityUsingZookeeper(): Unit = {
- doTestQuotaConfigEntity(zkConfig = true)
+ def testQuotaConfigEntity(): Unit = {
+ doTestQuotaConfigEntity(zkConfig = false)
}
@Test
- def testQuotaConfigEntity(): Unit = {
- doTestQuotaConfigEntity(zkConfig = false)
+ def testUserClientQuotaOptsUsingZookeeper(): Unit = {
Review comment:
s/testUserClientQuotaOptsUsingZookeeper/testUserClientQuotaOptsUsingZookeeperNotAllowed/
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -731,10 +726,11 @@ object ConfigCommand extends Config {
class ConfigCommandOptions(args: Array[String]) extends CommandDefaultOptions(args) {
val zkConnectOpt = parser.accepts("zookeeper", "DEPRECATED. The connection string for the zookeeper connection in the form host:port. " +
- "Multiple URLS can be given to allow fail-over. Replaced by --bootstrap-server, REQUIRED unless --bootstrap-server is given.")
- .withRequiredArg
- .describedAs("urls")
- .ofType(classOf[String])
+ "Multiple URLS can be given to allow fail-over. Replaced by --bootstrap-server except configuring SCRAM credentials for user or " +
+ "password configs before starting brokers. REQUIRED unless --bootstrap-server is given.")
Review comment:
s/password configs before starting brokers/dynamic broker configs before starting brokers/
--
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-871132339
@ijuma , I've updated the PR. Please take a look again. Thank you.
Failed tests are unrelated.
```
Build / JDK 16 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
Build / JDK 16 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
```
--
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-853823700
@ijuma , please help review this PR. Thanks.
--
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] showuon commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882023615
@rondagostino , I also checked the system test that used `Kafka-Configs.sh`, this PR should not failed the system tests because we only force to use zk connection when updating TLS config [here](https://github.com/apache/kafka/blob/trunk/tests/kafkatest/services/kafka/kafka.py#L947).
Thank you.
--
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] rondagostino commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-872262617
> I think this may be too much of a constraint
One counterargument to this is that the bootstrap use case is a pretty unusual one and would very likely be utilized by organizations that are quite security conscious -- and network segmentation would not be the sole security mechanism for such an organization. That being said, it would be better to avoid this constraint if another solution is available and is not too onerous to implement.
--
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] rondagostino edited a comment on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
rondagostino edited a comment on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-885022501
Yeah, it looks like we call `self.zk.describe()` in 3 system tests; the one above and also in `zookeeper_tls_encrypt_only_test.py` and `zookeeper_tls_test.py`. The latter two are simply confirming that the `--zk-tls-config-file` parameter will work with `kafka-configs.sh` (i.e. that it can talk to TLS-enabled ZooKeeper nodes), so we could easily change those to do something with SCRAM configs.
The one mentioned here is simply trying to surface an error as early as possible as per the comment:
```
# Confirm we have a successful ZooKeeper upgrade by describing the topic.
# Not trying to detect a problem here leads to failure in the ensuing Kafka roll, which would be a less
# intuitive failure than seeing a problem here, so detect ZooKeeper upgrade problems before involving Kafka.
self.zk.describe(self.topic)
```
So we could either just get rid of it or maybe list ACLs/do something with SCRAM.
It also looks like `describe()` and the above 3 invocations of it are the only things we have to worry about here -- the ZK Security Migrator in `zookeeper_migration()` and listing ACLs via the `list_acls()` method are the only other things that access ZooKeeper directly, and both of these are fully supported.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r667309254
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +59,48 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "ips",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandAlterUserQuota(): Unit = {
Review comment:
Good catch! Updated.
--
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] ijuma commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r659800702
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -68,7 +68,7 @@ object ConfigCommand extends Config {
val BrokerDefaultEntityName = ""
val BrokerLoggerConfigType = "broker-loggers"
val BrokerSupportedConfigTypes = ConfigType.all :+ BrokerLoggerConfigType
- val ZkSupportedConfigTypes = ConfigType.all
+ val ZkSupportedConfigTypes = Seq(ConfigType.User, ConfigType.Broker)
Review comment:
Wouldn't this allow quota updates via --zookeeper?
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "Ips",
Review comment:
Is "Ips" correct?
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
Review comment:
We should have a test like this but for `user` quotas.
##########
File path: docs/upgrade.html
##########
@@ -47,6 +47,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
<li>The <code>kafka-preferred-replica-election</code> command line tool was removed. Please use <code>kafka-leader-election</code> instead.</li>
<li>The <code>--zookeeper</code> option was removed from the <code>bin/kafka-topics.sh</code> and <code>bin/kafka-reassign-partitions.sh</code> command line tools.
Please use <code>--bootstrap-server</code> instead.</li>
+ <li>In <code>bin/kafka-configs.sh</code>, the <code>--zookeeper</code> option only supported for <a href="#security_sasl_scram_credentials">SCRAM Credentials configuration</a>
Review comment:
I would follow a similar pattern as the other lines. For example, "The <code>kafka-configs</code> command..."
--
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 edited a comment on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882171897
@ijuma , thanks for help update the comment and messages. :)
Failed tests are unrelated:
```
Build / ARM / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / ARM / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()
Build / JDK 11 and Scala 2.13 / kafka.server.LogDirFailureTest.testProduceErrorFromFailureOnCheckpoint()
Build / JDK 16 and Scala 2.13 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
Build / JDK 16 and Scala 2.13 / org.apache.kafka.clients.ClientUtilsTest.testResolveDnsLookup()
```
The `testResolveDnsLookup` failed should be because of infra's issue. It didn't fail in the previous and older PR build in this PR, and our latest update is only comment and message update. Besides, it also failed in another PR build that triggered 7 hours ago [here](https://github.com/apache/kafka/pull/11077/checks?check_run_id=3098846634).
Thank you.
--
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-877717089
@ijuma , I've uploaded the PR. Please take a look again. Thanks.
Failed tests are unrelated.
```
<div id="page-body" class="clear"><div id="main-panel">
Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()Build / JDK 16 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()Build / JDK 16 and Scala 2.13 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()Build / JDK 8 and Scala 2.12 / kafka.api.ConsumerBounceTest.testCloseDuringRebalance()Build / JDK 8 and Scala 2.12 / kafka.api.TransactionsTest.testSendOffsetsToTransactionTimeout()Build / JDK 8 and Scala 2.12 / kafka.api.TransactionsTest.testSendOffsetsToTransactionTimeout()
```
--
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-878703335
@rondagostino , thanks for the comments. Actually, I've already added 2 test cases for that:
`shouldNotAllowDescribeBrokerWhileBrokerUpUsingZookeeper` -> to test `describe` is not allowed while brokers are up
`shouldSupportDescribeBrokerBeforeBrokerUpUsingZookeeper` -> to test `describe` should be supported before brokers are up
Please let me know if there's any other suggestion. Thank you.
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661407689
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -68,14 +68,13 @@ object ConfigCommand extends Config {
val BrokerDefaultEntityName = ""
val BrokerLoggerConfigType = "broker-loggers"
val BrokerSupportedConfigTypes = ConfigType.all :+ BrokerLoggerConfigType
- val ZkSupportedConfigTypes = ConfigType.all
+ val ZkSupportedConfigTypes = Seq(ConfigType.User, ConfigType.Broker)
val DefaultScramIterations = 4096
// Dynamic broker configs can only be updated using the new AdminClient once brokers have started
// so that configs may be fully validated. Prior to starting brokers, updates may be performed using
// ZooKeeper for bootstrapping. This allows all password configs to be stored encrypted in ZK,
// avoiding clear passwords in server.properties. For consistency with older versions, quota-related
- // broker configs can still be updated using ZooKeeper at any time. ConfigCommand will be migrated
- // to the new AdminClient later for these configs (KIP-248).
Review comment:
> Why is that?
For consistency with older versions
> I think we should remove this.
Agree with you. It's a good timing now.
--
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 pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-883412685
Make sense. `KAFKA-13108: Improve the test coverage for ConfigCommandTest` is created for it. Thank you.
--
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] ijuma commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-869697363
@rajinisivaram @rondagostino The two cases mentioned in the PR still require --zookeeper, 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] ijuma commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882984058
--
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] ijuma commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661514537
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,39 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "ips",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusAlterUserQuotaWithoutEntityName(): Unit = {
Review comment:
But the CLI can verify if you are trying to update SCRAM credentials vs updating quotas. We should not allow the latter.
--
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] ijuma commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r659800702
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -68,7 +68,7 @@ object ConfigCommand extends Config {
val BrokerDefaultEntityName = ""
val BrokerLoggerConfigType = "broker-loggers"
val BrokerSupportedConfigTypes = ConfigType.all :+ BrokerLoggerConfigType
- val ZkSupportedConfigTypes = ConfigType.all
+ val ZkSupportedConfigTypes = Seq(ConfigType.User, ConfigType.Broker)
Review comment:
Wouldn't this allow quota updates via --zookeeper?
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "Ips",
Review comment:
Is "Ips" correct?
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
Review comment:
We should have a test like this but for `user` quotas.
##########
File path: docs/upgrade.html
##########
@@ -47,6 +47,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
<li>The <code>kafka-preferred-replica-election</code> command line tool was removed. Please use <code>kafka-leader-election</code> instead.</li>
<li>The <code>--zookeeper</code> option was removed from the <code>bin/kafka-topics.sh</code> and <code>bin/kafka-reassign-partitions.sh</code> command line tools.
Please use <code>--bootstrap-server</code> instead.</li>
+ <li>In <code>bin/kafka-configs.sh</code>, the <code>--zookeeper</code> option only supported for <a href="#security_sasl_scram_credentials">SCRAM Credentials configuration</a>
Review comment:
I would follow a similar pattern as the other lines. For example, "The <code>kafka-configs</code> command..."
--
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 edited a comment on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-877736694
@rondagostino , thanks for comments and many good catch! I've updated the PR. Also update the PR description.
> The PR as currently written restricts describing configs via --zookeeper to user and broker configs. Unlike with altering, it does not perform a check to see if the broker is not started. Do we wish to add such a check?
Good suggestion! Make sense to me. I added the check for broker type describe via `--zookeeper`, and add tests.
Thank you.
--
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] rajinisivaram commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-869699660
@ijuma Yes, those two cases would still require ZooKeeper for bootstrapping broker credentials/broker configs. I think there was some discussion around bootstrapping cases, @rondagostino would probably know what the plan is.
--
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] ijuma merged pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #10811:
URL: https://github.com/apache/kafka/pull/10811
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r667309366
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -850,22 +890,19 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
}
@Test
- def shouldAddBrokerQuotaConfig(): Unit = {
Review comment:
Yes, `shouldAddBrokerDynamicConfig` is testing it. I added one more quota config update to verify 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882984058
Failures are unrelated.
--
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] ijuma commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r667031473
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +59,48 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "ips",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandAlterUserQuota(): Unit = {
Review comment:
This says ZkCommand, but the test uses --bootstrap-server.
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -290,14 +340,9 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
assertEquals("[[1, 2], [3, 4]]", addedProps.getProperty("nested"))
}
- def doTestOptionEntityTypeNames(zkConfig: Boolean): Unit = {
- val connectOpts = if (zkConfig)
- ("--zookeeper", zkConnect)
- else
- ("--bootstrap-server", "localhost:9092")
-
+ def doTestOptionEntityTypeNames(): Unit = {
Review comment:
This should be inlined into the test since it's only used in one place? Also, the removed test is no longer needed, I assume?
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -876,6 +872,11 @@ object ConfigCommand extends Config {
throw new IllegalArgumentException(s"--bootstrap-server must be specified for --all")
}
+ if (options.has(zkTlsConfigFile) && options.has(bootstrapServerOpt)) {
+ throw new IllegalArgumentException("--bootstrap-server doesn't support --zk-tls-config-file option. " +
+ "Please use --zookeeper option instead")
Review comment:
Is this error only printed for cases that are _not_ supported by `--bootstrap-server`? For example, if both `--bootstrap-server` and `--zk-tls-config-file` are configured and you are trying to do something that is supported by `--bootstrap-server`, the message should suggest removing `--zk-tls-config-file` instead.
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -850,22 +890,19 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
}
@Test
- def shouldAddBrokerQuotaConfig(): Unit = {
Review comment:
We have a `shouldAddBrokerQuotaConfig` test with bootstrap-server?
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661880016
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,39 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "ips",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusAlterUserQuotaWithoutEntityName(): Unit = {
Review comment:
Aha, I know what you mean now. Let me update it. Thanks 😊
--
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] ijuma commented on pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-869697363
@rajinisivaram @rondagostino The two cases mentioned in the PR still require --zookeeper, 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] showuon commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882996715
@ijuma , thanks for the update. It looks better now! Also, thank you and @rondagostino for your patiently review!
For your question:
> it looks to me that we don't have test coverage in ConfigCommandTest for the case where we try to update broker configs via zk when the brokers are up, is that right?
I actually added a test for it:
`shouldNotAllowAddBrokerQuotaConfigWhileBrokerUpUsingZookeeper` [here](https://github.com/apache/kafka/blob/da252c633d88ecec6f68200081ad94a3081e5f35/core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala#L905)
And this test is to test update multiple broker configs using zookeeper (when no brokers up)
`testDynamicBrokerConfigUpdateUsingZooKeeper` [here](https://github.com/apache/kafka/blob/da252c633d88ecec6f68200081ad94a3081e5f35/core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala#L1245)
Do you think we should add more tests for 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.
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-853823700
@ijuma , please help review this PR. Thanks.
--
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] showuon commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r667309320
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -290,14 +340,9 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
assertEquals("[[1, 2], [3, 4]]", addedProps.getProperty("nested"))
}
- def doTestOptionEntityTypeNames(zkConfig: Boolean): Unit = {
- val connectOpts = if (zkConfig)
- ("--zookeeper", zkConnect)
- else
- ("--bootstrap-server", "localhost:9092")
-
+ def doTestOptionEntityTypeNames(): Unit = {
Review comment:
Thanks for reminder! I added the removed test back, and only test "user" and "brokers" config type for `zkConfig`. Thanks.
--
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] ijuma commented on a change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r661276882
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,39 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithIpsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "ips",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusAlterUserQuotaWithoutEntityName(): Unit = {
Review comment:
I was thinking this test should be exactly like the `clients` entity case, why did we make it different?
--
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 change in pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#discussion_r660562020
##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -60,6 +60,30 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
"--add-config", "security.inter.broker.protocol=PLAINTEXT"))
}
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithTopicsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "topics",
+ "--describe"))
+ }
+
+ @Test
+ def shouldExitWithNonZeroStatusOnZkCommandWithClientsEntity(): Unit = {
+ assertNonZeroStatusExit(Array(
+ "--zookeeper", zkConnect,
+ "--entity-type", "clients",
Review comment:
Will do. Thanks.
--
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 pull request #10811: KAFKA-12598: remove zookeeper support on configCommand except security config
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-871984330
@ijuma , I've updated the PR:
1. remove the support to update quota related broker configs using ZooKeeper.
2. require `zk-tls-config-file` option when `zookeeper` is provided.
I didn't block the `user` `describe` action before because the `user` can still describe to check SCRAM credential status. But you're right, we should not allow quota related update/describe. So, I add a required option `zk-tls-config-file` option when `zookeeper` is provided. This way, we can make sure it's doing SCRAM credential update or password update before broker started (both operation need `zk-tls-config-file` option)
3. update tests
Thank you.
--
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] ijuma commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-885004599
@showuon I'm seeing the following in the system test `upgrade_test`:
> INFO - 2021-07-22 08:16:06,976 - runner_client - log - lineno:241]: RunnerClient: kafkatest.tests.core.upgrade_test.TestUpgrade.test_upgrade.from_kafka_version=0.9.0.1.to_message_format_version=0.9.0.1.compression_types=.none: FAIL: Rem
> oteCommandError({'ssh_config': {'host': 'ducker11', 'hostname': 'ducker11', 'user': 'ducker', 'port': 22, 'password': '', 'identityfile': '/home/ducker/.ssh/id_rsa'}, 'hostname': 'ducker11', 'ssh_hostname': 'ducker11', 'user': 'ducker', '
> externally_routable_ip': 'ducker11', '_logger': <Logger kafkatest.tests.core.upgrade_test.TestUpgrade.test_upgrade.from_kafka_version=0.9.0.1.to_message_format_version=0.9.0.1.compression_types=.none-15 (DEBUG)>, 'os': 'linux', '_ssh_clie
> nt': <paramiko.client.SSHClient object at 0x7f7a45174f28>, '_sftp_client': <paramiko.sftp_client.SFTPClient object at 0x7f7a441069b0>}, '/opt/kafka-dev/bin/kafka-run-class.sh kafka.admin.ConfigCommand --zookeeper ducker11:2181 --describe
> --topic test_topic', 1, b'')
> Traceback (most recent call last):
> File "/usr/local/lib/python3.7/dist-packages/ducktape/tests/runner_client.py", line 133, in run
> data = self.run_test()
> File "/usr/local/lib/python3.7/dist-packages/ducktape/tests/runner_client.py", line 190, in run_test
> return self.test_context.function(self.test)
> File "/usr/local/lib/python3.7/dist-packages/ducktape/mark/_mark.py", line 429, in wrapper
> return functools.partial(f, *args, **kwargs)(*w_args, **w_kwargs)
> File "/opt/kafka-dev/tests/kafkatest/tests/core/upgrade_test.py", line 199, in test_upgrade
> cluster_id = self.kafka.cluster_id()
> File "/opt/kafka-dev/tests/kafkatest/tests/produce_consume_validate.py", line 105, in run_produce_consume_validate
> core_test_action(*args)
> File "/opt/kafka-dev/tests/kafkatest/tests/core/upgrade_test.py", line 200, in <lambda>
> assert cluster_id is not None
> File "/opt/kafka-dev/tests/kafkatest/tests/core/upgrade_test.py", line 57, in perform_upgrade
> self.zk.describe(self.topic)
> File "/opt/kafka-dev/tests/kafkatest/services/zookeeper.py", line 234, in describe
> output = self.nodes[0].account.ssh_output(cmd)
> File "/usr/local/lib/python3.7/dist-packages/ducktape/cluster/remoteaccount.py", line 370, in ssh_output
> raise RemoteCommandError(self, cmd, exit_status, stderr.read())
> ducktape.cluster.remoteaccount.RemoteCommandError: ducker@ducker11: Command '/opt/kafka-dev/bin/kafka-run-class.sh kafka.admin.ConfigCommand --zookeeper ducker11:2181 --describe --topic test_topic' returned non-zero exit status 1.
Is this related to this PR?
--
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 pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-885307107
Let me handle 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma merged pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #10811:
URL: https://github.com/apache/kafka/pull/10811
--
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