Thanks. Here is the PR for this catch: https://github.com/apache/kafka/pull/14187
On Thu, 10 Aug 2023 at 19:04, Matthias J. Sax <mj...@apache.org> wrote: > Good catch about the JavaDocs. > > Seems we missed to update them when we did K10277. Would you like to do > a PR to fix them right away for upcoming 3.6 release? > > If there is no more other comments, I think you can start a VOTE thread. > > > -Matthias > > On 8/10/23 4:22 AM, Florin Akermann wrote: > > Thank you for the feedback. > > > >> Not sure if this is the right phrasing? > > > > You are right. I adjusted the phrasing accordingly. > > Given your description of the current behavior, do I understand correctly > > that the current documentation for the left join KStream-GlobalKtable is > > out of date? > > > https://github.com/apache/kafka/blob/9318b591d7a57b9db1e7519986d78f0402cd5b5e/streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java#L2948C7-L2948C7 > > > >> I would remove this from the KIP > > > > I agree,Removed. > > Plus, relevant doc links added. > > > >> I think the way it's phrased is good. [...] We can cover details on the > > PR. > > > > Great. Yes, in general, I hope everybody agrees that we shouldn't add > more > > details to this KIP > > > > > > On Thu, 10 Aug 2023 at 00:16, Matthias J. Sax <mj...@apache.org> wrote: > > > >> Thanks for the KIP. > >> > >>> left join KStream-GlobalTable: no longer drop left records with > null-key > >> and call KeyValueMapper with 'null' for left key. The case where > >> KeyValueMapper returns null is already handled in the current > >> implementation. > >> > >> Not sure if this is the right phrasing? In the end, even now, the stream > >> input record key can be null (cf > >> https://issues.apache.org/jira/browse/KAFKA-10277) -- a stream record > is > >> only dropped if the `KeyValueMapper` returns `null` (note that the > >> key-extractor has no default implemenation but is a required argument) > >> -- this KIP would relax this case for left-join. > >> > >> > >>> In the pull request all relevant Javadocs will be updated with the > >> information on how to keep the old behavior for a given operator / > method. > >> > >> I would remove this from the KIP -- I am also not sure if we should put > >> it into the JavaDoc? -- I agree that it should go into the upgrade docs > >> as well as "join section" in the docs: > >> > >> > https://kafka.apache.org/35/documentation/streams/developer-guide/dsl-api.html#joining > >> > >> We also have > >> > >> > https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Streams+Join+Semantics > >> that we should update. > >> > >> > >>> I added a remark about the repartition of null-key records. > >> > >> I think the way it's phrase is good. In the end, it's an optimization to > >> drop records upstream (instead of piping them through the topic and drop > >> the downstream), and thus we don't have to cover it in the KIP in > >> detail. In general, for aggregations we can still apply the > >> optimization, however, we need to be careful as we could also have two > >> downstream operators with a shared repartition topic: for this case, we > >> can only drop upstream if all downstream operator would drop null-key > >> records anyway. We can cover details on the PR. > >> > >> > >> > >> -Matthias > >> > >> > >> > >> On 8/9/23 5:39 AM, Florin Akermann wrote: > >>> Hi All, > >>> > >>> I added a remark about the repartition of null-key records. > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-962%3A+Relax+non-null+key+requirement+in+Kafka+Streams#KIP962:RelaxnonnullkeyrequirementinKafkaStreams-Repartitionofnull-keyrecords > >>> > >>> Can we relax this constraint for any kind of repartitioning or should > it > >>> only be relaxed in the context of left stream-table and left/outer > >>> stream-stream joins? > >>> > >>> Florin > >>> > >>> On Mon, 7 Aug 2023 at 13:23, Florin Akermann < > florin.akerm...@gmail.com> > >>> wrote: > >>> > >>>> Hi Lucas, > >>>> > >>>> Thanks. I added the point about the upgrade guide as well. > >>>> > >>>> Florin > >>>> > >>>> On Mon, 7 Aug 2023 at 11:06, Lucas Brutschy <lbruts...@confluent.io > >> .invalid> > >>>> wrote: > >>>> > >>>>> Hi Florin, > >>>>> > >>>>> thanks for the KIP! This looks good to me. I agree that the precise > >>>>> Java doc wording doesn't have to be discussed as part of the KIP. > >>>>> > >>>>> I would also suggest to include an update to > >>>>> https://kafka.apache.org/documentation/streams/upgrade-guide > >>>>> > >>>>> Cheers, > >>>>> Lucas > >>>>> > >>>>> On Mon, Aug 7, 2023 at 10:51 AM Florin Akermann > >>>>> <florin.akerm...@gmail.com> wrote: > >>>>>> > >>>>>> Hi Both, > >>>>>> > >>>>>> Thanks. > >>>>>> I added remarks to account for this. > >>>>>> > >>>>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-962%3A+Relax+non-null+key+requirement+in+Kafka+Streams#KIP962:RelaxnonnullkeyrequirementinKafkaStreams-Remarks > >>>>>> > >>>>>> In short, let's add a note in the Java docs? The exact wording of > the > >>>>> note > >>>>>> can be scrutinized in the pull request? > >>>>>> > >>>>>> What do you think? > >>>>>> > >>>>>> > >>>>>> On Sun, 6 Aug 2023 at 19:41, Guozhang Wang < > >> guozhang.wang...@gmail.com> > >>>>>> wrote: > >>>>>> > >>>>>>> I'm just thinking we can try to encourage users to migrate from XX > to > >>>>>>> XXWithKey in the docs, giving this as one good example that the > >> latter > >>>>>>> can help you distinguish different scenarios whereas the former > >>>>>>> cannot. > >>>>>>> > >>>>>>> On Fri, Aug 4, 2023 at 6:32 PM Matthias J. Sax <mj...@apache.org> > >>>>> wrote: > >>>>>>>> > >>>>>>>> Guozhang, > >>>>>>>> > >>>>>>>> thanks for pointing out ValueJoinerWithKey. In the end, it's just > a > >>>>>>>> documentation change, ie, point out that the passed in key could > be > >>>>>>>> `null` and similar? > >>>>>>>> > >>>>>>>> -Matthias > >>>>>>>> > >>>>>>>> > >>>>>>>> On 8/2/23 3:20 PM, Guozhang Wang wrote: > >>>>>>>>> Thanks Florin for the writeup, > >>>>>>>>> > >>>>>>>>> One quick thing I'd like to bring up is that in KIP-149 > >>>>>>>>> ( > >>>>>>> > >>>>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-149%3A+Enabling+key+access+in+ValueTransformer%2C+ValueMapper%2C+and+ValueJoiner > >>>>>>> ) > >>>>>>>>> we introduced ValueJoinerWithKey which is aimed to enhance > >>>>>>>>> ValueJoiner. It would have a benefit for this KIP such that > >>>>>>>>> implementers can distinguish "null-key" v.s. "not-null-key but > >>>>>>>>> null-value" scenarios. > >>>>>>>>> > >>>>>>>>> Hence I'd suggest we also include the semantic changes with > >>>>>>>>> ValueJoinerWithKey, which can help distinguish these two > >>>>> scenarios, > >>>>>>>>> and also document that if users apply ValueJoiner only, they may > >>>>> not > >>>>>>>>> have this benefit, and hence we suggest users to use the former. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> Guozhang > >>>>>>>>> > >>>>>>>>> On Mon, Jul 31, 2023 at 12:11 PM Florin Akermann > >>>>>>>>> <florin.akerm...@gmail.com> wrote: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>> > >>>>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-962%3A+Relax+non-null+key+requirement+in+Kafka+Streams > >>>>>>> > >>>>> > >>>> > >>> > >> > > >
