You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "jack2012aa (via GitHub)" <gi...@apache.org> on 2025/09/13 19:59:58 UTC
[PR] Kafka-9825: Kafka protocol BNF format should have some way to display tagged fields [kafka]
jack2012aa opened a new pull request, #20536:
URL: https://github.com/apache/kafka/pull/20536
# Description
The protocol guide doesn't display tagged fields in BNF.
# Change
Now tagged fields are displayed as FIELD_NAME<tag number>
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #20536:
URL: https://github.com/apache/kafka/pull/20536#issuecomment-3351935398
> > > > This is a nice improvement but I wonder whether it could be even better with a bit of tweak. Here's an example of the current output.
> > > > The part that could do with improvement is the table entries `__tagged_fields The tagged fields` rows in the table. It's all so nice now, apart from that.
> > >
> > >
> > > @AndrewJSchofield Thanks for the suggestion. Since the ticket didn't mention the table, I have few question on how to modify it.
> > > 1.Are there reasons why empty tagged fields are left in the table? Removing them is easy, but I want to make sure it is appropriate. 2. Should we remove the nested table of tagged fields, to make the format consist to the BNF?
> >
> >
> >
> > 1. My view is that the empty `__tagged_fields` entries are redundant. Now that we are properly rendering the tagged fields, I would remove the empty ones.
> > 2. Provided that it's all comprehensible when compared with the BNF, I think that would be fine.
>
> The new output is included in the description. Thank you!
>
> Besides the current changes, I found an inconsistency in how nested fields are displayed:
>
> * Normal Fields: Their inner fields are flattened into the main table.
> * Tagged Fields: Their inner fields are nested within a separate, wrapped table.
>
> I think unifying this would improve readability. My recommendation is to use the flattened style for all fields (including tagged ones), as the parent-child relationship is already clear from the BNF format next to it. This would make the entire table structure cleaner and more consistent.
>
> What do you think of this approach?
@jack2012aa Thanks for the update. Personally, I prefer the nested table but that's just an opinion.
This PR looks good to me and I will approve. I'll tag @chia7712 for a review because he's commented already. I'll merge in a few days if no more comments are forthcoming.
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "jack2012aa (via GitHub)" <gi...@apache.org>.
jack2012aa commented on PR #20536:
URL: https://github.com/apache/kafka/pull/20536#issuecomment-3344756845
> > > This is a nice improvement but I wonder whether it could be even better with a bit of tweak. Here's an example of the current output.
> > > The part that could do with improvement is the table entries `__tagged_fields The tagged fields` rows in the table. It's all so nice now, apart from that.
> >
> >
> > @AndrewJSchofield Thanks for the suggestion. Since the ticket didn't mention the table, I have few question on how to modify it.
> > 1.Are there reasons why empty tagged fields are left in the table? Removing them is easy, but I want to make sure it is appropriate. 2. Should we remove the nested table of tagged fields, to make the format consist to the BNF?
>
> 1. My view is that the empty `__tagged_fields` entries are redundant. Now that we are properly rendering the tagged fields, I would remove the empty ones.
> 2. Provided that it's all comprehensible when compared with the BNF, I think that would be fine.
The new output is included in the description. Thank you!
Besides the current changes, I found an inconsistency in how nested fields are displayed:
* Normal Fields: Their inner fields are flattened into the main table.
* Tagged Fields: Their inner fields are nested within a separate, wrapped table.
I think unifying this would improve readability. My recommendation is to use the flattened style for all fields (including tagged ones), as the parent-child relationship is already clear from the BNF format next to it. This would make the entire table structure cleaner and more consistent.
What do you think of this approach?
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "jack2012aa (via GitHub)" <gi...@apache.org>.
jack2012aa commented on code in PR #20536:
URL: https://github.com/apache/kafka/pull/20536#discussion_r2347033722
##########
clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java:
##########
@@ -49,7 +51,23 @@ private static void schemaToBnfHtml(Schema schema, StringBuilder b, int indentSi
subTypes.put(field.def.name, type.arrayElementType().get());
}
} else if (type instanceof TaggedFields) {
- b.append("_tagged_fields ");
+ Map<Integer, Field> taggedFields = new TreeMap<>(((TaggedFields) type).fields());
+ taggedFields.forEach((tag, taggedField) -> {
+ if (taggedField.type.isArray()) {
Review Comment:
Sure. I added under the description.
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #20536:
URL: https://github.com/apache/kafka/pull/20536#issuecomment-3343799218
> > This is a nice improvement but I wonder whether it could be even better with a bit of tweak. Here's an example of the current output.
> > <img alt="Screenshot 2025-09-27 at 22 00 36" width="1019" height="672" src="https://private-user-images.githubusercontent.com/13795597/494792419-f760d697-0664-4d13-89c5-8acdac3b9916.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NTkwMjEyMTksIm5iZiI6MTc1OTAyMDkxOSwicGF0aCI6Ii8xMzc5NTU5Ny80OTQ3OTI0MTktZjc2MGQ2OTctMDY2NC00ZDEzLTg5YzUtOGFjZGFjM2I5OTE2LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTA5MjglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwOTI4VDAwNTUxOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU5MDBiNzViZjIwOTlmMTA0NDIxZjA0MTJlNzY5YmNhN2E3MGU2ODY1ZDY5ZWYyOGQ4ZTk3OWZkYjY1MWQyNzMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.mC_RusFgoZDWqTLfngBAaSiu-BKRlnzSapwsicc4nXA">
> > The part that could do with improvement is the table entries `__tagged_fields The tagged fields` rows in the table. It's all so nice now, apart from that.
>
> @AndrewJSchofield Thanks for the suggestion. Since the ticket didn't mention the table, I have few question on how to modify it.
>
> 1.Are there reasons why empty tagged fields are left in the table? Removing them is easy, but I want to make sure it is appropriate. 2. Should we remove the nested table of tagged fields, to make the format consist to the BNF?
1. My view is that the empty `__tagged_fields` entries are redundant. Now that we are properly rendering the tagged fields, I would remove the empty ones.
2. Provided that it's all comprehensible when compared with the BNF, I think that would be fine.
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #20536:
URL: https://github.com/apache/kafka/pull/20536#issuecomment-3327337494
Nice improvement. I guess that `diverging_epoch<0>` means that this is a tagged field with tag `0`. I wonder whether `diverging_epoch<tag:0>` might be clearer. I would not have any idea what the proposed notation means if I had not read this PR.
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #20536:
URL: https://github.com/apache/kafka/pull/20536#issuecomment-3342044685
This is a nice improvement but I wonder whether it could be even better with a bit of tweak. Here's an example of the current output.
<img width="1019" height="672" alt="Screenshot 2025-09-27 at 22 00 36" src="https://github.com/user-attachments/assets/f760d697-0664-4d13-89c5-8acdac3b9916" />
The part that could do with improvement is the table entries `__tagged_fields The tagged fields` rows in the table. It's all so nice now, apart from that.
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20536:
URL: https://github.com/apache/kafka/pull/20536#discussion_r2347002628
##########
clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java:
##########
@@ -49,7 +51,23 @@ private static void schemaToBnfHtml(Schema schema, StringBuilder b, int indentSi
subTypes.put(field.def.name, type.arrayElementType().get());
}
} else if (type instanceof TaggedFields) {
- b.append("_tagged_fields ");
+ Map<Integer, Field> taggedFields = new TreeMap<>(((TaggedFields) type).fields());
+ taggedFields.forEach((tag, taggedField) -> {
+ if (taggedField.type.isArray()) {
Review Comment:
Could you please attach the result for array type?
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "jack2012aa (via GitHub)" <gi...@apache.org>.
jack2012aa commented on PR #20536:
URL: https://github.com/apache/kafka/pull/20536#issuecomment-3342221120
> This is a nice improvement but I wonder whether it could be even better with a bit of tweak. Here's an example of the current output.
>
> <img alt="Screenshot 2025-09-27 at 22 00 36" width="1019" height="672" src="https://private-user-images.githubusercontent.com/13795597/494792419-f760d697-0664-4d13-89c5-8acdac3b9916.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NTkwMjEyMTksIm5iZiI6MTc1OTAyMDkxOSwicGF0aCI6Ii8xMzc5NTU5Ny80OTQ3OTI0MTktZjc2MGQ2OTctMDY2NC00ZDEzLTg5YzUtOGFjZGFjM2I5OTE2LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTA5MjglMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwOTI4VDAwNTUxOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU5MDBiNzViZjIwOTlmMTA0NDIxZjA0MTJlNzY5YmNhN2E3MGU2ODY1ZDY5ZWYyOGQ4ZTk3OWZkYjY1MWQyNzMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.mC_RusFgoZDWqTLfngBAaSiu-BKRlnzSapwsicc4nXA">
> The part that could do with improvement is the table entries `__tagged_fields The tagged fields` rows in the table. It's all so nice now, apart from that.
@AndrewJSchofield Thanks for the suggestion. Since the ticket didn't mention the table, I have few question on how to modify it.
1.Are there reasons why empty tagged fields are left in the table? Removing them is easy, but I want to make sure it is appropriate.
2. Should we remove the nested table of tagged fields, to make the format consist to the BNF?
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield merged PR #20536:
URL: https://github.com/apache/kafka/pull/20536
--
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
Re: [PR] KAFKA-9825 Kafka protocol BNF format should have some way to display tagged fields [kafka]
Posted by "jack2012aa (via GitHub)" <gi...@apache.org>.
jack2012aa commented on PR #20536:
URL: https://github.com/apache/kafka/pull/20536#issuecomment-3329871465
@AndrewJSchofield Thanks for the review!
Exactly, `<tag: t>` will be better. I have pushed a new version and updated the description.
--
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