chickenchickenlove commented on code in PR #21280:
URL: https://github.com/apache/kafka/pull/21280#discussion_r2714851541
##########
tools/src/test/java/org/apache/kafka/tools/ConfigCommandIntegrationTest.java:
##########
@@ -461,27 +461,37 @@ public void
testUpdatePerBrokerConfigInKRaftThenShouldFail() {
}
@ClusterTest
- public void testUpdateInvalidBrokerConfigs() {
+ public void testUpdateInvalidBrokerConfigs() throws InterruptedException {
updateAndCheckInvalidBrokerConfig(Optional.empty());
updateAndCheckInvalidBrokerConfig(Optional.of(String.valueOf((cluster.brokers().entrySet().iterator().next().getKey()))));
}
- private void updateAndCheckInvalidBrokerConfig(Optional<String>
brokerIdOrDefault) {
+ private void updateAndCheckInvalidBrokerConfig(Optional<String>
brokerIdOrDefault) throws InterruptedException {
List<String> alterOpts =
generateDefaultAlterOpts(cluster.bootstrapServers());
try (Admin client = cluster.admin()) {
alterConfigWithAdmin(client, brokerIdOrDefault, Map.of("invalid",
"2"), alterOpts);
+ AtomicReference<String> last = new AtomicReference<>("");
- Stream<String> describeCommand = Stream.concat(
- Stream.concat(
- Stream.of("--bootstrap-server",
cluster.bootstrapServers()),
- Stream.of(entityOp(brokerIdOrDefault).toArray(new
String[0]))),
- Stream.of("--entity-type", "brokers", "--describe"));
- String describeResult = captureStandardOut(run(describeCommand));
-
- // We will treat unknown config as sensitive
- assertTrue(describeResult.contains("sensitive=true"),
describeResult);
- // Sensitive config will not return
- assertTrue(describeResult.contains("invalid=null"),
describeResult);
+ TestUtils.waitForCondition(() -> {
+ Stream<String> describeCommand = Stream.concat(
+ Stream.concat(
+ Stream.of("--bootstrap-server",
cluster.bootstrapServers()),
+
Stream.of(entityOp(brokerIdOrDefault).toArray(new String[0]))),
+ Stream.of("--entity-type", "brokers", "--describe")
+ );
+ String describeResult =
captureStandardOut(run(describeCommand));
+ last.set(describeResult);
+
+ boolean hasInvalidNull =
describeResult.contains("invalid=null");
+ boolean hasSensitiveTrue =
describeResult.contains("sensitive=true");
+
+ if (hasInvalidNull && !hasSensitiveTrue) {
+ throw new AssertionError("Expected sensitive=true when
invalid=null is visible, but it was not present.\n" +
Review Comment:
@lianetm
Thanks for the detailed review!!! 🙇♂️ — good point about waitForCondition
catching exceptions (via `retryOnExceptionWithTimeout`), which could lead to
unnecessary retries.
I refactored the test to wait for `invalid=null` first, and then assert
`sensitive=true` outside the wait loop.
I also removed the custom error message since the assertion logic is now
explicit and self-explanatory.
When you have bandwidth, please take another look. 🙇♂️
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
