You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "rondagostino (via GitHub)" <gi...@apache.org> on 2023/02/13 17:36:22 UTC
[GitHub] [kafka] rondagostino opened a new pull request, #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
rondagostino opened a new pull request, #13241:
URL: https://github.com/apache/kafka/pull/13241
…g option
https://github.com/apache/kafka/pull/12951 accidentally eliminated support for the `--command-config` option in the `kafka-metadata-quorum.sh` command. This was an undetected regression in the 3.4.0 release.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on PR #13241:
URL: https://github.com/apache/kafka/pull/13241#issuecomment-1428462509
Noting that we do in fact test the use of other security protocols with the KRaft remote controllers in `tests/kafkatest/sanity_checks/test_verifiable_producer.py`. But we don't invoke the CLI in that test. The added unit test here is sufficient to catch the issue, and the lack of a system test failure during the release does not necessarily indicate a system test gap.
--
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] jsancio commented on pull request #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on PR #13241:
URL: https://github.com/apache/kafka/pull/13241#issuecomment-1428433542
@rondagostino Is there a work around for this issue or do we need to do a 3.4.1 release ASAP?
--
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] jsancio commented on a diff in pull request #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13241:
URL: https://github.com/apache/kafka/pull/13241#discussion_r1105024172
##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -116,6 +111,16 @@ static void execute(String... args) throws Exception {
}
}
+ private static Properties getProperties(File optionalCommandConfig) throws TerseException, IOException {
+ if (optionalCommandConfig == null) {
+ return new Properties();
+ } else {
+ if (!optionalCommandConfig.exists())
+ throw new TerseException("Properties file " + optionalCommandConfig.getPath() + " does not exists!");
Review Comment:
Minor but I would add a `{ }` block statement instead of using single line statement for `if`.
--
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 #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on PR #13241:
URL: https://github.com/apache/kafka/pull/13241#issuecomment-1428859177
test failures are unrelated. Both failures mentioned pass locally. Merging to trunk and 3.4
--
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 #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino commented on PR #13241:
URL: https://github.com/apache/kafka/pull/13241#issuecomment-1428435013
There is no workaround for this issue. A 3.4.1 release should happen ASAP.
--
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 merged pull request #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
Posted by "rondagostino (via GitHub)" <gi...@apache.org>.
rondagostino merged PR #13241:
URL: https://github.com/apache/kafka/pull/13241
--
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] jsancio commented on a diff in pull request #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13241:
URL: https://github.com/apache/kafka/pull/13241#discussion_r1104851324
##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -92,7 +92,7 @@ static void execute(String... args) throws Exception {
if (!commandConfig.exists())
throw new TerseException("Properties file " + commandConfig.getPath() + " does not exists!");
- Utils.loadProps(commandConfig.getPath());
+ props = Utils.loadProps(commandConfig.getPath());
Review Comment:
How about making `props` final with `final Properties props;`? Setting here and added an `else` block that sets it to `new Properties()`.
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java:
##########
@@ -141,6 +145,16 @@ public void testOnlyOneBrokerAndOneController() {
assertEquals("0", replicationOutput.split("\n")[1].split("\\s+")[2]);
}
+ @ClusterTests({
+ @ClusterTest(clusterType = Type.CO_KRAFT, brokers = 1, controllers = 1)
+ })
+ public void testCommandConfig() throws IOException {
+ // specifying a --command-config containing properties that would prevent login must fail
+ File tmpfile = TestUtils.tempFile(AdminClientConfig.SECURITY_PROTOCOL_CONFIG + "=SSL_PLAINTEXT");
+ assertEquals(1, MetadataQuorumCommand.mainNoExit("--bootstrap-server", cluster.bootstrapServers(),
+ "--command-config", tmpfile.getAbsolutePath(), "describe", "--status"));
+ }
Review Comment:
Thanks for adding a test to make sure we don't regress this in the future.
--
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 diff in pull request #13241: KAFKA-14711: kafaka-metadata-quorum.sh does not honor --command-confi…
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #13241:
URL: https://github.com/apache/kafka/pull/13241#discussion_r1105053282
##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -116,6 +111,16 @@ static void execute(String... args) throws Exception {
}
}
+ private static Properties getProperties(File optionalCommandConfig) throws TerseException, IOException {
+ if (optionalCommandConfig == null) {
+ return new Properties();
+ } else {
+ if (!optionalCommandConfig.exists())
+ throw new TerseException("Properties file " + optionalCommandConfig.getPath() + " does not exists!");
Review Comment:
That's not required by the coding guidelines though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org