KAFKA-18616; Refactor Tools's ApiMessageFormatter #18695
Conversation
dajac
added
the
do-not-merge
|
@chia7712 What do you think about doing something like this? |
There was a problem hiding this comment.
This makes me wonder whether we should strengthen the version validation in the CoordinatorRecordSerdes.
If the version of the value record bumps in the future, formatters that lack version checks may print incorrect data. In such cases, throwing an accurate exception would be more appropriate.
tools/src/main/java/org/apache/kafka/tools/consumer/CoordinatorRecordMessageFormatter.java
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/consumer/OffsetsMessageFormatter.java
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/consumer/TransactionLogMessageFormatter.java
Show resolved
Hide resolved
|
|
@chia7712 I finally had the time to update this one. Could you take another look when you get a chance? |
dajac
changed the title
dajac
removed
the
do-not-merge
| JsonNode dataNode = readToKeyJson(ByteBuffer.wrap(key)); | ||
| json | ||
| .putObject(KEY) | ||
| .put(TYPE, record.key().apiKey()) |
There was a problem hiding this comment.
ShareGroupStateMessageFormatter use "version" rather than "type", but I guess we fix it in the follow-up by refactoring ShareGroupStateMessageFormatter?
There was a problem hiding this comment.
That's right. We will it in #18510.
| import static java.util.Collections.emptyMap; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) |
There was a problem hiding this comment.
Is it used to speedup those tests?
There was a problem hiding this comment.
It is used to allow using a non-static method in @MethodSource.
|
|
||
| @ParameterizedTest | ||
| @MethodSource("parameters") | ||
| public void testMessageFormatter(byte[] keyBuffer, byte[] valueBuffer, String expectedOutput) { |
There was a problem hiding this comment.
Could you please add unit test for UnknownRecordTypeException?
There was a problem hiding this comment.
Sure. I added test cases to the concrete classes.
|
@chia7712 Thanks for your comments. I just addressed them. |
This patch refactors the `ApiMessageFormatter` to follow what we have done in apache#18688. Reviewers: Chia-Ping Tsai <[email protected]>
This patch refactors the `ApiMessageFormatter` to follow what we have done in apache#18688. Reviewers: Chia-Ping Tsai <[email protected]>
This patch refactors the
ApiMessageFormatterto follow what we have done in #18688.Reviewers: Chia-Ping Tsai [email protected]