gharris1727 commented on a change in pull request #9765:
URL: https://github.com/apache/kafka/pull/9765#discussion_r551448513
##########
File path:
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java
##########
@@ -566,6 +566,112 @@ public Boolean answer() throws Throwable {
PowerMock.verifyAll();
}
+ @Test
+ public void testRevoke() throws TimeoutException {
+ revokeAndReassign(false);
+ }
+
+ @Test
+ public void testIncompleteRebalanceBeforeRevoke() throws TimeoutException {
+ revokeAndReassign(true);
+ }
+
+ public void revokeAndReassign(boolean incompleteRebalance) throws
TimeoutException {
Review comment:
Yeah, this test was a bit difficult to wrap my head around at first, but
I think it's the best way to target this section of the code. I don't believe
that adding a new flakey test is prudent, and making a non-flakey test with
less mocks might end up to be harder to follow than this mocked test.
I think what _would_ be a good test to add would be a variant which replaced
this contrived reading-config-offset-topic-failure with a [genuine
WakeupException thrown from the end of
tick](https://github.com/apache/kafka/blob/ac7b5d3389fddf46bc53ab656de1fa7e2562efdb/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java#L433-L443),
which I believe is the true root cause of this issue most of the time. This is
not easy with the boilerplate in this test as-is, and requires a little bit of
refactoring to set up the rebalance during that block.
----------------------------------------------------------------
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:
us...@infra.apache.org
