frankvicky commented on code in PR #23717:
URL: https://github.com/apache/kafka/pull/23717#discussion_r4351432736
##########
clients/src/test/java/org/apache/kafka/common/message/MessageTest.java:
##########
@@ -858,6 +861,77 @@ public void testUnknownTaggedFields() {
verifyWriteSucceeds((short) 6, createTopics);
}
+ @Test
+ public void testTaggedFieldsWrittenInAscendingTagOrder() {
+ // KIP-482 requires tagged fields in strictly ascending tag order.
Review Comment:
nit: I would drop the KIP reference here; the KIP number belongs in the PR
description rather than in source comments.
##########
clients/src/test/java/org/apache/kafka/common/message/MessageTest.java:
##########
@@ -858,6 +861,77 @@ public void testUnknownTaggedFields() {
verifyWriteSucceeds((short) 6, createTopics);
}
+ @Test
+ public void testTaggedFieldsWrittenInAscendingTagOrder() {
+ // KIP-482 requires tagged fields in strictly ascending tag order.
+ // Setters for SimpleExampleMessage's known tagged fields, indexed by
tag.
+ List<Consumer<SimpleExampleMessageData>> knownTagSetters = List.of(
+ m -> m.setMyTaggedIntArray(List.of(1)),
+ m -> m.setMyNullableString("a"),
+ m -> m.setMyInt16((short) 1),
+ m -> m.setMyFloat64(1.0),
+ m -> m.setMyString("b"),
+ m -> m.setMyBytes(new byte[] {0x1}),
+ m -> m.setTaggedUuid(new Uuid(1L, 2L)),
+ m -> m.setTaggedLong(1L),
+ m -> m.setMyTaggedStruct(new
SimpleExampleMessageData.TaggedStruct().setStructId("c")),
+ m -> m.setTaggedLongFlexibleVersionSubset(1L));
+ for (int run = 0; run < 100; run++) {
+ long seed = System.nanoTime() + run;
Review Comment:
I don't think the randomization buys us anything here.
The write order is fixed at code-generation time, and the setters each touch
an independent field, so the order in which they are called cannot affect the
output.
Randomly choosing a subset of tags only toggles independent `if` blocks.
A deterministic `@ParameterizedTest` over versions 1 and 2 that sets every
known tagged field (8 for v1, 10 for v2) plus a few unknown tags would verify
the same property without the 100-iteration loop and the seed plumbing.
##########
clients/src/test/java/org/apache/kafka/common/message/MessageTest.java:
##########
@@ -858,6 +861,77 @@ public void testUnknownTaggedFields() {
verifyWriteSucceeds((short) 6, createTopics);
}
+ @Test
+ public void testTaggedFieldsWrittenInAscendingTagOrder() {
+ // KIP-482 requires tagged fields in strictly ascending tag order.
+ // Setters for SimpleExampleMessage's known tagged fields, indexed by
tag.
+ List<Consumer<SimpleExampleMessageData>> knownTagSetters = List.of(
+ m -> m.setMyTaggedIntArray(List.of(1)),
+ m -> m.setMyNullableString("a"),
+ m -> m.setMyInt16((short) 1),
+ m -> m.setMyFloat64(1.0),
+ m -> m.setMyString("b"),
+ m -> m.setMyBytes(new byte[] {0x1}),
+ m -> m.setTaggedUuid(new Uuid(1L, 2L)),
+ m -> m.setTaggedLong(1L),
+ m -> m.setMyTaggedStruct(new
SimpleExampleMessageData.TaggedStruct().setStructId("c")),
+ m -> m.setTaggedLongFlexibleVersionSubset(1L));
+ for (int run = 0; run < 100; run++) {
+ long seed = System.nanoTime() + run;
Review Comment:
If you do want to keep randomness, `TestUtils.SEEDED_RANDOM` is what the
rest of the clients tests use.
`System.nanoTime()` seeds make a failure non-reproducible unless someone
copies the seed out of the log.
##########
clients/src/test/java/org/apache/kafka/common/message/MessageTest.java:
##########
@@ -858,6 +861,77 @@ public void testUnknownTaggedFields() {
verifyWriteSucceeds((short) 6, createTopics);
}
+ @Test
+ public void testTaggedFieldsWrittenInAscendingTagOrder() {
+ // KIP-482 requires tagged fields in strictly ascending tag order.
+ // Setters for SimpleExampleMessage's known tagged fields, indexed by
tag.
+ List<Consumer<SimpleExampleMessageData>> knownTagSetters = List.of(
+ m -> m.setMyTaggedIntArray(List.of(1)),
+ m -> m.setMyNullableString("a"),
+ m -> m.setMyInt16((short) 1),
+ m -> m.setMyFloat64(1.0),
+ m -> m.setMyString("b"),
+ m -> m.setMyBytes(new byte[] {0x1}),
+ m -> m.setTaggedUuid(new Uuid(1L, 2L)),
+ m -> m.setTaggedLong(1L),
+ m -> m.setMyTaggedStruct(new
SimpleExampleMessageData.TaggedStruct().setStructId("c")),
+ m -> m.setTaggedLongFlexibleVersionSubset(1L));
+ for (int run = 0; run < 100; run++) {
+ long seed = System.nanoTime() + run;
+ Random random = new Random(seed);
+ short version = (short) (1 + random.nextInt(2));
+ // Tags 8 and 9 are only valid from version 2.
+ int numKnownTags = version == 1 ? 8 : knownTagSetters.size();
+ SimpleExampleMessageData message = new SimpleExampleMessageData();
+ List<Integer> expectedTags = new ArrayList<>();
+ for (int tag = 0; tag < numKnownTags; tag++) {
+ if (random.nextBoolean()) {
+ expectedTags.add(tag);
+ }
+ }
+ // Set the known tagged fields in random order, so the written
order cannot come from it.
+ Collections.shuffle(expectedTags, random);
+ for (int tag : expectedTags) {
+ knownTagSetters.get(tag).accept(message);
+ }
+ // Unknown tags are the ones the schema does not define, so they
are above all known tags.
+ // They must be added in ascending order, since
RawTaggedFieldWriter rejects an unsorted list.
+ int unknownTag = knownTagSetters.size() - 1;
+ int numUnknownTags = random.nextInt(5);
+ for (int i = 0; i < numUnknownTags; i++) {
+ unknownTag += 1 + random.nextInt(1000);
+ message.unknownTaggedFields().add(new
RawTaggedField(unknownTag, new byte[random.nextInt(3)]));
+ expectedTags.add(unknownTag);
+ }
+ List<Integer> writtenTags = writtenTaggedFieldTags(message,
version);
+ for (int i = 1; i < writtenTags.size(); i++) {
+ assertTrue(
+ writtenTags.get(i - 1) < writtenTags.get(i),
+ "Tags " + writtenTags + " are not strictly ascending,
seed=" + seed);
+ }
+ assertEquals(Set.copyOf(expectedTags), Set.copyOf(writtenTags),
"Failed with seed=" + seed);
Review Comment:
These two assertions can be collapsed into one that is also stricter:
```java
assertEquals(expectedTags.stream().sorted().toList(), writtenTags);
```
##########
clients/src/test/java/org/apache/kafka/common/message/MessageTest.java:
##########
@@ -858,6 +861,77 @@ public void testUnknownTaggedFields() {
verifyWriteSucceeds((short) 6, createTopics);
}
+ @Test
+ public void testTaggedFieldsWrittenInAscendingTagOrder() {
+ // KIP-482 requires tagged fields in strictly ascending tag order.
+ // Setters for SimpleExampleMessage's known tagged fields, indexed by
tag.
+ List<Consumer<SimpleExampleMessageData>> knownTagSetters = List.of(
+ m -> m.setMyTaggedIntArray(List.of(1)),
+ m -> m.setMyNullableString("a"),
+ m -> m.setMyInt16((short) 1),
+ m -> m.setMyFloat64(1.0),
+ m -> m.setMyString("b"),
+ m -> m.setMyBytes(new byte[] {0x1}),
+ m -> m.setTaggedUuid(new Uuid(1L, 2L)),
+ m -> m.setTaggedLong(1L),
+ m -> m.setMyTaggedStruct(new
SimpleExampleMessageData.TaggedStruct().setStructId("c")),
+ m -> m.setTaggedLongFlexibleVersionSubset(1L));
+ for (int run = 0; run < 100; run++) {
+ long seed = System.nanoTime() + run;
+ Random random = new Random(seed);
+ short version = (short) (1 + random.nextInt(2));
+ // Tags 8 and 9 are only valid from version 2.
+ int numKnownTags = version == 1 ? 8 : knownTagSetters.size();
+ SimpleExampleMessageData message = new SimpleExampleMessageData();
+ List<Integer> expectedTags = new ArrayList<>();
+ for (int tag = 0; tag < numKnownTags; tag++) {
+ if (random.nextBoolean()) {
+ expectedTags.add(tag);
+ }
+ }
+ // Set the known tagged fields in random order, so the written
order cannot come from it.
+ Collections.shuffle(expectedTags, random);
+ for (int tag : expectedTags) {
+ knownTagSetters.get(tag).accept(message);
+ }
+ // Unknown tags are the ones the schema does not define, so they
are above all known tags.
Review Comment:
This statement isn't true in general.
Unknown tags can be lower than the highest known tag.
for example, in version 1 of this message tags 8 and 9 are unknown, and a
schema may have gaps between its tags.
That matters for coverage. The generator interleaves unknown tags between
known ones by emitting `_rawWriter.writeRawTags(_writable, <nextKnownTag>)`
wherever there is a gap. `SimpleExampleMessage` has contiguous tags 0-9, so the
generated writer only calls `writeRawTags(_writable, Integer.MAX_VALUE)` at the
very end, and this test only exercises that trailing path. I checked the
production schemas under `clients/src/main/resources/common/message` and none
of them has a tag gap either, so the interleaving path has no test anywhere in
the repo.
If the goal is "tags are written in ascending order", the two cases worth
adding are:
1. version 1 with `RawTaggedField`s for tags 8 and 9, asserting they are
written after tag 7;
2. a test schema with a gap in its tags (either a new field in
`SimpleExampleMessage.json` or a small dedicated schema), with unknown tags
placed inside the gap.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
