m1a2st commented on code in PR #18510:
URL: https://github.com/apache/kafka/pull/18510#discussion_r1913502873
##########
tools/src/main/java/org/apache/kafka/tools/consumer/ApiMessageFormatter.java:
##########
@@ -38,7 +38,7 @@ public abstract class ApiMessageFormatter implements
MessageFormatter {
private static final String DATA = "data";
private static final String KEY = "key";
private static final String VALUE = "value";
- static final String UNKNOWN = "unknown";
+ public static final String UNKNOWN = "unknown";
Review Comment:
I think we should use the modifier `protected`.
##########
tools/src/main/java/org/apache/kafka/tools/consumer/ApiMessageFormatter.java:
##########
@@ -79,5 +79,5 @@ public void writeTo(ConsumerRecord<byte[], byte[]>
consumerRecord, PrintStream o
}
protected abstract JsonNode readToKeyJson(ByteBuffer byteBuffer);
- protected abstract JsonNode readToValueJson(ByteBuffer byteBuffer);
-}
+ protected abstract JsonNode readToValueJson(ByteBuffer byteBuffer, short
keyVersion);
Review Comment:
Since the `keyVersion` parameter is only used by the
`ShareGroupStateMessageFormatter` class, we should refactor this to make the
method less abstract. Instead of requiring all subclasses to implement the
keyVersion parameter, we could:
1. Move keyVersion out of the abstract definition
2. Provide a default implementation in the base class
WDYT?
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
