Re: [PR] KAFKA-16684: fix flaky DedicatedMirrorIntegrationTest [kafka]
C0urante merged PR #15906: URL: https://github.com/apache/kafka/pull/15906 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16684: fix flaky DedicatedMirrorIntegrationTest [kafka]
C0urante commented on PR #15906: URL: https://github.com/apache/kafka/pull/15906#issuecomment-2105098368 CI has passed on at least one node and the changes are trivial; merging... -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16684: fix flaky DedicatedMirrorIntegrationTest [kafka]
johnnychhsu commented on PR #15906: URL: https://github.com/apache/kafka/pull/15906#issuecomment-2104887677 @C0urante thanks for the review! Just updated 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16684: fix flaky DedicatedMirrorIntegrationTest [kafka]
johnnychhsu commented on code in PR #15906:
URL: https://github.com/apache/kafka/pull/15906#discussion_r1596951379
##
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/DedicatedMirrorIntegrationTest.java:
##
@@ -353,6 +353,9 @@ private void
awaitTaskConfigurations(MirrorMaker mm,
.map(TaskInfo::config)
.allMatch(predicate);
} catch (Exception ex) {
+if (ex instanceof RebalanceNeededException) {
+throw ex;
+}
Review Comment:
you are right, let me add the comment, 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16684: fix flaky DedicatedMirrorIntegrationTest [kafka]
johnnychhsu commented on code in PR #15906:
URL: https://github.com/apache/kafka/pull/15906#discussion_r1596951056
##
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/DedicatedMirrorIntegrationTest.java:
##
@@ -260,9 +261,8 @@ public void testMultiNodeCluster() throws Exception {
awaitConnectorTasksStart(mirrorMakers.get("node 0"),
MirrorHeartbeatConnector.class, sourceAndTarget);
// Create one topic per Kafka cluster per MirrorMaker node
-final int topicsPerCluster = numNodes;
Review Comment:
thanks for the review!
totally agree, let me move 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16684: fix flaky DedicatedMirrorIntegrationTest [kafka]
C0urante commented on code in PR #15906:
URL: https://github.com/apache/kafka/pull/15906#discussion_r1595528749
##
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/DedicatedMirrorIntegrationTest.java:
##
@@ -353,6 +353,9 @@ private void
awaitTaskConfigurations(MirrorMaker mm,
.map(TaskInfo::config)
.allMatch(predicate);
} catch (Exception ex) {
+if (ex instanceof RebalanceNeededException) {
+throw ex;
+}
Review Comment:
Can we add a comment here explaining this non-obvious behavior?
```suggestion
if (ex instanceof RebalanceNeededException) {
// It's okay to retry on this error; happens when a
worker has read a new config
// from the config topic but hasn't completed the
subsequent rebalance yet
throw ex;
}
```
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16684: fix flaky DedicatedMirrorIntegrationTest [kafka]
C0urante commented on code in PR #15906:
URL: https://github.com/apache/kafka/pull/15906#discussion_r1595516663
##
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/DedicatedMirrorIntegrationTest.java:
##
@@ -260,9 +261,8 @@ public void testMultiNodeCluster() throws Exception {
awaitConnectorTasksStart(mirrorMakers.get("node 0"),
MirrorHeartbeatConnector.class, sourceAndTarget);
// Create one topic per Kafka cluster per MirrorMaker node
-final int topicsPerCluster = numNodes;
Review Comment:
With cosmetic changes like this, please watch out so that comments don't
become stale. The `// Create one topic per Kafka cluster...` comment should be
moved to just above the for-loop since it has nothing to do with the
`messagesPerTopic` declaration.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
