KAFKA-8885: The Kafka Protocol should Support Optional Tagged Fields by cmccabe · Pull Request #7325 · apache/kafka

cmccabe · 2019-09-11T21:40:07Z

No description provided.

WithClientID was annoying by having to specify a string pointer; instead this adds a second opt to nil the client id if necessary. Doing so is likely highly uncommon. Adds controlled shutdown v0 correct encoding; noticed when scanning apache/kafka#7325.

twmb · 2019-09-22T21:14:03Z

+* "string": a string.  Strings are serialized as a length followed by the
+  contents as UTF-8.  The contents must be less than 64kb in size.  In
+  non-flexible versions, the string length will always be 2 bytes.  In flexible
+  versions, the length is a variable-length integer. 


KIP-482 indicates that flexible versions will actually use unsigned variable length numbers, offset by 1 to reserve 0 indicating null. If that's the case, this should change from variable-length integer, which implies the old zig-zag varint, to unsigned variable-length integer offset by 1, with 0 indicating null (or something like that).

Also, it might be worth mentioning that the numbers are always offset by one, even if the field is non-nullable.

I'm going to split the documentation part off into a separate PR, since there are some tricky questions about what should go in what documentation section. Let's discuss it there.

cmccabe · 2019-09-23T23:32:42Z

Hi all, I have split this PR up into several other PRs to be more reviewable, including #7372, #7344, #7340, and more to come. I'm going to leave this one up for now so that people can see the bigger context of some of the changes, however. Maybe I will rebase this one and use it for the last part once the other ones are in. Thanks.

dajac

Thanks for the PR @cmccabe. Overall, it looks good to me. I have left few minor comments.

dajac · 2019-09-30T11:36:37Z

+  //
+  // Version 3 is the first flexible version.
+  "validVersions": "0-3",
+  "flexibleVersions": "3+",


Leaving the comment here because I haven't found a better place. It would be great if you could add tests in RequestResponseTest#testSerialization to cover all the versions which have been bumped.

Hmm. We don't generally do exhaustive testing of all versions in RequestResponseTest. There would be a lot of entries! But I agree we should add more stuff in RequestResponseTest. Let's think about it in a follow-on PR

+1 for additional tests. Testing every version is not crazy. We have been bitten a few times already due to untested version bumps.

ijuma

Thanks for the PR. I took an initial pass and left some high level questions and a couple of nits.

ijuma · 2019-10-02T14:36:39Z

+    default List<RawTaggedField> readRawTaggedField(List<RawTaggedField> unknowns, int tag, int size) {
+        if (unknowns == null) {
+            unknowns = new ArrayList<>();
        }


Are we doing this to avoid the allocation of unknowns unless there is at least one unknown?

ijuma · 2019-10-02T14:44:29Z

    }
+
+    @Test
+    public void testInvalidFieldName() {


It would be helpful to indicate what is invalid about the name. Is it the underscore at the start?

I added some JavaDoc

ijuma · 2019-10-02T14:45:15Z

+                "    { \"name\": \"_badName\", \"type\": \"[]int32\", \"versions\": \"0+\" }",
+                "  ]",
+                "}")), MessageSpec.class);
+            fail("Expected MessageDataGenerator constructor to fail");


I would suggest using assertThrows in this and other tests added in this PR that validate that an exception is thrown.

ijuma · 2019-10-02T14:46:45Z

+ * A compact array represents its length with a varint rather than a
+ * fixed-length field.
+ */
+public class CompactArrayOf extends DocumentedType {


We also talked about arrays of primitives having a compact representation where tags are not needed per element. How would we describe such arrays, are they packed arrays versus compact arrays?

They could be either ArrayOf and CompactArrayOf. We don't have a separate array type for arrays of objects vs. arrays of non-object types

cmccabe · 2019-10-03T00:56:43Z

I have split the changes to message/README.md into a follow-on PR.
I rebased on trunk and fixed a few minor conflicts.
Split headerVersion into requestHeaderVersion and responseHeaderVersion.

ijuma · 2019-10-03T04:25:15Z

There are some compiler errors after the latest updates.

cmccabe · 2019-10-03T16:51:26Z

The compiler errors should be fixed now.

hachikuji

Thanks, looking good overall. I left a few comments.

hachikuji · 2019-10-04T01:01:03Z

Seems a bit messy to support different value types in the same map. Are we saving that much by not having separate maps?

The memory overhead of having a separate map would be pretty large in the common case where objects are small.

hachikuji · 2019-10-04T01:01:58Z

I couldn't find any uses for this code in any of the generated classes. Do we have test cases which exercise this logic?

The test message file SimpleExampleMessage.json contains a tagged array, which will use this logic. I will add a test that uses that field.

hachikuji · 2019-10-04T01:12:51Z

Maybe readUnknownTaggedField?

hachikuji · 2019-10-04T01:55:44Z

nit: use vararg constructor. A couple below as well

hachikuji · 2019-10-04T04:31:55Z

nit: would be nice to document the expected type of fields

hachikuji · 2019-10-04T05:53:54Z

Could probably use Collections.emptyList() here

the problem is that this field is mutable, and Collections.emptyList returns something immutable

hachikuji · 2019-10-04T05:58:39Z

This is nifty

hachikuji · 2019-10-04T06:11:34Z

I wonder if you have given any thought to limiting allocations like this. For example, in the case of the byte array, we may be able to validate the size using the available bytes in the request

I do think we're kind of goofy to allow arrays with 2**31 elements. There must be a reasonable maximum we could set lower than that. But there will probably be some compatibility implications to this, so it will take time to impose a reasonable limit now....

hachikuji · 2019-10-04T06:21:55Z

Not a big deal, but there are a few cases where we could use a null check of _taggedField instead of a version check. Might make the generated code a little more readable.

It would be a bit complex to change now since we're also filtering versions that aren't present at all and so on

hachikuji · 2019-10-04T06:33:36Z

I think there's a bug in the handling of nullable arrays when the default is not null. For example, consider the following field:

{ "name": "field2", "type": "[]BlahType", "versions": "1+", "taggedVersions": "1+", "tag": 1, "nullableVersions": "1+", "fields": [ { "name": "wootId", "versions": "1+", "type": "int32" }, ] }

This results in the following code:

if (_version >= 1) { if (!field2.isEmpty()) { if (field2 == null) { _taggedFields.put(1, null); } else { Struct[] _nestedObjects = new Struct[field2.size()]; int i = 0; for (BlahType element : this.field2) { _nestedObjects[i++] = element.toStruct(_version); } _taggedFields.put(1, _nestedObjects); } } }

The null check should come first. Seems like the default value optimization needs to take into account nullable values. The same bug affects size.

In general, we probably need more testing, especially for default value handling.

Thanks for finding this. It might be better to address it in a follow on, since the fix could get complicated. I'll push what I have for now.

Rename ObjectSizeCache to ObjectSerializationCache Prefix readable, writable, and size with an underscore in the generated code to avoid conflicting with message fields that have these names. Create MessageTestUtil.

* Fix code generation for tagged array fields * Rename TestUUID to SimpleExampleMessage and add some tests for tagged fields there. * Fix a bug in generating the code for tagged array fields

cmccabe · 2019-10-04T21:43:41Z

Responded to all comments
I rebased on trunk to catch up with the KIP-511 changes.
Fixed the bugs in ApiMessageTypeTest and ApiMessageType.
Fixed some other miscellaneous issues with ApiVersionsRequest.

I filed some follow-on JIRAs:

KAFKA-8984: Improve tagged fields documentation
KAFKA-8985: Use flexibleVersions with LeaderAndIsr, and improve RequestResponseTest coverage
KAFKA-8986: Allow null as a valid default for tagged fields

hachikuji · 2019-10-05T22:12:37Z

Still at least one test failure. This one is reproducible locally:

12:22:00 kafka.server.ApiVersionsRequestTest > testApiVersionsRequest STARTED
12:22:06 kafka.server.ApiVersionsRequestTest.testApiVersionsRequest failed, log available in /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk11-scala2.13/core/build/reports/testOutput/kafka.server.ApiVersionsRequestTest.testApiVersionsRequest.test.stdout
12:22:06 
12:22:06 kafka.server.ApiVersionsRequestTest > testApiVersionsRequest FAILED
12:22:06     java.lang.OutOfMemoryError: Java heap space
12:22:06         at org.apache.kafka.common.utils.ImplicitLinkedHashCollection.clear(ImplicitLinkedHashCollection.java:566)
12:22:06         at org.apache.kafka.common.utils.ImplicitLinkedHashCollection.<init>(ImplicitLinkedHashCollection.java:530)
12:22:06         at org.apache.kafka.common.utils.ImplicitLinkedHashMultiCollection.<init>(ImplicitLinkedHashMultiCollection.java:52)
12:22:06         at org.apache.kafka.common.message.ApiVersionsResponseData$ApiVersionsResponseKeyCollection.<init>(ApiVersionsResponseData.java:615)
12:22:06         at org.apache.kafka.common.message.ApiVersionsResponseData.read(ApiVersionsResponseData.java:137)
12:22:06         at org.apache.kafka.common.message.ApiVersionsResponseData.<init>(ApiVersionsResponseData.java:87)
12:22:06         at org.apache.kafka.common.requests.ApiVersionsResponse.parse(ApiVersionsResponse.java:88)
12:22:06         at kafka.server.ApiVersionsRequestTest.sendApiVersionsRequest(ApiVersionsRequestTest.scala:81)
12:22:06         at kafka.server.ApiVersionsRequestTest.testApiVersionsRequest(ApiVersionsRequestTest.scala:48)

ijuma · 2019-10-06T15:45:04Z

retest this please

ijuma · 2019-10-07T00:34:48Z

Failures are all flakes.

ijuma · 2019-10-07T00:34:57Z

retest this please

ijuma · 2019-10-07T03:40:23Z

Tests passed locally:

BUILD SUCCESSFUL in 42m 58s
150 actionable tasks: 139 executed, 11 up-to-date

hachikuji · 2019-10-07T04:09:45Z

I will go ahead and merge. The failing tests are known to be flaky prior to this patch.

cmccabe · 2019-10-07T04:17:38Z

Thanks, @ijuma and @hachikuji! And all the other reviewers who helped with this

twmb · 2019-10-08T09:28:19Z

Edit: concern retracted; after consideration, I think that tags on every struct level is fine.

cmccabe · 2019-10-09T19:11:24Z

Hi @twmb, thanks for looking at this. As you probably figured out (I see you edited your comment a bit), it's important to allow tagged fields to be added without a version bump. Otherwise we don't get a lot of the benefits of a flexible schema. This does require an extra byte per struct. There was a lot of discussion about this on the mailing list. The discussion period was actually much longer than the implementation period and definitely was not done at the last minute. I looked for alternate solutions that didn't require the extra byte, but they were all very awkward and complex.

To counteract the extra space taken, we implemented more efficient serialization for strings, bytes, and arrays. In the common case where these fields are small, we save between 1 and 3 bytes per object. So if the objects in your hypothetical array of objects contain any of these things, the overhead is already cancelled out.

I agree that it is annoying that an []int32 is now different from []MySingleMemberStruct which contains an int32 each. The issue here is that there are a bunch of places where we pass around a short list of int32s to represent nodes, and it seemed excessive to add a byte of overhead to each. Another issue is that there were optimizations for calculating the message size based on the fact that each entry was the same length. I wanted to keep those optimizations. So therefore, although it was a judgement call, I proposed a design where arrays of primitives could be serialized in the old, simpler way.

I hope this answers all the questions (and potential ones?) :) There is more discussion about this on the mailing list if you want to go in depth. I always appreciate feedback and I made a point of pulling in some Kafka client authors before this was finalized.

cmccabe mentioned this pull request Sep 11, 2019

KIP-482 WIP #7234

Closed

cmccabe force-pushed the KIP-482-III branch 9 times, most recently from 0616758 to ef30342 Compare September 13, 2019 22:20

twmb reviewed Sep 22, 2019

View reviewed changes

Comment thread clients/src/main/resources/common/message/README.md

twmb reviewed Sep 22, 2019

View reviewed changes

cmccabe force-pushed the KIP-482-III branch from ef30342 to 0bb3970 Compare September 26, 2019 15:28

dajac reviewed Sep 30, 2019

View reviewed changes

ijuma reviewed Oct 2, 2019

View reviewed changes

cmccabe force-pushed the KIP-482-III branch from 0bb3970 to e1fbbee Compare October 3, 2019 02:23

hachikuji reviewed Oct 4, 2019

View reviewed changes

cmccabe added 8 commits October 4, 2019 13:48

KAFKA-8885: The Kafka Protocol should Support Optional Tagged Fields

0effb0f

Revert message/README.md changes since we'll do those in a follow-on

d91ae87

Fix merge conflicts

b9db536

split headerVersion into requestHeaderVersion and responseHeaderVersion

f2313ce

address review comments

af6c68d

Fix some naming issues and awkward code

2123734

Rename ObjectSizeCache to ObjectSerializationCache Prefix readable, writable, and size with an underscore in the generated code to avoid conflicting with message fields that have these names. Create MessageTestUtil.

Use assertThrows in MessageDataGeneratorTest

cd2c35e

Fix compiler errors

d4cba0a

cmccabe added 6 commits October 4, 2019 13:48

Fix tagged array fields

03cfab8

* Fix code generation for tagged array fields * Rename TestUUID to SimpleExampleMessage and add some tests for tagged fields there. * Fix a bug in generating the code for tagged array fields

Fix a bug where UUID objects were compared using == instead of equals()

66f2288

Add a test of tagged arrays to SimpleExampleMessageTest

45db026

Address some review comments

aff3e1d

Do not use flexibleVersions for inter-broker RPCs (yet)

c8bd076

Fix header stuff

d6c0e73

cmccabe force-pushed the KIP-482-III branch from 547d83e to d6c0e73 Compare October 4, 2019 21:28

cmccabe added 4 commits October 4, 2019 15:51

Fix SaslAuthenticatorTest

4fdc750

Fix MessageTest

3453bb3

Fix some unit tests and a networkclient bug

b38f6da

Fix checkstyle

dc8cefc

Fix ApiVersionsRequestTest failure

5bdfa27

hachikuji approved these changes Oct 7, 2019

View reviewed changes

hachikuji merged commit 0de61a4 into apache:trunk Oct 7, 2019

Conversation

cmccabe commented Sep 11, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Sep 23, 2019

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Oct 3, 2019

Uh oh!

ijuma commented Oct 3, 2019

Uh oh!

cmccabe commented Oct 3, 2019

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

twmb commented Oct 8, 2019 •

edited

Loading