KIP has been updated. thanks On Wed, 23 Aug 2017 at 09:10 Damian Guy <damian....@gmail.com> wrote:
> Hi Matthias, > > >> KStream: >> leftJoin and outerJoin for KStream/KTable join should not have >> `JoinWindows` parameter >> >> Thanks! > > >> >> Nit: TopologyBuilder -> Topology >> >> Ack > > >> Nit: new class Serialized list static method #with twice >> >> Ack > > >> WindowedKStream -> for consistency we should either have GroupedKStream >> or KWindowedStream... (similar argument for SessionWindowedKStream) >> >> We can't rename KGroupedStream -> GroupedKStream without breaking > compatibility. So we are stuck with it for now. Hopefully in the future we > can rename KGroupedStream to GroupedKStream. > > >> >> KGroupedStream >> -> why do we use a different name for `sessionWindowedBy()` -- seems to >> be cleaner to call both methods `windowedBy()` >> >> > I beg to differ that it is cleaner either way! > > >> >> StreamsBuilder#stream -> parameter order is confusing... We should have >> Pattern as second parameter to align both methods. >> >> Ack > > >> StreamsBuilder#table/globalTable -> move parameter `Consumed` as first >> parameter to align with `#stream` >> >> >> Ack > >> Produced#with(Serde, Serde) >> Produced#with(StreamPartitioner, Serde, Serde) >> -> should StreamPartitioner be the third argument instead of the first? >> >> Sure > >> >> Consumed: >> Why do we need 3 different names for the 3 static methods? I would all >> of them just call `with()`. Current names sound clumsy to me. And a >> plain `with()` also aligns with the naming of static methods of other >> classes. >> > > I disagree that the names sound clumsy! But yes they should be aligned > with the others. > > >> >> >> I guess we are also deprecation a bunch of method for >> KStream/KTable/KGroupedStream/KGroupedTable and should mention which >> one? There is just one sentence "Deprecate the existing overloads.", but >> we don't deprecate all existing once. I personally don't care to much if >> we spell deprecated method out explicitly, but right now it's not >> consistent as we only list methods we add. >> >> > >> Should we deprecate `StateStoreSupplier`? >> > Yep > >> >> >> -Matthias >> >> >> >> On 8/22/17 6:55 AM, Damian Guy wrote: >> > I've just updated the KIP with some additional changes targeted at >> > StreamsBuilder >> > >> > Thanks, >> > Damian >> > >> > On Thu, 10 Aug 2017 at 12:59 Damian Guy <damian....@gmail.com> wrote: >> > >> >> >> >>> Got it, thanks. >> >>> >> >>> Does it still make sense to have one static constructors for each >> spec, >> >>> with one constructor having only one parameter to make it more usable, >> >>> i.e. >> >>> as a user I do not need to give all parameters if I only want to >> override >> >>> one of them? Maybe we can just name the constructors as `with` but >> I'm not >> >>> sure if Java distinguish: >> >>> >> >>> public static <K, V> Produced<K, V> with(final Serde<K> keySerde) >> >>> public static <K, V> Produced<K, V> with(final Serde<V> valueSerde) >> >>> >> >>> as two function signatures. >> >>> >> >>> >> >> No that won't work. That is why we have all options, i.e., on Produce >> >> public static <K, V> Produced<K, V> with(final Serde<K> keySerde, >> final Serde<V> >> >> valueSerde) >> >> public static <K, V> Produced<K, V> with(final StreamPartitioner<K, V> >> >> partitioner, final Serde<K> keySerde, final Serde<V> valueSerde) >> >> public static <K, V> Produced<K, V> keySerde(final Serde<K> keySerde) >> >> public static <K, V> Produced<K, V> valueSerde(final Serde<V> >> valueSerde) >> >> public static <K, V> Produced<K, V> streamPartitioner(final >> StreamPartitioner<K, >> >> V> partitioner) >> >> >> >> So if you only want to use one you can just use the function that takes >> >> one argument. >> >> >> >>> >> >>> Guozhang >> >>> >> >>> >> >>> On Wed, Aug 9, 2017 at 6:20 AM, Damian Guy <damian....@gmail.com> >> wrote: >> >>> >> >>>> On Tue, 8 Aug 2017 at 20:11 Guozhang Wang <wangg...@gmail.com> >> wrote: >> >>>> >> >>>>> Damian, >> >>>>> >> >>>>> Thanks for the proposal, I had a few comments on the APIs: >> >>>>> >> >>>>> 1. Printed#withFile seems not needed, as users should always spec if >> >>> it >> >>>> is >> >>>>> to sysOut or to File at the beginning. In addition as a second >> >>> thought, I >> >>>>> think serdes are not useful for prints anyways since we assume >> >>> `toString` >> >>>>> is provided except for byte arrays, in which we will special handle >> >>> it. >> >>>>> >> >>>>> >> >>>> +1 >> >>>> >> >>>> >> >>>>> Another comment about Printed in general is it differs with other >> >>> options >> >>>>> that it is a required option than optional one, since it includes >> >>>> toSysOut >> >>>>> / toFile specs; what are the pros and cons for including these two >> in >> >>> the >> >>>>> option and hence make it a required option than leaving them at the >> >>> API >> >>>>> layer and make Printed as optional for mapper / label only? >> >>>>> >> >>>>> >> >>>> It isn't required as we will still have the no-arg print() which will >> >>> just >> >>>> go to sysout as it does now. >> >>>> >> >>>> >> >>>>> >> >>>>> 2.1 KStream#through / to >> >>>>> >> >>>>> We should have an overloaded function without Produced? >> >>>>> >> >>>> >> >>>> Yes - we already have those so they are not part of the KIP, i.e, >> >>>> through(topic) >> >>>> >> >>>> >> >>>>> >> >>>>> 2.2 KStream#groupBy / groupByKey >> >>>>> >> >>>>> We should have an overloaded function without Serialized? >> >>>>> >> >>>> >> >>>> Yes, as above >> >>>> >> >>>>> >> >>>>> 2.3 KGroupedStream#count / reduce / aggregate >> >>>>> >> >>>>> We should have an overloaded function without Materialized? >> >>>>> >> >>>> >> >>>> As above >> >>>> >> >>>>> >> >>>>> 2.4 KStream#join >> >>>>> >> >>>>> We should have an overloaded function without Joined? >> >>>>> >> >>>> >> >>>> as above >> >>>> >> >>>>> >> >>>>> >> >>>>> 2.5 Each of KTable's operators: >> >>>>> >> >>>>> We should have an overloaded function without Produced / Serialized >> / >> >>>>> Materialized? >> >>>>> >> >>>>> >> >>>> as above >> >>>> >> >>>> >> >>>>> >> >>>>> >> >>>>> 3.1 Produced: the static functions have overlaps, which seems not >> >>>>> necessary. I'd suggest jut having the following three static with >> >>> another >> >>>>> three similar member functions: >> >>>>> >> >>>>> public static <K, V> Produced<K, V> withKeySerde(final Serde<K> >> >>> keySerde) >> >>>>> >> >>>>> public static <K, V> Produced<K, V> withValueSerde(final Serde<V> >> >>>>> valueSerde) >> >>>>> >> >>>>> public static <K, V> Produced<K, V> withStreamPartitioner(final >> >>>>> StreamPartitioner<K, V> partitioner) >> >>>>> >> >>>>> The key idea is that by using the same function name string for >> static >> >>>>> constructor and member functions, users do not need to remember what >> >>> are >> >>>>> the differences but can call these functions with any ordering they >> >>> want, >> >>>>> and later calls on the same spec will win over early calls. >> >>>>> >> >>>>> >> >>>> That would be great if java supported it, but it doesn't. You can't >> have >> >>>> static an member functions with the same signature. >> >>>> >> >>>> >> >>>>> >> >>>>> 3.2 Serialized: similarly >> >>>>> >> >>>>> public static <K, V> Serialized<K, V> withKeySerde(final Serde<K> >> >>>> keySerde) >> >>>>> >> >>>>> public static <K, V> Serialized<K, V> withValueSerde(final Serde<V> >> >>>>> valueSerde) >> >>>>> >> >>>>> public Serialized<K, V> withKeySerde(final Serde<K> keySerde) >> >>>>> >> >>>>> public Serialized<K, V> withValueSerde(final Serde valueSerde) >> >>>>> >> >>>> >> >>>> as above >> >>>> >> >>>> >> >>>>> >> >>>>> Also it has a final Serde<V> otherValueSerde in one of its static >> >>>>> constructor, it that intentional? >> >>>>> >> >>>> >> >>>> Nope: thanks. >> >>>> >> >>>>> >> >>>>> 3.3. Joined: similarly, keep the static constructor signatures the >> >>> same >> >>>> as >> >>>>> its corresponding member fields. >> >>>>> >> >>>>> >> >>>> As above >> >>>> >> >>>> >> >>>>> 3.4 Materialized: it is a bit special, and I think we can keep its >> >>> static >> >>>>> constructors with only two `as` as they are today.K >> >>>>> >> >>>>> >> >>>> 4. Is there any modifications on StateStoreSupplier? Is it replaced >> by >> >>>>> BytesStoreSupplier? Seems some more descriptions are lacking here. >> >>> Also >> >>>> in >> >>>>> >> >>>>> >> >>>> No modifications to StateStoreSupplier. It is superseceded by >> >>>> BytesStoreSupplier. >> >>>> >> >>>> >> >>>> >> >>>>> public static <K, V, S extends StateStore> Materialized<K, V, S> >> >>>>> as(final StateStoreSupplier<S> >> >>>>> supplier) >> >>>>> >> >>>>> Is the parameter in type of BytesStoreSupplier? >> >>>>> >> >>>> >> >>>> Yep - thanks >> >>>> >> >>>> >> >>>>> >> >>>>> >> >>>>> >> >>>>> >> >>>>> Guozhang >> >>>>> >> >>>>> >> >>>>> On Thu, Jul 27, 2017 at 5:26 AM, Damian Guy <damian....@gmail.com> >> >>>> wrote: >> >>>>> >> >>>>>> Updated link: >> >>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- >> >>>>>> 182%3A+Reduce+Streams+DSL+overloads+and+allow+easier+ >> >>>>>> use+of+custom+storage+engines >> >>>>>> >> >>>>>> Thanks, >> >>>>>> Damian >> >>>>>> >> >>>>>> On Thu, 27 Jul 2017 at 13:09 Damian Guy <damian....@gmail.com> >> >>> wrote: >> >>>>>> >> >>>>>>> Hi, >> >>>>>>> >> >>>>>>> I've put together a KIP to make some changes to the KafkaStreams >> >>> DSL >> >>>>> that >> >>>>>>> will hopefully allow us to: >> >>>>>>> 1) reduce the explosion of overloads >> >>>>>>> 2) add new features without having to continue adding more >> >>> overloads >> >>>>>>> 3) provide simpler ways for people to use custom storage engines >> >>> and >> >>>>> wrap >> >>>>>>> them with logging, caching etc if desired >> >>>>>>> 4) enable per-operator caching rather than global caching without >> >>>>> having >> >>>>>>> to resort to supplying a StateStoreSupplier when you just want to >> >>>> turn >> >>>>>>> caching off. >> >>>>>>> >> >>>>>>> The KIP is here: >> >>>>>>> https://cwiki.apache.org/confluence/pages/viewpage. >> >>>>>> action?pageId=73631309 >> >>>>>>> >> >>>>>>> Thanks, >> >>>>>>> Damian >> >>>>>>> >> >>>>>> >> >>>>> >> >>>>> >> >>>>> >> >>>>> -- >> >>>>> -- Guozhang >> >>>>> >> >>>> >> >>> >> >>> >> >>> >> >>> -- >> >>> -- Guozhang >> >>> >> >> >> > >> >>
