Hi Victoria, Thanks for the KIP; this is a beneficial addition.
I'm a +1 on the KIP and the changes made: - Using a config object TableJoined - Limiting the static methods to two I have an additional "wild" thought about rolling the `Function<V, KO> foreignKeyExtractor` and `Materialized<K, VR, KeyValueStore<Bytes, byte[]>>` into TableJoined to align and reduce the number of KTable interfaces. But I don't have a strong opinion on this; I am curious to see what others think about this possibility. -Bill On Tue, Sep 21, 2021 at 2:52 AM Matthias J. Sax <mj...@apache.org> wrote: > Thanks for updating the KIP. > > One nit: > > > The existing methods which accept Named will be marked for deprecation > in 4.0. > > We can skip `in 4.0`. (1) The next release will be 3.1 (not 4.0) and (2) > a KIP could always slip into a future release. > > > About `TableJoined`: It seems you propose to add static methods for all > possible parameter combination. We usually try to avoid this to keep the > number of methods low; if we add too many methods, it defeats the > purpose to use a "builder like" config object. > > To me, it seems sufficient to only have two static methods: > > > as(final String name); > > and > > > with(final StreamPartitioner<K, Void> partitioner, > > final StreamPartitioner<KO, Void> otherPartitioner); > > The second one should allow to pass in `null` to only set one of both > partitioners. > > Curious to hear what other think. > > > -Matthias > > On 9/20/21 8:27 PM, Victoria Xia wrote: > > Hi Matthias, > > > > Thanks for having a look at the KIP! I've updated it with your suggestion > > to introduce a new `TableJoined` object with partitioners of type > > `StreamPartitioner<K, Void>` and `StreamPartitioner<KO, Void>`, and to > > deprecate the existing FK join methods which accept a `Named` object > > accordingly. I agree it makes sense to keep the number of join interfaces > > smaller. > > > > Thanks, > > Victoria > > > > On Sat, Sep 18, 2021 at 11:07 AM Matthias J. Sax <mj...@apache.org> > wrote: > > > >> Thanks for the KIP Victoria. > >> > >> As pointed out on the Jira ticket by you, using `<K,V>` and `<KO,VO>` as > >> partitioner types does not really work, because we don't have access to > >> the right value on the left side nor have we access to the left value on > >> the right hand side. -- I like your idea to use `Void` as value types to > >> make it clear to the users that partitioning must be done on the key > only. > >> > >> For the proposed public API change, I would propose not to pass the > >> partitioners directly, but to introduce a config object (similar to > >> `Joined` for stream-table joins, and `StreamJoined` for stream-stream > >> joins). This new object could also implement `NamedOperation` and thus > >> replace `Named`. To this end, we would deprecate the existing methods > >> using `Named` and replace them with the new methods. Net benefit is, > >> that we don't get more overloads (after we removed the deprecated ones). > >> > >> Not sure how we want to call the new object. Maybe `TableJoined` in > >> alignment to `StreamJoined`? > >> > >> > >> -Matthias > >> > >> On 9/15/21 3:36 PM, Victoria Xia wrote: > >>> Hi, > >>> > >>> I've opened a small KIP for adding Kafka Streams support for foreign > key > >>> joins on tables with custom partitioners: > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-775%3A+Custom+partitioners+in+foreign+key+joins > >>> > >>> Feedback appreciated. Thanks! > >>> > >>> - Victoria > >>> > >> > > >
