Skip to content

KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest#9741

Closed
dengziming wants to merge 1 commit intoapache:trunkfrom
dengziming:remove-parseResponse-impl
Closed

KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest#9741
dengziming wants to merge 1 commit intoapache:trunkfrom
dengziming:remove-parseResponse-impl

Conversation

@dengziming
Copy link
Copy Markdown
Member

More detailed description of your change
#7409 has removed the conversion to Struct when parsing resp, ApiVersionsResponse.parse will be used instead of API_VERSIONS.parseResponse, so the overwrite in ApiKeys.API_VERSIONS is useless.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

@dengziming dengziming changed the title MINOR; Remove redudant parseResponse implimention MINOR; Remove redudant parseResponse implemention Dec 13, 2020
@dengziming dengziming changed the title MINOR; Remove redudant parseResponse implemention MINOR; Remove redudant parseResponse implementation Dec 13, 2020
@dengziming
Copy link
Copy Markdown
Member Author

Hi, @chia7712 , 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 Thanks for your patch. Could you also remove ApiKeys#parseResponse and ApiKeys#parseRequest? They are not used anymore.

@dengziming dengziming changed the title MINOR; Remove redudant parseResponse implementation KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest Dec 14, 2020
@dengziming
Copy link
Copy Markdown
Member Author

@chia7712 Thank you, I also created an issue and changed the title.

@chia7712
Copy link
Copy Markdown
Member

@dengziming Thanks for your updating. requestSchema and responseSchema are not used by production code also. Could you remove them?

@dengziming
Copy link
Copy Markdown
Member Author

dengziming commented Dec 14, 2020

@chia7712 I removed responseSchema which was used in ApiKeysTest.testResponseThrottleTime to inspect all response, I think it's similar to tests in ApiVersionsResponseTest, so I just also moved it.

Comment on lines 184 to 187
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.

@dengziming Do we maintain this fallback mechanism somewhere?

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.

@dengziming dengziming force-pushed the remove-parseResponse-impl branch from 1cffbc4 to bfdac17 Compare December 14, 2020 09:21
@chia7712
Copy link
Copy Markdown
Member

@dengziming I just notice there is a PR (#9748) covering this one. Could you please close this PR and then be a reviewer for that PR?

@dengziming
Copy link
Copy Markdown
Member Author

@chia7712 Oh, sorry for the repeated work, I just closed this pr.

@dengziming dengziming closed this Dec 15, 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