hachikuji commented on a change in pull request #10793:
URL: https://github.com/apache/kafka/pull/10793#discussion_r645084226
##########
File path:
server-common/src/main/java/org/apache/kafka/server/common/serialization/AbstractApiMessageSerde.java
##########
@@ -69,16 +76,42 @@ public void write(ApiMessageAndVersion data,
@Override
public ApiMessageAndVersion read(Readable input,
int size) {
- short frameVersion = (short) input.readUnsignedVarint();
+ short frameVersion;
+ try {
+ frameVersion = unsignedIntToShort(input.readUnsignedVarint(),
"frame version");
+ } catch (Exception e) {
+ throw new MetadataParseException("Failed to read variable-length "
+
+ "frame version number: " + e.getClass().getSimpleName() +
": " + e.getMessage());
+ }
if (frameVersion != DEFAULT_FRAME_VERSION) {
throw new SerializationException("Could not deserialize metadata
record due to unknown frame version "
- + frameVersion + "(only
frame version " + DEFAULT_FRAME_VERSION + " is supported)");
+ + frameVersion + "(only frame version " +
DEFAULT_FRAME_VERSION + " is supported)");
+ }
+ short apiKey;
+ try {
+ apiKey = unsignedIntToShort(input.readUnsignedVarint(), "type");
+ } catch (Exception e) {
+ throw new MetadataParseException("Failed to read variable-length
type " +
+ "number: " + e.getClass().getSimpleName() + ": " +
e.getMessage());
+ }
+ short version;
+ try {
+ version = unsignedIntToShort(input.readUnsignedVarint(),
"version");
+ } catch (Exception e) {
+ throw new MetadataParseException("Failed to read variable-length "
+
+ "version number: " + e.getClass().getSimpleName() + ": " +
e.getMessage());
}
-
- short apiKey = (short) input.readUnsignedVarint();
- short version = (short) input.readUnsignedVarint();
ApiMessage record = apiMessageFor(apiKey);
- record.read(input, version);
+ try {
+ record.read(input, version);
+ } catch (Exception e) {
+ throw new MetadataParseException(apiKey + "#parse failed: " +
+ e.getClass().getSimpleName() + ": " + e.getMessage());
Review comment:
nit: perhaps we may as should pass through `e` as the cause?
##########
File path:
server-common/src/main/java/org/apache/kafka/server/common/serialization/AbstractApiMessageSerde.java
##########
@@ -69,16 +76,42 @@ public void write(ApiMessageAndVersion data,
@Override
public ApiMessageAndVersion read(Readable input,
int size) {
- short frameVersion = (short) input.readUnsignedVarint();
+ short frameVersion;
+ try {
+ frameVersion = unsignedIntToShort(input.readUnsignedVarint(),
"frame version");
+ } catch (Exception e) {
+ throw new MetadataParseException("Failed to read variable-length "
+
Review comment:
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?
##########
File path:
server-common/src/main/java/org/apache/kafka/server/common/serialization/AbstractApiMessageSerde.java
##########
@@ -69,16 +76,42 @@ public void write(ApiMessageAndVersion data,
@Override
public ApiMessageAndVersion read(Readable input,
int size) {
- short frameVersion = (short) input.readUnsignedVarint();
+ short frameVersion;
+ try {
+ frameVersion = unsignedIntToShort(input.readUnsignedVarint(),
"frame version");
+ } catch (Exception e) {
+ throw new MetadataParseException("Failed to read variable-length "
+
+ "frame version number: " + e.getClass().getSimpleName() +
": " + e.getMessage());
+ }
if (frameVersion != DEFAULT_FRAME_VERSION) {
throw new SerializationException("Could not deserialize metadata
record due to unknown frame version "
Review comment:
Should we use the parse exception consistently?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
