Skip to content

KAFKA-12338: Remove useless MetadataParser#10793

Merged
hachikuji merged 4 commits into
apache:trunkfrom
dengziming:KAFKA-12338-remove-metadata-parser
Jun 7, 2021
Merged

KAFKA-12338: Remove useless MetadataParser#10793
hachikuji merged 4 commits into
apache:trunkfrom
dengziming:KAFKA-12338-remove-metadata-parser

Conversation

@dengziming

Copy link
Copy Markdown
Member

More detailed description of your change
MetadataParser is a duplication of MetadataRecordSerde and it's not used in any code, so we can remove it.

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.

QA

@hachikuji

Copy link
Copy Markdown
Contributor

@dengziming Thanks for the patch. Looks like the parser has some additional validations which are not present in MetadataRecordSerde. Are any of them worth porting over?

@dengziming

Copy link
Copy Markdown
Member Author

@hachikuji Thanks for the remindings, I think the validations in read are useful and I copied them to MetadataRecordSerde, but not sure whether the DEFAULT_FRAME_VERSION_SIZE check can be applied to MetadataRecordSerde, because the logic in MetadataParser is size=apiKey+apiVersion+message while in MetadataRecordSerde is size=frameVersion+apiKey+apiVersion+message.

@dengziming
dengziming force-pushed the KAFKA-12338-remove-metadata-parser branch from 0eef960 to a51b2bc Compare June 3, 2021 14:27
try {
frameVersion = unsignedIntToShort(input.readUnsignedVarint(), "frame version");
} catch (Exception e) {
throw new MetadataParseException("Failed to read variable-length " +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like MetadataParseException is already the only thing which can be raised by unsignedIntToShort. Is it necessary to catch it and raise a new instance?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In fact input.readUnsignedVarint() may throw a IlleagelArgumentException if it fails to read a short, this can be seen in MetadataRecordSerdeTest.testParsingMalformedFrameVersionVarint.

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the updates. Just a few more small comments.

Comment thread metadata/src/test/java/org/apache/kafka/metadata/MetadataRecordSerdeTest.java Outdated
@dengziming

Copy link
Copy Markdown
Member Author

@hachikuji I update the code according to your suggestions and also make the test more accurate, please take a look again.

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks for the patch!

@hachikuji
hachikuji merged commit 51665b9 into apache:trunk Jun 7, 2021
@dengziming
dengziming deleted the KAFKA-12338-remove-metadata-parser branch November 24, 2022 06:46
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.

2 participants