Skip to content

KAFKA-10083: fix failed testReassignmentWithRandomSubscriptionsAndChanges tests#8778

Closed
showuon wants to merge 1 commit intoapache:trunkfrom
showuon:KAFKA-10083
Closed

KAFKA-10083: fix failed testReassignmentWithRandomSubscriptionsAndChanges tests#8778
showuon wants to merge 1 commit intoapache:trunkfrom
showuon:KAFKA-10083

Conversation

@showuon
Copy link
Copy Markdown
Member

@showuon showuon commented Jun 2, 2020

The failed test is because we changed the class member partitionMovements initialization to when the class instance created, from initialized when used within assign method. This won't have any issue when 1st used the AbstractStickyAssignor instance. But if it is used later, the partitionMovements will store the old info, and cause this failed tests. Fix it by moving the partitionMovements initialization back to assign method.

Committer Checklist (excluded from commit message)

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

@showuon
Copy link
Copy Markdown
Member Author

showuon commented Jun 2, 2020

@ableegoldman , could you review this PR to fix the failed tests? Thanks.

public static final int DEFAULT_GENERATION = -1;

private PartitionMovements partitionMovements = new PartitionMovements();
private PartitionMovements partitionMovements;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we still initialize it here as well? I remember that was necessary for some tests to pass since they might never get to the generalAssign method and isSticky would hit NPE

On the other hand, it seems like isSticky is pointless to call unless we get to the generalAssign method. So maybe we should just remove that from the tests that only do the constrainedAssign and just verify the stickiness directly?

@ableegoldman
Copy link
Copy Markdown
Member

cc @mjsax @guozhangwang , should be cherrypicked to 2.6, 2.5, and 2.4 (once my comment above is addressed)

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 2, 2020

ok to test

@ableegoldman
Copy link
Copy Markdown
Member

Hey @showuon, this failing test has been blocking others so I submitted a quick PR to fix it ASAP: #8786

But as noted in the PR and in my earlier comment here, this is just the "minimum fix". I think ideally we would improve the tests to not just rely on PartitionMovements/#isSticky for stickiness verification. If you're interested in pivoting this PR to work on that, I'd be happy to review it!

@showuon
Copy link
Copy Markdown
Member Author

showuon commented Jun 3, 2020

Close this PR since it'll be fixed in #8786. And #8788 will do the test improvement. Thanks.

@showuon showuon closed this Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants