KAFKA-13763 (1): Improve unit testing coverage and flexibility for IncrementalCooperativeAssignor by C0urante · Pull Request #11974 · apache/kafka

C0urante · 2022-03-30T21:54:42Z

This is strictly a testing refactor. No functional changes are made; this can be easily verified by confirming that the only affected file is the IncrementalCooperativeAssignorTest.java test suite.

These changes were initially discussed during review of #10367, which partially focused on improving readability in the unit tests for incremental rebalancing in Connect.

The goals here include:

Simplify the logic that has to be manually specified on a per-test-case basis for simulating a rebalance (accomplished by extracting common logic into reusable utility methods such as performRebalance and addNewEmptyWorkers)
Reduce the cognitive burden for following testing logic by removing unnecessary fields (like expectedMemberConfigs and assignments) and assertions (like the redundant checks for leader and leader URL)
Add powerful, granular, and reusable utility methods that can provide stronger guarantees about the state of a cluster across successive rebalances without forcing people to track this state in their heads (accomplished by replacing the existing assertAssignments method with assertWorkers, assertEmptyAssignment, assertConnectorAllocations, and assertTaskAllocations, and by adding the new assertBalancedAllocation and assertCompleteAllocation methods)
Refactor common logic for testing utilities to be more concise and reduce the number of Java 8 streams statements that have to be understood in order to read through a test case
Fix a bug in the assertion logic for checking for duplicates currently present here and here (List::removeAll removes all occurrences of any element contained in the collection passed to the method)
Remove an incorrect assertion in the assertNoReassignments (now renamed to assertNoRedundantAssignments) method that there should be no duplicated connectors or tasks in the assignments reported by each worker to the leader during rebalance (this is unnecessary and even contradicts logic used for testing in cases like this where we intentionally simulate a worker with a duplicated set of connectors and/or tasks rejoining a cluster; the only reason this bug wasn't surfaced sooner is because the bug mentioned in the prior point covers it)

Once merged, this should allow for cleaner, faster test writing when adding new cases for incremental rebalancing, such as with #10367.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…s at the end of test cases

…or generation mismatch

…emove unnecessary expectedMemberConfigs field

…ethod

…for redundant assignments during tests

…ask allocation

…eaving the cluster

…ask allocation

… logic for examining aggregated assignment data

…t and worker metadata

…ertConnectorAllocations, and assertTaskAllocations methods

…ary duplication of worker assignments

C0urante · 2022-03-30T21:56:36Z

There's still room for improvement with the incremental rebalancing testing logic, but many of the remaining changes would involve modifications to the IncrementalCooperativeAssignor class as well. In order to reduce PR size and keep things focused, I've decided to leave that to a follow-up pull request, but since it would accomplish basically the same goal, I'll attach it to KAFKA-13763 as well.

C0urante · 2022-04-01T19:01:29Z

Filed #11983 as a more aggressive follow-up that touches on the IncrementalCooperativeAssignor class.

mimaison

Thanks @C0urante for the PR, this is a nice cleanup!

I've not looked at everything in detail yet. I'll make another pass tomorrow. I've left a few small suggestions.

mimaison · 2022-04-04T16:34:18Z

 import static org.apache.kafka.connect.runtime.distributed.IncrementalCooperativeConnectProtocol.CONNECT_PROTOCOL_V2;
 import static org.apache.kafka.connect.runtime.distributed.WorkerCoordinator.WorkerLoad;
-import static org.hamcrest.CoreMatchers.hasItems;
 import static org.hamcrest.CoreMatchers.is;


Could we remove hamcrest now and only use junit?

Good call, done 👍

mimaison · 2022-04-04T16:47:03Z

+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.runners.Parameterized.Parameter;


Not related to changes in this PR. I see we are defining a test parameter with mode() but the class is not annotated with @RunWith(value = Parameterized.class) so protocolVersion is never initialized and always set to 0.

Ah, good catch! I've corrected this in the test, although it's worth noting that in #11983 I'm proposing that we remove the protocolVersion parameter entirely since it's not accomplishing a whole lot here.

mimaison · 2022-04-04T16:55:23Z

+    private Map<String, ExtendedWorkerState> memberConfigs(String givenLeader,
                                                                  long givenOffset,
                                                                  int start,
                                                                  int connectorNum) {


Should this be workerNum instead of connectorNum?

Yep, good catch 👍

mimaison · 2022-04-04T17:01:57Z

-        return IntStream.range(start, connectorNum + 1)
-                .mapToObj(i -> new SimpleEntry<>("connector" + i, state))
-                .collect(Collectors.toMap(SimpleEntry::getKey, SimpleEntry::getValue));
+        return fillMap(


I wonder if building this map explicitly may be more readable. What about something like:

private Map<String, ExtendedWorkerState> memberConfigs(String givenLeader, long givenOffset, String ... workers)

Then you call it with memberConfigs(leader, offset, W0, W1). It's slightly less flexible, but since all tests only use a few workers I think this could make the code a bit simpler.

Agree that it's more readable, and in some ways it's actually more flexible since you can now specify an arbitrary set of worker names instead of having them generated for you. Done 👍

mimaison · 2022-04-05T09:49:06Z

    @Parameters
    public static Iterable<?> mode() {
-        return Arrays.asList(new Object[][] {{CONNECT_PROTOCOL_V1, CONNECT_PROTOCOL_V2}});
+        return Arrays.asList(new Object[][] {{CONNECT_PROTOCOL_V1}, {CONNECT_PROTOCOL_V2}});


This can be simplified into return Arrays.asList(CONNECT_PROTOCOL_V1, CONNECT_PROTOCOL_V2); but since you're removing it in #11983, we can ignore it.

Blegh, thanks. I'll simplify it anyways; rather leave trunk in a healthy place and not risk FUD from someone else copy+pasting this style.

mimaison

Thanks @C0urante, the PR looks good. I left a few more very minor suggestions.

mimaison · 2022-04-05T10:29:56Z

+    private void expectGeneration(boolean expectMismatch) {
+        when(coordinator.generationId())
+                .thenReturn(assignor.previousGenerationId + 1)
+                .thenReturn(assignor.previousGenerationId + 1);


Since we return the same value, we can remove one of these thenReturn()

Ah, good call. I figured the existing style was necessary since WorkerCoordinator::generationId would be invoked twice for every call to expectGeneration but it looks like Mockito handles this just fine with a single thenReturn.

mimaison · 2022-04-05T10:38:58Z

                memberConfigs);

-        assertThat("Wrong set of workers for reassignments",
+        assertEquals("Wrong set of workers for reassignments",


What about simplifying these assertions into:

assertTrue("Wrong set of workers for reassignments", assignor.candidateWorkersForReassignment.isEmpty());

I usually prefer assertEquals when possible since it shows the difference between actual and expected, which in this case would mean the complete set of incorrect candidate workers for reassignment.

…ck expectations

C0urante · 2022-04-05T15:39:41Z

Thanks @mimaison. I hope you don't mind but I've pushed one more change that simplifies how the configured set of connectors and tasks is simulated (this came in handy when designing tests for one of the scenarios described in KAFKA-13764).

It should improve readability by removing the existing clusterConfigState methods (and the fillMap method that was introduced in this PR to clean those up) and making changes to the set of configured connectors and tasks more explicit. However, if this is too much then let me know and I can revert the commit from this PR and add it in a follow-up.

mimaison

Thanks, LGTM

C0urante added 14 commits March 23, 2022 18:00

KAFKA-13763: Add utility method for verifying coordinator interaction…

25e4161

…s at the end of test cases

KAFKA-13763: Add utility method for simulating a round of rebalance

72800cf

KAFKA-13763: Augment rebalance simulation method, fix testing logic f…

453188f

…or generation mismatch

KAFKA-13763: Remove redundant assertions for leader and leader URL, r…

c76a72e

…emove unnecessary expectedMemberConfigs field

KAFKA-13763: Remove unnecessary assignments field and memberConfigs m…

cd429bb

…ethod

KAFKA-13763: Automatically apply rebalance results and perform check …

59bf8d8

…for redundant assignments during tests

KAFKA-13763: Add utility method for ensuring balanced connector and t…

8a67769

…ask allocation

KAFKA-13763: Add utility methods for simulating workers joining and l…

20fa63b

…eaving the cluster

KAFKA-13763: Add utility method for ensuring complete connector and t…

e4fcfc5

…ask allocation

KAFKA-13763: Remove incorrect check for redunant assignment, refactor…

2c9b753

… logic for examining aggregated assignment data

KAFKA-13763: Refactor boilerplate logic for generating config snapsho…

4194b1e

…t and worker metadata

KAFKA-13763: Remove unnecessary parameter from assertDelay method

4e47c72

KAFKA-13763: Replace assertAssignments method with assertWorkers, ass…

f1ad262

…ertConnectorAllocations, and assertTaskAllocations methods

KAFKA-13763: Refine logic for simulating new worker to avoid unnecess…

940c4d6

…ary duplication of worker assignments

This was referenced Mar 30, 2022

KAFKA-12495: allow consecutive revoke in incremental cooperative assignor in connector #10367

Closed

KAFKA-13763 (2): Refactor IncrementalCooperativeAssignor for improved unit testing #11983

Merged

mimaison reviewed Apr 4, 2022

View reviewed changes

KAFKA-13763: Address review comments

37aafe6

mimaison reviewed Apr 5, 2022

View reviewed changes

KAFKA-13763: Clean up unit test parameterization, remove redundant mo…

2516b51

…ck expectations

KAFKA-13763: Simplify logic for simulating new and deleted connectors

d68d57f

mimaison approved these changes Apr 5, 2022

View reviewed changes

mimaison merged commit 88e5f13 into apache:trunk Apr 5, 2022

C0urante deleted the kafka-13763 branch April 5, 2022 19:45

Conversation

C0urante commented Mar 30, 2022

Committer Checklist (excluded from commit message)

Uh oh!

C0urante commented Mar 30, 2022

Uh oh!

C0urante commented Apr 1, 2022

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0urante commented Apr 5, 2022

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mimaison Apr 5, 2022 •

edited

Loading