Hi, Chia-Ping shared in the RC5 thread now two cases where the behavior under discussion actually exists prior to 3.9 so all my arguments here are moot. I missed those two cases because they have the same name as the struct introduced in 3.9, and my regex excluded that by name when I was looking for existing cases.
> It seems like we both agree that the implicit defaults are documented. I showed you where in the README.md they are discussed. Technically, it was *I* who showed *you* that 😅 I feel like you have been ignoring most of the details I have been writing in this thread. Including now with the reasoning about saved bytes ... I again feel paraphrased. > Perhaps the disagreement is that you assumed that they didn't apply to tagged fields, whereas I assumed that it was obvious that they did. It's more narrow than this, and I guess the fact that this is not understood yet is why we have been going in circles. It was already clear that tagged struct fields are considered to have a default value when all of the struts fields have an explicit default value. I opened a thread about improving the KIP process such that it accounts for documentation improvements over time, having that in place would be a motivation for me to contribute further to the documentation of the wire protocol. Cheers, Anton Den mån 28 okt. 2024 kl 17:02 skrev Colin McCabe <cmcc...@apache.org>: > On Sun, Oct 27, 2024, at 01:44, Anton Agestam wrote: > > Colin > > > > I have presented four reasons, I'll list them again below. Please let me > > know which ones there isn't already enough information on the thread > > already. > > > > - The behavior is new. > > Hi Anton, > > This behavior isn't new. I gave an example of tagged fields that have an > implicit default in 3.8 and earlier. > > > - The behavior is undocumented. > > It seems like we both agree that the implicit defaults are documented. I > showed you where in the README.md they are discussed. That section is from > 2019. Perhaps the disagreement is that you assumed that they didn't apply > to tagged fields, whereas I assumed that it was obvious that they did. > > It looks like Chia-Ping Tsai has opened a JIRA to clarify that implicit > defaults do indeed apply to tagged fields. I think this will help avoid > confusion in the future. > > > - The behavior is bad API design. > > Why is it bad API design? > > > - The behavior does not really save bytes *in practice*. > > The example you gave shows that the current behavior sends less over the > wire than your proposed change. Those are not theoretical bytes, they are > actual bytes. > > Saving space on the wire for fields that were not often used was one of > the explicit goals of the tagged fields KIP, which was KIP-482. As it says > in the "motivation" section: > > > While [the current] versioning scheme allows us to change the message > schemas over > > time, there are many scenarios that it doesn't support well. One > scenario > > that isn't well-supported is when we have data that should be sent in > some > > contexts, but not others. For example, when a MetadataRequest is made > > with IncludeClusterAuthorizedOperations set to true, we need to include > > the authorized operations in the response. However, even when > > IncludeClusterAuthorizedOperations is set to false, we still must waste > > bandwidth sending a set of blank authorized operations fields in the > > response. The problem is that the field that is semantically optional > in > > the message, but that is can't be expressed in the type system for the > > Kafka RPC protocol. > > You can read it here: https://cwiki.apache.org/confluence/x/OhMyBw > > Obviously sending defaults over the wire, in cases where this is not > needed, goes against that. > > > > > I don't see why *fixing* the release candidate to not break documented > > behavior should require a KIP, I would actually expect the opposite -- > the > > new behavior that is being introduced should really have required one. > > > >> These two behaviors, taken together, save space on the wire > > > > Then you are implicitly arguing that the combination of host="" port=0 > > are common enough that this will practically save bytes on the wire, I > find > > that hard to believe. > > > > For any future schema that we want to save bytes, there is just as much > > opportunity to save bytes on the wire with my proposal, they just have to > > explicitly define default nested values in order to do so. > > As I said, there is nothing special about 3.9. This behavior has always > existed. > > If you really want to force everyone to explicitly declare a default for > each field, then just introduce a KIP to do that. I wouldn't vote for it (I > still don't see why this is better), but this would at least follow our > usual process. > > One of the problems with forcing an explicit default everywhere is that we > don't really have a syntax for specifying that the default should be the > empty collection. For collections, the only choice you get is explicitly > declaring that the default is null. > > best, > Colin > > > > > > BR, > > Anton > > > > > > Den sön 27 okt. 2024 kl 02:56 skrev Colin McCabe <cmcc...@apache.org>: > > > >> Hi Anton, > >> > >> The behavior where we leave out tagged fields when the default value is > >> present is intentional. As is the behavior where default values are > treated > >> as 0, the empty array, etc. when defaults are not explicitly specified. > >> These two behaviors, taken together, save space on the wire, and are > >> simpler for us to implement than what you have proposed. You haven't > >> presented a single reason why this should change. > >> > >> There simply isn't any reason to change the current tagged fields > >> behavior. And even if we wanted to, it would require a KIP, whereas 3.9 > is > >> past KIP freeze (along with feature freeze, code freeze, and every other > >> freeze.) > >> > >> best, > >> Colin > >> > >> > >> On Sat, Oct 26, 2024, at 09:45, Anton Agestam wrote: > >> > Hi Colin, > >> > > >> >> The current behavior is older than 3.9 > >> > > >> > No it is not. The *Java implementation* is older than 3.9. But the > >> behavior > >> > is a sum of Java implementation + JSON schema. It's a new construct of > >> JSON > >> > schema that exposes the new behavior. It is fully possible to adjust > >> > behavior only for the new kind of construct and remain fully > equivalent > >> in > >> > behavior with 3.8 for everything else. > >> > > >> >> a lot of things depend on it > >> > > >> > Do you have an example of this? The field you pointed at is an array > >> field, > >> > so it doesn't exercise the behavior we are talking about here. > >> > > >> >> I also disagree that the implicit default behavior is bad. > >> > > >> > This is a mischaracterization of my previous messages. I am NOT > proposing > >> > to change any of the implicit default behavior that Kafka 3.8 > exercises. > >> > You are not disagreeing with me because I have not stated that the > >> implicit > >> > default behavior in general is what is bad. > >> > > >> >> we might have to agree to disagree about > >> > > >> > That's always an acceptable outcome, however, I think it would be very > >> sad > >> > to see this decision made without a clear understanding of the > problem. > >> > > >> > This proof of concept pull request demonstrates that it is possible > to do > >> > what I am suggesting: https://github.com/apache/kafka/pull/17608 > >> > > >> > BR, > >> > Anton > >> > > >> > Den fre 25 okt. 2024 kl 23:32 skrev Colin McCabe <cmcc...@apache.org > >: > >> > > >> >> Hi Anton, > >> >> > >> >> I appreciate your desire for clarity in the specification. If you > think > >> >> there's something we could do to clarify the docs, let me know (or > >> better > >> >> yet, file a PR!) > >> >> > >> >> But we cannot change the behavior of defaults for tagged fields. The > >> >> current behavior is older than 3.9 and a lot of things depend on it. > For > >> >> example, if you go back to 3.8, you'll find a tagged field in > >> >> ApiVersionsResponse.json named SupportedFeatures. It doesn't specify > an > >> >> explicit default. Therefore it gets the implicit default of the empty > >> array > >> >> if nothing is sent for this field. (I don't think this field was new > in > >> >> 3.8, either, but I'm just giving a random example). > >> >> > >> >> I also disagree that the implicit default behavior is bad. Forcing > >> >> everyone to specify a default for every field would have been > verbose, > >> and > >> >> in most cases not necessary. While there are some things I wish I had > >> done > >> >> differently about the protocol generator, this isn't one of them. > Maybe > >> >> this is a matter of opinion, though, that we might have to agree to > >> >> disagree about... > >> >> > >> >> best, > >> >> Colin > >> >> > >> >> > >> >> On Fri, Oct 25, 2024, at 01:37, Anton Agestam wrote: > >> >> > Hi again Colin, > >> >> > > >> >> >> The implicit defaults have been documented for a long time > >> >> > > >> >> > Yes, if you read my message you will see I am referencing and > quoting > >> >> that > >> >> > exact documentation. The new behavior is clearly excluded by the > >> explicit > >> >> > pre requisite in that exact documentation. > >> >> > > >> >> > Again in summary (all three of these points have more details in my > >> >> > previous message, so I'll refrain from repeating too much): > >> >> > > >> >> > - This is new behavior. There was no nested tagged entity field > with > >> >> > non-explicit defaults in any API schema version up until 3.9. > >> >> > - The new behavior is _not_ specified in documentation. The > existing > >> >> > documentation is specific under which circumstances it applies. > >> >> > - The new behavior is bad. > >> >> > > >> >> >> It is, as you said, "a quirk of the implementation" but it's a > quirk > >> >> that > >> >> > you have to know to speak the Kafka protocol > >> >> > > >> >> > This is somewhat paraphrasing. It is a *quirk of the implementation > >> >> > introduced in 3.9*, without any formalization of the change, and > with > >> >> > consequences that will be extremely cumbersome to undo. Now is the > >> only > >> >> > time to fix this without introducing backwards incompatible > semantics. > >> >> > > >> >> > In terms of specification, this is the addition that I am > proposing: > >> >> > "nested tagged entity fields are only to be considered as having an > >> >> > implicit default value if all of fields in the nested entity also > have > >> >> > explicit defaults". This would make no difference for any message > >> prior > >> >> to > >> >> > 3.9. On 3.9, it will only have an effect on > UpdateRaftVoterResponse, > >> >> where, > >> >> > if you want to send a CurrentLeader object that matches the > implicit > >> >> > defaults, this will have to be serialized fully on the wire. Do you > >> see > >> >> any > >> >> > concrete consequences of that change? > >> >> > > >> >> > As an alternative that would not require improving the protocol > >> >> > specification, the currentLeader field could be made optional and > have > >> >> null > >> >> > as an explicit default. I think it's a worse solution, but it > defers > >> >> having > >> >> > to improve the specification. > >> >> > > >> >> >> Perhaps there is something we could do to improve these docs? Or > >> somehow > >> >> > make them easier to find? > >> >> > > >> >> > Yes, I'm happy to discuss this, but for the sake of keeping this > >> >> discussion > >> >> > on topic, can we do that separately? There are some related Jira > >> tickets > >> >> > for this. > >> >> > > >> >> > Cheers, > >> >> > Anton > >> >> > > >> >> > > >> >> > Den tors 24 okt. 2024 kl 18:31 skrev Colin McCabe < > cmcc...@apache.org > >> >: > >> >> > > >> >> >> Hi Anton, > >> >> >> > >> >> >> The implicit defaults have been documented for a long time. Take a > >> look > >> >> at > >> >> >> this documentation from 2019 in > >> >> >> clients/src/main/resources/common/message/README.md : > >> >> >> > >> >> >> > Deserializing Messages > >> >> >> > ---------------------- > >> >> >> > Message objects may be deserialized using the Message#read > method. > >> >> This > >> >> >> method > >> >> >> > overwrites all the data currently in the message object with new > >> data. > >> >> >> > > >> >> >> > Any fields in the message object that are not present in the > >> version > >> >> >> that you > >> >> >> > are deserializing will be reset to default values. Unless a > custom > >> >> >> default has > >> >> >> > been set: > >> >> >> > > >> >> >> > * Integer fields default to 0. > >> >> >> > > >> >> >> > * Floats default to 0. > >> >> >> > > >> >> >> > * Booleans default to false. > >> >> >> > > >> >> >> > * Strings default to the empty string. > >> >> >> > > >> >> >> > * Bytes fields default to the empty byte array. > >> >> >> > > >> >> >> > * Uuid fields default to zero uuid. > >> >> >> > > >> >> >> > * Records fields default to null. > >> >> >> > > >> >> >> > * Array fields default to empty. > >> >> >> > > >> >> >> > You can specify "null" as a default value for a string field by > >> >> >> specifying the > >> >> >> > literal string "null". Note that you can only specify null as a > >> >> default > >> >> >> if all > >> >> >> > versions of the field are nullable. > >> >> >> > >> >> >> Any implementation of sending and receiving Kafka messages needs > to > >> know > >> >> >> about these defaults. It is, as you said, "a quirk of the > >> >> implementation" > >> >> >> but it's a quirk that you have to know to speak the Kafka > protocol. > >> The > >> >> >> defaults apply to both tagged and untagged fields. The main > >> difference > >> >> is > >> >> >> that untagged fields are always present except in older versions, > >> >> whereas > >> >> >> tagged fields may not be present even in the most recent version. > >> >> >> > >> >> >> Perhaps there is something we could do to improve these docs? Or > >> somehow > >> >> >> make them easier to find? > >> >> >> > >> >> >> best, > >> >> >> Colin > >> >> >> > >> >> >> > >> >> >> On Wed, Oct 23, 2024, at 00:19, Anton Agestam wrote: > >> >> >> > Hi Colin, > >> >> >> > > >> >> >> > I understand your perspective that this is a bug in the client, > >> but I > >> >> >> > disagree that this is something that's established, or > inferrable > >> from > >> >> >> > existing specification and implementation. > >> >> >> > > >> >> >> > The behavior of exposing the implicit defaults on the wire > protocol > >> >> >> really > >> >> >> > is new. The only scenario that they have come into play prior to > >> this > >> >> >> > change is when parsing an entity version that is not the latest > >> >> version > >> >> >> > that the parsing code base supports -- and -- when that code > base > >> >> models > >> >> >> > all versions of an API with the same entity. In this scenario > the > >> >> >> implicit > >> >> >> > defaults must be there to fill in newly added fields with _some_ > >> >> value in > >> >> >> > lack of being able to read from the message. However, for an > >> >> >> implementation > >> >> >> > that doesn't reuse the same class for every version, there has > >> never > >> >> been > >> >> >> > any need to use the implicit defaults, as when each version has > a > >> >> >> dedicated > >> >> >> > class it doesn't need to define any fields for values that are > >> added > >> >> in > >> >> >> > future versions. We have a comprehensive compatibility test > suite > >> in > >> >> kio > >> >> >> > that proves this is the case, we can parse and serialize the > full > >> >> >> protocol > >> >> >> > up to version 3.8.0 (every version of every message). This would > >> not > >> >> have > >> >> >> > been possible if the implicit defaults were semantically > required, > >> >> >> because > >> >> >> > we do not use them. > >> >> >> > > >> >> >> > The reason that the above hasn't become visible until now, as I > >> >> mentioned > >> >> >> > in the bug report, is that the newly introduced protocol > message is > >> >> the > >> >> >> > first one (of all message versions in the whole protocol) to > make > >> use > >> >> of > >> >> >> > this construct: a nested entity field where not all nested > fields > >> >> have an > >> >> >> > explicit default value. > >> >> >> > > >> >> >> > Further, the new behavior is not in accordance with what is > >> specified. > >> >> >> > The > >> >> >> > implicit defaults are really only documented to be used in the > >> above > >> >> >> > mentioned scenario, with the following prerequisite > >> >> >> > < > >> >> >> > >> >> > >> > https://github.com/apache/kafka/tree/trunk/clients/src/main/resources/common/message#deserializing-messages > >> >> >> >: > >> >> >> > "Any fields in the message object that are not present in the > >> version > >> >> >> > that > >> >> >> > you are deserializing will be reset to default values". That > >> >> >> > prerequisite > >> >> >> > is not fulfilled here, so this cannot be said to be in > accordance > >> with > >> >> >> > what > >> >> >> > is specified. The behavior occurs even though I am parsing a > >> message > >> >> of > >> >> >> > the > >> >> >> > _same_, latest, version as the schema of the model. > >> >> >> > > >> >> >> > In summary, this is new behavior, and it is unspecified > behavior. > >> >> >> > > >> >> >> > On top of that, I think this is also bad API design, as it > forces > >> >> >> > cumbersome and strange semantics into client implementations. In > >> the > >> >> >> > context of parsing a model that has more fields than the data > it is > >> >> >> > parsing, it makes sense to have implicit defaults. However, when > >> the > >> >> >> model > >> >> >> > has the same amount of fields as there are values in the data, > this > >> >> does > >> >> >> > not make sense. Why should we force client implementations to > >> induce > >> >> non > >> >> >> > defaults into their data structures? The empty string for a > >> hostname, > >> >> and > >> >> >> > zero for a port are clearly not good defaults, both are really > >> invalid > >> >> >> > values. > >> >> >> > > >> >> >> > The choice of implicitly introducing this behavior at the > protocol > >> >> level > >> >> >> in > >> >> >> > this way will most likely mean that this becomes a quirk of the > >> >> protocol > >> >> >> > forever. I think this situation is avoidable, and I think this > is a > >> >> >> > decision that should not be made silently without proper > >> specification > >> >> >> and > >> >> >> > design process to make sure it is really the right decision. > >> >> >> > > >> >> >> > BR, > >> >> >> > Anton > >> >> >> > > >> >> >> > Den mån 21 okt. 2024 kl 23:12 skrev Colin McCabe < > >> cmcc...@apache.org > >> >> >: > >> >> >> > > >> >> >> >> Hi all, > >> >> >> >> > >> >> >> >> I have posted a new release candidate, RC3. See the RC3 thread. > >> >> >> >> > >> >> >> >> best, > >> >> >> >> Colin > >> >> >> >> > >> >> >> >> On Mon, Oct 21, 2024, at 11:31, Colin McCabe wrote: > >> >> >> >> > Hi Anton, > >> >> >> >> > > >> >> >> >> > I replied on the JIRA. I do not think this is a bug, you just > >> >> failed > >> >> >> to > >> >> >> >> > account for implicit defaults in your protocol code. That > is, 0 > >> is > >> >> the > >> >> >> >> > default of numeric fields if no other default is specified, > etc. > >> >> >> >> > > >> >> >> >> > best, > >> >> >> >> > Colin > >> >> >> >> > > >> >> >> >> > On Mon, Oct 21, 2024, at 08:07, Anton Agestam wrote: > >> >> >> >> >> Hi everyone, > >> >> >> >> >> > >> >> >> >> >> I have found a protocol serialization bug that surfaces only > >> with > >> >> >> one of > >> >> >> >> >> the entities introduced for KIP-853 > (UpdateRaftVoterResponse). > >> >> >> >> >> > >> >> >> >> >> Due to the irreversible implications this might have once > >> merged, > >> >> I'd > >> >> >> >> argue > >> >> >> >> >> that this needs to be considered a release blocker. > >> >> >> >> >> > >> >> >> >> >> https://issues.apache.org/jira/browse/KAFKA-17845 > >> >> >> >> >> > >> >> >> >> >> BR, > >> >> >> >> >> Anton > >> >> >> >> >> > >> >> >> >> >> Den tors 10 okt. 2024 kl 23:16 skrev Colin McCabe < > >> >> >> cmcc...@apache.org>: > >> >> >> >> >> > >> >> >> >> >>> This is the second candidate for the release of Apache > Kafka > >> >> 3.9.0. > >> >> >> I > >> >> >> >> have > >> >> >> >> >>> titled it rc2 since I had an rc1 which got very far, even > to > >> the > >> >> >> point > >> >> >> >> of > >> >> >> >> >>> pushing tags and docker images, before I spotted an issue. > So > >> >> rather > >> >> >> >> than > >> >> >> >> >>> mutate the tags, I decided to skip over rc1. > >> >> >> >> >>> > >> >> >> >> >>> - This is a major release, the final one in the 3.x line. > >> (There > >> >> >> may of > >> >> >> >> >>> course be other minor releases in this line, such as > 3.9.1.) > >> >> >> >> >>> - Tiered storage will be considered production-ready in > this > >> >> >> release. > >> >> >> >> >>> - This will be the final major release to feature the > >> deprecated > >> >> >> >> ZooKeeper > >> >> >> >> >>> mode. > >> >> >> >> >>> > >> >> >> >> >>> This release includes the following KIPs: > >> >> >> >> >>> - KIP-853: Support dynamically changing KRaft controller > >> >> membership > >> >> >> >> >>> - KIP-1057: Add remote log metadata flag to the dump log > tool > >> >> >> >> >>> - KIP-1049: Add config log.summary.interval.ms to Kafka > >> Streams > >> >> >> >> >>> - KIP-1040: Improve handling of nullable values in > >> InsertField, > >> >> >> >> >>> ExtractField, and other transformations > >> >> >> >> >>> - KIP-1031: Control offset translation in > >> MirrorSourceConnector > >> >> >> >> >>> - KIP-1033: Add Kafka Streams exception handler for > exceptions > >> >> >> >> occurring > >> >> >> >> >>> during processing > >> >> >> >> >>> - KIP-1017: Health check endpoint for Kafka Connect > >> >> >> >> >>> - KIP-1025: Optionally URL-encode clientID and > clientSecret in > >> >> >> >> >>> authorization header > >> >> >> >> >>> - KIP-1005: Expose EarliestLocalOffset and TieredOffset > >> >> >> >> >>> - KIP-950: Tiered Storage Disablement > >> >> >> >> >>> - KIP-956: Tiered Storage Quotas > >> >> >> >> >>> > >> >> >> >> >>> Release notes for the 3.9.0 release: > >> >> >> >> >>> > >> >> >> >> > >> >> >> > >> >> > >> > https://dist.apache.org/repos/dist/dev/kafka/3.9.0-rc2/RELEASE_NOTES.html > >> >> >> >> >>> > >> >> >> >> >>> *** Please download, test and vote by October 16, 2024. > >> >> >> >> >>> > >> >> >> >> >>> Kafka's KEYS file containing PGP keys we use to sign the > >> release: > >> >> >> >> >>> https://kafka.apache.org/KEYS > >> >> >> >> >>> > >> >> >> >> >>> * Release artifacts to be voted upon (source and binary): > >> >> >> >> >>> https://dist.apache.org/repos/dist/dev/kafka/3.9.0-rc2/ > >> >> >> >> >>> > >> >> >> >> >>> * Docker release artifacts to be voted upon: > >> >> >> >> >>> apache/kafka:3.9.0-rc2 > >> >> >> >> >>> apache/kafka-native:3.9.0-rc2 > >> >> >> >> >>> > >> >> >> >> >>> * Maven artifacts to be voted upon: > >> >> >> >> >>> > >> >> >> > >> https://repository.apache.org/content/groups/staging/org/apache/kafka/ > >> >> >> >> >>> > >> >> >> >> >>> * Javadoc: > >> >> >> >> >>> > >> https://dist.apache.org/repos/dist/dev/kafka/3.9.0-rc2/javadoc/ > >> >> >> >> >>> > >> >> >> >> >>> * Documentation: > >> >> >> >> >>> https://kafka.apache.org/39/documentation.html > >> >> >> >> >>> > >> >> >> >> >>> * Protocol: > >> >> >> >> >>> https://kafka.apache.org/39/protocol.html > >> >> >> >> >>> > >> >> >> >> >>> * Tag to be voted upon (off 3.9 branch) is the 3.9.0-rc2 > tag: > >> >> >> >> >>> https://github.com/apache/kafka/releases/tag/3.9.0-rc2 > >> >> >> >> >>> > >> >> >> >> >>> * Successful Docker Image Github Actions Pipeline for 3.9 > >> branch: > >> >> >> >> >>> Docker Build Test Pipeline (JVM): > >> >> >> >> >>> https://github.com/apache/kafka/actions/runs/11281563007 > >> >> >> >> >>> Docker Build Test Pipeline (Native): > >> >> >> >> >>> https://github.com/apache/kafka/actions/runs/11281608809 > >> >> >> >> >>> > >> >> >> >> >>> Thanks to everyone who helped with this release candidate, > >> >> either by > >> >> >> >> >>> contributing code, testing, or documentation. > >> >> >> >> >>> > >> >> >> >> >>> Regards, > >> >> >> >> >>> Colin > >> >> >> >> >>> > >> >> >> >> > >> >> >> > >> >> > >> >