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://github.com/apache/kafka/pull/22477 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 > >>>> > >>> > >> > > > >
