Skip to content

MINOR: Remove ReplicaManagerTest.initializeLogAndTopicId#12276

Merged
jsancio merged 1 commit intoapache:trunkfrom
ijuma:remove-easymock-hack-replica-manager-test
Jun 14, 2022
Merged

MINOR: Remove ReplicaManagerTest.initializeLogAndTopicId#12276
jsancio merged 1 commit intoapache:trunkfrom
ijuma:remove-easymock-hack-replica-manager-test

Conversation

@ijuma
Copy link
Member

@ijuma ijuma commented Jun 9, 2022

The workaround is not required with mockito.

Committer Checklist (excluded from commit message)

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

@ijuma ijuma requested a review from showuon June 9, 2022 14:32
Copy link
Member

Choose a reason for hiding this comment

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

is this intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if this unused, we should remove it ?

Copy link
Member Author

@ijuma ijuma Jun 9, 2022

Choose a reason for hiding this comment

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

No :) The test doesn't pass without this line. Fixed.

The workaround is not required with mockito.
@ijuma ijuma force-pushed the remove-easymock-hack-replica-manager-test branch from 5c96617 to 22251a9 Compare June 9, 2022 16:10
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if this unused, we should remove it ?

@divijvaidya
Copy link
Member

The failing test testSnapshotOnlyAfterConfiguredMinBytes is failing due to a bug which creates a snapshot with LONG_MAX epoch. This is documented in https://issues.apache.org/jira/browse/KAFKA-13943. The bug has been fixed in #12224. @ijuma perhaps you may be interested to review that one after this? It will help us reduce flaky test behaviour.

Copy link
Member

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes @ijuma

@jsancio jsancio merged commit f421c00 into apache:trunk Jun 14, 2022
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.

4 participants