Skip to content

MINOR: remove useless repeated method call in KafkaApiTest#9988

Merged
chia7712 merged 1 commit intoapache:trunkfrom
dengziming:minor-kafkaapitest-optimize
Feb 3, 2021
Merged

MINOR: remove useless repeated method call in KafkaApiTest#9988
chia7712 merged 1 commit intoapache:trunkfrom
dengziming:minor-kafkaapitest-optimize

Conversation

@dengziming
Copy link
Copy Markdown
Member

More detailed description of your change
SetReplicas was invoked twice with same params, I think the author means setIsrs

Summary of testing strategy (including rationale)
QA

Committer Checklist (excluded from commit message)

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

@dengziming
Copy link
Copy Markdown
Member Author

@ijuma Hello, this is your code, PTAL.

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@dengziming nice find and thanks for your patch.

There are 94 test cases but no one does use ISR info?

Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM. It may make sense to assert this in some test too.

@dengziming
Copy link
Copy Markdown
Member Author

dengziming commented Feb 3, 2021

@chia7712 , I checked the code again and found no one uses ISR info. In fact this code is copied when replacing req/resp with auto-generated protocol in #7353, and the original code set ISR=allReplicas.

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Feb 3, 2021

the original code set ISR=allReplicas.

Could you share the link to me? thanks!

@dengziming
Copy link
Copy Markdown
Member Author

@chia7712 chia7712 merged commit 1390d02 into apache:trunk Feb 3, 2021
@dengziming dengziming deleted the minor-kafkaapitest-optimize branch October 8, 2022 11:59
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