Skip to content

KAFKA-12933: Flaky test ReassignPartitionsIntegrationTest.testReassignmentWithAlterIsrDisabled#11244

Merged
ijuma merged 1 commit into
apache:trunkfrom
splett2:KAFKA-12933
Aug 23, 2021
Merged

KAFKA-12933: Flaky test ReassignPartitionsIntegrationTest.testReassignmentWithAlterIsrDisabled#11244
ijuma merged 1 commit into
apache:trunkfrom
splett2:KAFKA-12933

Conversation

@splett2

@splett2 splett2 commented Aug 20, 2021

Copy link
Copy Markdown
Contributor

What

Removes assertion added in #10471
It's unsafe to assert that there are partition movements ongoing for some of the tests in the suite because partitions in some of the tests have 0 data, which may complete reassignment before verify can run.

Testing

Tests pass locally.

Committer Checklist (excluded from commit message)

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

@splett2

splett2 commented Aug 20, 2021

Copy link
Copy Markdown
Contributor Author

@showuon can you take a look? thanks.

@showuon

showuon commented Aug 20, 2021

Copy link
Copy Markdown
Member

I'll check it. Thank you for the PR.

@showuon showuon left a comment

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.

@splett2 , thanks for the fix. Left a comment. Thanks.

Comment on lines 129 to 130
val verifyAssignmentResult = runVerifyAssignment(cluster.adminClient, assignment, false)
assertTrue(verifyAssignmentResult.partsOngoing)
assertFalse(verifyAssignmentResult.movesOngoing)

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.

I checked and confirmed that this change makes sense, because there might not have on-going reassignment at this moment if the reassignment completed before we verify it.

However, in this case, I don't think we verify the moveOngoing makes sense here, either, since it might passed just because the reassignment completed before the verification. I think we can directly remove all these 3 lines and directly wait for the reassignment completion. What do you think?

@splett2 splett2 Aug 22, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

verifying moveOngoing here is fine because we don't perform intra-broker replica assignments(alter log dirs) in these tests, so movesOngoing should always be false.

@ijuma ijuma left a comment

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.

LGTM, thanks!

@ijuma

ijuma commented Aug 23, 2021

Copy link
Copy Markdown
Member

Some builds exited before completion, one completed successfully. Since this PR only removed a test assertion, we know they're unrelated. Merging to trunk and cherry-picking to 3.0.

@ijuma ijuma merged commit 9cf7a5c into apache:trunk Aug 23, 2021
ijuma pushed a commit that referenced this pull request Aug 23, 2021
…nmentWithAlterIsrDisabled (#11244)

Removes assertion added in #10471. It's unsafe to assert that
there are partition movements ongoing for some of the tests in
the suite because partitions in some of the tests have 0 data,
which may complete reassignment before `verify` can run.

Tests pass locally.

Reviewers: Luke Chen <showuon@gmail.com>, Ismael Juma <ismael@juma.me.uk>
xdgrulez pushed a commit to xdgrulez/kafka that referenced this pull request Dec 22, 2021
…nmentWithAlterIsrDisabled (apache#11244)

Removes assertion added in apache#10471. It's unsafe to assert that
there are partition movements ongoing for some of the tests in
the suite because partitions in some of the tests have 0 data,
which may complete reassignment before `verify` can run.

Tests pass locally.

Reviewers: Luke Chen <showuon@gmail.com>, Ismael Juma <ismael@juma.me.uk>
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