Thanks Lucy and Matthias.
I also lean toward the second approach (the ValueJoinerWithKeys overload). Although the initial approach is an easier fix, the future for each approach would be: - Under the second approach: one join family — join/leftJoin — with different joiner flavors. It's internally consistent and learnable from existing knowledge. - Under the first approach: join for some variants and joinByMappedKey for others — a method name that appears nowhere else in the API, justified by an internal detail/nuance (mapped key != record key) Just one thing I'd want to raise: The naming: ValueJoinerWithKeys (plural) is one character different from the existing "ValueJoinerWithKey" (singular). Since both live in the same package and appear together in autocomplete, this naming similarity is an easy long-term footgun (also confusing). Could we consider a less collision-prone name? I don't have a great one yet, but I'd raise it now than after it's public. Thanks - Alieh On Fri, Jun 12, 2026 at 3:06 AM Matthias J. Sax <[email protected]> wrote: > Thanks for digging into the details. > > What you found matches my understanding. Overall, it's a tricky > tradeoff. Personally, I think it's better to keep the more prominent > API, ie, KStream#join() clean, so the alternative to add > `ValueJoinerWithKeys` (plural) might be the better tradeoff. > > Would be good to also hear from others about it. > > > -Matthias > > > On 6/4/26 12:44 PM, Lucy Liu via dev wrote: > > Hi Matthias, thanks for the feedback! > > > > MJS1: Yes, compile-time, not runtime. I reworded that. > > > > MJS2, MJS3, MJS4: > > I agree that introducing the new methods will lead to name asymmetry. If > we > > add a new join/leftJoin overload taking a new functional interface rather > > than introducing new methods, all stream-globalTable joins could keep the > > same join/leftJoin naming. So I explored the last rejected alternative > > further: a new ValueJoinerWithKeys interface `ValueJoinerWithKeys<K1, K2, > > V1, V2, VR>` with `apply(readOnlyKey, streamKey, value1, value2)` > exposing > > both keys, and overloaded the existing join/leftJoin methods to accept > it. > > The four ValueJoinerWithKey overloads get @Deprecated as in the original > > plan, but the migration target is now the new overload rather than a > > renamed method. The ValueJoiner overloads stay untouched. > > For example, the new `join` method would be: > > > >> <GlobalKey, GlobalValue, VOut> KStream<K, VOut> join(final > >> GlobalKTable<GlobalKey, GlobalValue> globalTable, > >> final > >> KeyValueMapper<? super K, ? super V, ? extends GlobalKey> keySelector, > >> final > >> ValueJoinerWithKeys<? super GlobalKey, ? super K, ? super V, ? super > >> GlobalValue, ? extends VOut> joiner); > > > > > > I think this could be a better solution because the overload works > cleanly, > > and the joiner allowing both keys offers greater flexibility. > > This is some draft code changes this approach would introduce: > > > https://urldefense.com/v3/__https://github.com/apache/kafka/pull/22477__;!!Ayb5sqE7!t-Pgojx5hrlsNZCev0a6uksclbJfRMZotq56k4WSPVrACGz_vECOVEhjiAQqr7bMkJEQqbWHHCtUCY-1$ > > > > Happy to learn what others think about this approach. If this approach is > > indeed better, I will update the KIP. > > > > Best regards, > > Lucy Liu > > > > > > > > On Wed, Jun 3, 2026 at 4:43 PM Matthias J. Sax <[email protected]> wrote: > > > >> Thanks for the KIP Lucy. > >> > >> Overall, it seems to be a non-controversial proposal, to solve the issue > >> with new overloads. > >> > >> > >> > >> MJS1: The KIP says: > >> > >>> The discrepancy is not surfaced at runtime > >> > >> What this paragraph describe is a compile time type check, right? Not a > >> runtime check? > >> > >> > >> > >> MJS2: It seems you propose to replace only the 4 methods which take > >> `ValueJoinerWithKey` argument. While this is the smallest change we > >> could make, I am wondering if it introduced some name inconsistencies? > >> So even if technically not necessary, I am wondering if we would get > >> better UX, if we would also deprecate the 4 method taking `ValueJoiner`? > >> This way, all method which are doing the same thing, ie > >> stream-globalTable join have the same name. > >> > >> > >> > >> MJS3: About naming the new methods. I guess both `joinOnMappedKey` and > >> `joinByMappedKey` would work. But I am wondering if > >> `foreignKeyJoin()`would be good names, too? Or maybe > >> `globalKTableJoin()`? Just putting forward some alternatives to spark > >> some thoughts. Naming is always hard. Curious to hear what others think. > >> > >> > >> > >> MJS4: About the last rejected alternative. If my assessment is right, it > >> would have the advantage that we could add new overloads called > >> `join()`, and thus we would get better method naming overall, and my > >> point MJS2 from above would become void? I does have the disadvantage > >> that we need a new interface, but given the naming question, I am > >> wondering if it would provide an overall better tradeoff? -- Again, > >> curious to hear what others think. > >> > >> > >> -Matthias > >> > >> > >> > >> On 5/27/26 10:17 PM, Lucy Liu via dev wrote: > >>> Hi Bill, > >>> > >>> Thanks for the suggestions! I have made some changes accordingly. > >>> BB1: I changed the 4 added methods to use `by` instead of `on`. > >>> BB2: For each deprecated method, an additional warning comment will be > >>> added, stressing that the `readOnlyKey` is the stream key and might not > >> be > >>> behaving as intended. > >>> > >>> Best, > >>> Lucy > >>> > >>> On Wed, May 27, 2026 at 4:07 PM Bill Bejeck <[email protected]> > wrote: > >>> > >>>> Hi Lucy, > >>>> > >>>> Thanks for the KIP! This is a very much needed update to Kafka > Streams. > >>>> Overall the KIP looks good. > >>>> > >>>> BB1. I have one nit comment with regards to the name. What would you > >> think > >>>> about `joinByMappedKey / leftJoinByMappedKey` instead? > >>>> BB2. Since the error condition will continue to exist until we remove > >> the > >>>> deprecated methods, maybe we could add one line to the Javadoc > >> explicitly > >>>> stating that readOnlyKey is currently the stream key, not the mapped > >> join > >>>> key. > >>>> > >>>> Thanks, > >>>> Bill > >>>> > >>>> On Tue, May 26, 2026 at 1:19 PM Lucy Liu via dev < > [email protected]> > >>>> wrote: > >>>> > >>>>> Hi everyone, > >>>>> > >>>>> Gentle ping on this thread. Looking forward to feedback on the KIP! > >>>>> > >>>>> Best, > >>>>> Lucy Liu > >>>>> > >>>>> On Fri, May 15, 2026 at 4:19 PM Lucy Liu <[email protected]> > wrote: > >>>>> > >>>>>> Hello everyone, > >>>>>> > >>>>>> I would like to start a discussion on KIP-1340 Pass Join Key to > >>>>>> `ValueJoinerWithKey` in Streams-GlobalKTable Joins > >>>>>> < > >>>>>> > >>>>> > >>>> > >> > https://urldefense.com/v3/__https://cwiki.apache.org/confluence/display/KAFKA/KIP-1340*3A*Pass*Join*Key*to**A60ValueJoinerWithKey*60*in*Streams-GlobalKTable*Joins__;JSsrKysrJSUrKys!!Ayb5sqE7!sMsc7OEJtzyBJn3kfDmAjAi8gklV-8hDbb2Oiwk_NV0RC8giJPWimU7sX4jc4Niu_29yjxAv1souD4H5kUY$ > >>>>>>> > >>>>>> > >>>>>> This proposal aims to introduce joinOnMappedKey and > >>>> leftJoinOnMappedKey, > >>>>>> new KStream methods that pass the mapped join key (the result of the > >>>>>> user-supplied KeyValueMapper) into ValueJoinerWithKey for > >>>>>> stream-globalTable joins. The existing join/leftJoin overloads pass > >> the > >>>>>> stream record's key into the joiner instead, contradicting KIP-149's > >>>>>> contract that readOnlyKey is the join key and silently producing > wrong > >>>>>> values if a joiner intends to read the real join key. The existing > >>>>>> overloads will be deprecated and removed in a future major release. > >>>>>> > >>>>>> Best regards, > >>>>>> Lucy Liu > >>>>>> > >>>>> > >>>> > >>> > >> > >> > > > >
