C0urante commented on code in PR #14893:
URL: https://github.com/apache/kafka/pull/14893#discussion_r1415960042


##########
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java:
##########
@@ -770,6 +771,9 @@ public void testSyncTopicConfigs() throws 
InterruptedException {
         Map<ConfigResource, Collection<AlterConfigOp>> configOps = 
Collections.singletonMap(configResource, ops);
         // alter configs on target cluster
         backup.kafka().incrementalAlterConfigs(configOps);
+        // wait until the configs are changed
+        waitForConfigValueChange(backup, backupTopic, "delete.retention.ms", 
"2000");
+        waitForConfigValueChange(backup, backupTopic, "retention.bytes", 
"2000");

Review Comment:
   @ashwinpankaj I looked into the suggestion to wait on the returned futures 
as it seemed quite promising, but it turns out that we already do that in the 
body of `EmbeddedKafkaCluster::incrementalAlterConfigs`: 
https://github.com/apache/kafka/blob/587f50d48f832f2ce76ed8f47b6ea472da3b0848/connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedKafkaCluster.java#L368-L370
   
   Because of this, though, I'm pretty skeptical that any extra assertions 
about config changes making it to the backup cluster are going to be 
beneficial. We also perform assertions about propagated config updates inside a 
`waitForCondition` call, so there should already be bounded retry logic to 
handle potential delays in propagation. Maybe we just don't give enough time 
for that to work on Jenkins?
   
   @atu-sharm have you considered adding more informative error messages to the 
[currently-failing 
assertion](https://github.com/apache/kafka/blob/587f50d48f832f2ce76ed8f47b6ea472da3b0848/connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java#L775-L789)?
 That may be useful for distinguishing between failures caused by legitimate 
bugs, bad testing logic, and timeouts that are too short for our CI machinery.



-- 
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]

Reply via email to