You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/12/01 14:20:35 UTC
[GitHub] [kafka] dajac opened a new pull request, #12932: [WIP] KAFKA-14425; The Kafka protocol should support nullable structs
dajac opened a new pull request, #12932:
URL: https://github.com/apache/kafka/pull/12932
WIP
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac merged pull request #12932: KAFKA-14425; The Kafka protocol should support nullable structs
Posted by GitBox <gi...@apache.org>.
dajac merged PR #12932:
URL: https://github.com/apache/kafka/pull/12932
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #12932: KAFKA-14425; The Kafka protocol should support nullable structs
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12932:
URL: https://github.com/apache/kafka/pull/12932#issuecomment-1339754347
@cmccabe Could you review this one please? The vote is not closed yet but we have all the votes.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #12932: KAFKA-14425; The Kafka protocol should support nullable structs
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12932:
URL: https://github.com/apache/kafka/pull/12932#issuecomment-1343231442
The KIP has been accepted.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #12932: KAFKA-14425; The Kafka protocol should support nullable structs
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12932:
URL: https://github.com/apache/kafka/pull/12932#issuecomment-1341277577
Thanks for this, @dajac . Can you add a test that the `Invalid default for struct field` message appears as expected when the default is set to something other than `null`? `MessageDataGeneratorTest.java` has some examples of tests like that
The other test that I really want is a test that a tagged nullable struct will be correctly ignored by code prior to this PR. Unfortunately I don't have a good suggestion for how to do that in junit or ducktape. Can you do some kind of manual test to ensure that this is the case?
(Once we have people using tagged nullable structs the ducktape cross-version compatibility tests will serve as a de-facto test of this, but that will come later)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #12932: KAFKA-14425; The Kafka protocol should support nullable structs
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12932:
URL: https://github.com/apache/kafka/pull/12932#issuecomment-1341640919
> Can you add a test that the Invalid default for struct field message appears as expected when the default is set to something other than null? MessageDataGeneratorTest.java has some examples of tests like that
That makes sense. Added tests.
> The other test that I really want is a test that a tagged nullable struct will be correctly ignored by code prior to this PR. Unfortunately I don't have a good suggestion for how to do that in junit or ducktape. Can you do some kind of manual test to ensure that this is the case?
I did the following. With this PR, I used the following schema and code to serialize a message.
```
{
"name": "TestMessage",
"type": "header",
"validVersions": "0",
"flexibleVersions": "0+",
"fields": [
{ "name": "struct1", "type": "Struct", "versions": "0+", "nullableVersions": "0+", "default": "null",
"tag": 1, "taggedVersions": "0+", "fields": [
{ "name": "int1", "type": "int32", "versions": "0+" }
]
},
{ "name": "int0", "type": "int32", "versions": "0+", "tag": 0, "taggedVersions": "0+" }
]
}
```
```
TestMessageData.Struct struct = new TestMessageData.Struct()
.setInt1(100);
TestMessageData message = new TestMessageData()
.setStruct1(struct)
.setInt0(200);
ByteBuffer buffer = MessageUtil.toByteBuffer(message, (short) 0);
FileChannel fc = new FileOutputStream("/tmp/test-message").getChannel();
fc.write(buffer);
fc.close();
```
Then, with trunk, I used the following schema and code to validate.
```
{
"name": "TestMessage",
"type": "header",
"validVersions": "0",
"flexibleVersions": "0+",
"fields": [
{ "name": "int0", "type": "int32", "versions": "0+", "tag": 0, "taggedVersions": "0+" }
]
}
```
```
FileChannel fc = new FileInputStream("/tmp/test-message").getChannel();
ByteBuffer buffer = ByteBuffer.allocate((int) fc.size());
fc.read(buffer);
fc.close();
buffer.flip();
TestMessageData message = new TestMessageData();
message.read(new ByteBufferAccessor(buffer), (short) 0);
assertEquals(200, message.int0());
assertEquals(1, message.unknownTaggedFields().size());
```
That worked as expected.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org