kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeyhun Karimov <je.kari...@gmail.com>
Subject Re: [DISCUSS]: KIP-149: Enabling key access in ValueTransformer, ValueMapper, and ValueJoiner
Date Tue, 04 Jul 2017 21:31:39 GMT
Hi Matthias,

Sorry for long delay. Thanks for the comment. Good to know that the
specified issue is not worth to break the backwards-compatibility.
I fixed the KIP.

Cheers,
Jeyhun

On Fri, Jun 30, 2017 at 1:38 AM Matthias J. Sax <matthias@confluent.io>
wrote:

> Hi Jeyhun,
>
> thanks for starting the VOTE thread. I did make one more pass over the
> KIP before casting my vote and I saw that the KIP still contains
> backward incompatible change introducing `ValueTransformerCommon`.
>
> I think, that for this case, it is not worth breaking compatibility. We
> should have two independent interface and duplicate init() and close()
> (note, with KIP-138 that got merged already, we don't need `punctuate()`
> for ValueTransformerWithKey)
>
>
> -Matthias
>
> On 6/14/17 3:03 PM, Matthias J. Sax wrote:
> > I have no strong opinion, but it seems that at least InitializerWithKey
> > with be helpful if you want to have different start values for different
> > keys (even if I cannot come up with a use case example why one wanted to
> > do this...). Otherwise, there is just the "completeness" argument, that
> > is not too strong either.
> >
> >
> > -Matthias
> >
> > On 6/14/17 2:03 PM, Guozhang Wang wrote:
> >> I'm not particularly concerning that we should NEVER break
> compatibility;
> >> in fact if we think that is worthwhile (i.e. small impact with large
> >> benefit) I think we can break compatibility as long as we have not
> removed
> >> the compatibility annotations from Streams. All I was saying is that if
> we
> >> decided to go this way we need to make sure this is mentioned in the
> >> upgrade guidance.
> >>
> >> Regarding the scope I'm still trying to solicit opinions regarding
> >> ReducerWithKey and InitializerWithKey; to me they are not necessarily
> to be
> >> included.
> >>
> >>
> >> Guozhang
> >>
> >>
> >> On Wed, Jun 14, 2017 at 5:22 AM, Jeyhun Karimov <je.karimov@gmail.com>
> >> wrote:
> >>
> >>> Hi,
> >>>
> >>> I introduced ValueTransformerCommon just to combine common methods of
> >>> ValueTransformer and ValueTransformerWithKey and avoid copy-paste.
> >>> I am aware of this issue and I agree that this needs users to compile
> the
> >>> code and therefore is not backwards compatible. When I saw this issue,
> I
> >>> thought the degree of incompatibility is small (the public APIs are the
> >>> same, users just need to recompile their code), so we can trade more
> >>> maintainable code in this case. I have to comments/solutions:
> >>>
> >>> 1. Basically we can remove ValueTransformerCommon class and return
> >>> ValueTransformer to its original form, which means there will be no
> issues
> >>> with backwards-compatibility. We just copy and past the methods inside
> >>> ValueTransformerCommon to ValueTransformerWithKey and maybe in future
> >>> releases, we can introduce ValueTransformerCommon.
> >>>
> >>> 2.  I have some doubts about Matthias's proposal.
> >>> If we extent withKey interface from original one   as you mentioned in
> >>> previous email, then we have to deal with
> >>>  ValueTransformer.transform(V value) method. As a result, inside
> withKey
> >>> interface we will have two transforms. Even if we make it abstract
> class,
> >>> user still have an access to play with both transform() methods. I
> think
> >>> this should not be allowed and seems "hacky" to me. Actually this was
> one
> >>> reason why I created withKey classes independently from original
> >>> interfaces.
> >>>
> >>> Of course, you can correct me if I am wrong.
> >>>
> >>>
> >>> Cheers,
> >>> Jeyhun
> >>>
> >>>
> >>>
> >>> On Tue, Jun 13, 2017 at 7:42 PM Matthias J. Sax <matthias@confluent.io
> >
> >>> wrote:
> >>>
> >>>> I agree with Guozhang's second point. This change does not seem
> backward
> >>>> compatible.
> >>>>
> >>>> As we don't have to support lambdas, it might be the easiest thing to
> >>>> just extend the current interface:
> >>>>
> >>>>> public interface ValueTransformerWithKey<K, V, VR> extends
> >>>> ValueTransformer<VR>
> >>>>
> >>>> When plugging the topology together, we can check if we get the
> >>>> `withKey` variant and use a corresponding runtime class for execution,
> >>>> so we get only a single time check. Thus, for the `withKey` variant,
> the
> >>>> will be a `transfrom(V value)` method, but we will never call it.
> >>>>
> >>>> Maybe we could make `ValueTransformerWithKey` an abstract class with a
> >>>> `final` no-op implemenation of `transform(V value)` ?
> >>>>
> >>>>
> >>>> -Matthias
> >>>>
> >>>>
> >>>> On 6/6/17 4:58 PM, Guozhang Wang wrote:
> >>>>> Jeyhun, Matthias:
> >>>>>
> >>>>> Thanks for the explanation, I overlooked the repartition argument
> >>>>> previously.
> >>>>>
> >>>>> 1) Based on that argument I'm convinced of having ValueMapperWithKey
> /
> >>>>> ValueJoinerWithKey / ValueTransformerWithKey; though I'm still not
> >>>>> convinced with ReducerWithKey and InitializerWithKey since for the
> >>> former
> >>>>> it can be covered with `aggregate` completely and with latter I have
> >>> seen
> >>>>> little use cases with it.
> >>>>>
> >>>>> 2) Another comment is on public interface ValueTransformer<V, VR>
> >>> extends
> >>>>> ValueTransformerCommon<VR>:
> >>>>>
> >>>>> I think changing the interface to extend from a new interface is not
> >>>> binary
> >>>>> compatible though source compatible, i.e. users still need to
> recompile
> >>>>> their code though no need to make code changes. We may need to
> mention
> >>>> that
> >>>>> in the upgrade path if we want to keep it that way.
> >>>>>
> >>>>> Guozhang
> >>>>>
> >>>>> On Mon, Jun 5, 2017 at 2:28 PM, Jeyhun Karimov <je.karimov@gmail.com
> >
> >>>> wrote:
> >>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>>
> >>>>>> Sorry for late reply. Just to make everybody in sync, the current
> >>>> version
> >>>>>> of KIP supports lambdas. "withKey" (ValueMapperWithKey) interfaces
> are
> >>>>>> independent, meaning they do not extend from "withoutKey"
> >>> (ValueMapper)
> >>>>>> interfaces.
> >>>>>>
> >>>>>>
> >>>>>> I agree with Guozhang, and I am personally a bit reluctant to
> increase
> >>>>>> overloaded methods in public APIs but it seems this is only way to
> >>> solve
> >>>>>> all related jira issues.
> >>>>>> However, the most overloaded methods will be with ValueJoiner type,
> >>>> which
> >>>>>> will be with ValueJoinerWithKey with new overloaded methods. Other
> >>>>>> interfaces require mostly 1 extra overload.
> >>>>>>
> >>>>>>
> >>>>>>>> I would suggest not doing it if user pop it up, but rather
> >>> suggesting
> >>>>>> them
> >>>>>>>> to use `map`
> >>>>>>
> >>>>>> I agree with Matthias as the core idea of this KIP was to collect
> all
> >>>>>> related jira issues and propose one-shot solution for all.
> Afterwards,
> >>>> we
> >>>>>> broke its scope into 2 KIPs (149 and 159).
> >>>>>>
> >>>>>> Cheers,
> >>>>>> Jeyhun
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Mon, Jun 5, 2017 at 7:55 AM Matthias J. Sax <
> matthias@confluent.io
> >>>>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> I guess I missunderstood you. Your are right that overloading the
> >>>> method
> >>>>>>> would also work. As both interfaces will be independent from each
> >>>> other,
> >>>>>>> there should be no problem.
> >>>>>>>
> >>>>>>> The initial proposal was to use
> >>>>>>>
> >>>>>>>> interface ValueMapperWithKey extends ValueMapper
> >>>>>>>
> >>>>>>> and this would break Lambda. We totally missed, that we don't need
> >>> new
> >>>>>>> methods but only only overlaods. Great you point this out! I was
> not
> >>>>>>> quite happy with the newly added methods.
> >>>>>>>
> >>>>>>>>> I would suggest not doing it if user pop it up, but rather
> >>> suggesting
> >>>>>>> them
> >>>>>>>>> to use `map`
> >>>>>>>
> >>>>>>> But this might introduce unnecessary re-partitioning for downstream
> >>>>>>> operators. I don't thinks that a good suggestion. But if we only
> add
> >>>> new
> >>>>>>> overloads for mapValue(), flatMapValue() etc, I don't see a big
> issue
> >>>>>>> with "overstaffing" the API (what is btw a very valid concern).
> >>>>>>>
> >>>>>>>
> >>>>>>> -Matthias
> >>>>>>>
> >>>>>>>
> >>>>>>> On 6/4/17 10:37 PM, Guozhang Wang wrote:
> >>>>>>>> On Sun, Jun 4, 2017 at 8:41 PM, Matthias J. Sax <
> >>>> matthias@confluent.io
> >>>>>>>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> We started with this proposal but it does not allow for Lambdas
> (in
> >>>>>> case
> >>>>>>>>> you want key access). Do you think preserving Lambdas is not
> >>>> important
> >>>>>>>>> enough for this case -- I agree that there is some price to be
> paid
> >>>> to
> >>>>>>>>> keep Lambdas.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Not sure if I understand this comment.. My main point is not on
> >>>>>> changing
> >>>>>>>> the proposal but just reduce it scope to `ValueJoinerWithKey`
> only;
> >>>> and
> >>>>>>>> even if we want to keep them all, I'd suggest we just implement
> the
> >>>>>> added
> >>>>>>>> APIs by using the `KeyValueMapper` for `ValueMapperWithKeys`, etc,
> >>>>>> which
> >>>>>>>> seems simpler to me. Does that affect lambda expression usage?
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> About API consistency: I can't remember a concrete user request
> to
> >>>>>> have
> >>>>>>>>> key access in all operators. But I am pretty sure, if we only add
> >>> it
> >>>>>> for
> >>>>>>>>> some, this request will pop up quickly. IMHO, it's better to do
> it
> >>>> all
> >>>>>>>>> in one shot. (But I am not strict about it if most people think
> we
> >>>>>>>>> should limit it to what was requested by users.)
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>> I would suggest not doing it if user pop it up, but rather
> >>> suggesting
> >>>>>>> them
> >>>>>>>> to use `map` etc directly but set the key unchanged rather than
> >>>>>>> providing a
> >>>>>>>> new operator for it. To me some syntax sugars like this seems of
> >>> less
> >>>>>>>> valuable than others (like print / writeAsText / foreach that are
> >>> all
> >>>>>>>> depending on peek), and keeping adding them will just make the DSL
> >>> too
> >>>>>>>> “overstaffed”.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> -Matthias
> >>>>>>>>>
> >>>>>>>>> On 6/4/17 6:23 PM, Guozhang Wang wrote:
> >>>>>>>>>> With KIP-159, the added "valueMapperWithKey" and
> >>>>>>>>> "valueTransformerWithKey"
> >>>>>>>>>> along seem to be just adding a few syntax-sugars? E.g. we can
> >>> simply
> >>>>>>>>>> implement:
> >>>>>>>>>>
> >>>>>>>>>> mapValues(ValueMapperWithKey mapperWithKey)
> >>>>>>>>>>
> >>>>>>>>>> as
> >>>>>>>>>>
> >>>>>>>>>> map((k, v) -> (k, mapperWithKey(k, v))
> >>>>>>>>>>
> >>>>>>>>>> ----------------------
> >>>>>>>>>>
> >>>>>>>>>> I'm not sure how many users would really need such syntax
> sugars,
> >>>> and
> >>>>>>>>> even
> >>>>>>>>>> they do, we can support them easily as the above
> implementations;
> >>>>>>>>>>
> >>>>>>>>>> Similarly for "ReducerWithKey", it can be implemented as
> >>>>>> `Aggregator<K,
> >>>>>>>>> V,
> >>>>>>>>>> V>`, and people who needs key access can just use `aggregate`
> >>>>>> directly.
> >>>>>>>>>>
> >>>>>>>>>> The function which I think is really of valuable is
> >>>>>>> `ValueJoinerWithKey`,
> >>>>>>>>>> and that seems to be the original request from users while
> others
> >>>> are
> >>>>>>>>>> coming from "API consistency" concerns. Personally I'd prefer
> only
> >>>>>> keep
> >>>>>>>>> the
> >>>>>>>>>> last one (`InitializerWithKey` might also have some value, but I
> >>>> have
> >>>>>>> not
> >>>>>>>>>> seen people widely requesting it in their DSL usage yet; if
> there
> >>> is
> >>>>>> a
> >>>>>>>>>> common request we can keep it in this KIP as well). WDYT?
> >>>>>>>>>>
> >>>>>>>>>> Guozhang
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On Sun, May 28, 2017 at 10:16 AM, Jeyhun Karimov <
> >>>>>> je.karimov@gmail.com
> >>>>>>>>
> >>>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> Thanks for clarification Matthias, now everything is clear.
> >>>>>>>>>>>
> >>>>>>>>>>> On Sun, May 28, 2017 at 6:21 PM Matthias J. Sax <
> >>>>>>> matthias@confluent.io>
> >>>>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> I don't think we can drop ValueTransformerSupplier. If you
> don't
> >>>>>> have
> >>>>>>>>> an
> >>>>>>>>>>>> supplier, you only get a single instance of your function. But
> >>> for
> >>>>>> a
> >>>>>>>>>>>> stateful transformation, we need multiple instances (one for
> >>> each
> >>>>>>> task)
> >>>>>>>>>>>> of ValueTransformer.
> >>>>>>>>>>>>
> >>>>>>>>>>>> We don't need suppliers for functions like "ValueMapper" etc
> >>>>>> because
> >>>>>>>>>>>> those are stateless and thus, we can reuse a single instance
> >>> over
> >>>>>>>>>>>> multiple tasks -- but we can't do this for ValueTransformer
> (and
> >>>>>>>>>>> similar).
> >>>>>>>>>>>>
> >>>>>>>>>>>> Btw: This reminds me about KIP-159: with regard to the
> >>>> RichFunction
> >>>>>>> we
> >>>>>>>>>>>> might need a supplier pattern, too. (I'll comment on the other
> >>>>>>> thread,
> >>>>>>>>>>>> too.)
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> -Matthias
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 5/28/17 5:45 AM, Jeyhun Karimov wrote:
> >>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I updated KIP.
> >>>>>>>>>>>>> Just to avoid misunderstanding, I meant deprecating
> >>>>>>>>>>>> ValueTransformerSupplier
> >>>>>>>>>>>>> and I am ok with ValueTransformer.
> >>>>>>>>>>>>> So instead of using ValueTransformerSupplier can't we
> directly
> >>>> use
> >>>>>>>>>>>>> ValueTransformer
> >>>>>>>>>>>>> or ValueTransformerWithKey?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Btw, in current design all features of ValueTransformer will
> be
> >>>>>>>>>>> available
> >>>>>>>>>>>>> in  ValueTransformerWithKey interface.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>> Jeyhun
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Sun, May 28, 2017 at 6:15 AM Matthias J. Sax <
> >>>>>>>>> matthias@confluent.io
> >>>>>>>>>>>>
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> Thanks Jeyhun.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> About ValueTransformer: I don't think we can remove it.
> Note,
> >>>>>>>>>>>>>> ValueTransformer allows to attach a state and also allows to
> >>>>>>> register
> >>>>>>>>>>>>>> punctuations. Both those features will not be available via
> >>>>>>> withKey()
> >>>>>>>>>>>>>> interfaces.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> -Matthias
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On 5/27/17 1:25 PM, Jeyhun Karimov wrote:
> >>>>>>>>>>>>>>> Hi Matthias,
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Thanks for your comments.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I tested the deep copy approach. It has significant
> overhead.
> >>>>>>>>>>>> Especially
> >>>>>>>>>>>>>>> for "light" and stateless operators it slows down the
> >>> topology
> >>>>>>>>>>>>>>> significantly (> 20% ). I think "warning"  users about
> >>>>>>> not-changing
> >>>>>>>>>>> the
> >>>>>>>>>>>>>> key
> >>>>>>>>>>>>>>> is better warning them about possible performance loss.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> About the interfaces, additionally I considered adding
> >>>>>>>>>>>>>> InitializerWithKey,
> >>>>>>>>>>>>>>> AggregatorWithKey and ValueTransformerWithKey. I think they
> >>> are
> >>>>>>>>>>>> included
> >>>>>>>>>>>>>> in
> >>>>>>>>>>>>>>> PR but not in KIP. I will also include them in KIP, sorry
> my
> >>>>>> bad.
> >>>>>>>>>>>>>>> Including ReducerWithKey definitely makes sense. Thanks.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> One thing I want to mention is that, maybe we should
> >>> deprecate
> >>>>>>>>>>> methods
> >>>>>>>>>>>>>> with
> >>>>>>>>>>>>>>> argument type ValueTransformerSupplier
> >>>>>>>>> (KStream.transformValues(...))
> >>>>>>>>>>>> and
> >>>>>>>>>>>>>>> and as a whole the ValueTransformerSupplier interface.
> >>>>>>>>>>>>>>> We can use ValueTransformer/ValueTransformerWithKey type
> >>>>>> instead
> >>>>>>>>>>>> without
> >>>>>>>>>>>>>>> additional supplier layer.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>>>> Jeyhun
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On Thu, May 25, 2017 at 1:07 AM Matthias J. Sax <
> >>>>>>>>>>> matthias@confluent.io
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> One more question:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Should we add any of
> >>>>>>>>>>>>>>>>  - InitizialierWithKey
> >>>>>>>>>>>>>>>>  - ReducerWithKey
> >>>>>>>>>>>>>>>>  - ValueTransformerWithKey
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> To get consistent/complete API, it might be a good idea.
> Any
> >>>>>>>>>>> thoughts?
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> -Matthias
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> On 5/24/17 3:47 PM, Matthias J. Sax wrote:
> >>>>>>>>>>>>>>>>> Jeyhun,
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> I was just wondering if you did look into the
> key-deep-copy
> >>>>>> idea
> >>>>>>>>> we
> >>>>>>>>>>>>>>>>> discussed. I am curious to see what the impact might be.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> -Matthias
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> On 5/20/17 2:03 AM, Jeyhun Karimov wrote:
> >>>>>>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Thanks for your comments. I rethink about including rich
> >>>>>>>>> functions
> >>>>>>>>>>>>>> into
> >>>>>>>>>>>>>>>>>> this KIP.
> >>>>>>>>>>>>>>>>>> I think once we include rich functions in this KIP and
> >>> then
> >>>>>> fix
> >>>>>>>>>>>>>>>>>> ProcessorContext in another KIP and incorporate with
> >>>> existing
> >>>>>>>>> rich
> >>>>>>>>>>>>>>>>>> functions, the code will not be backwards compatible.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> I see Damian's and your point more clearly now.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Conclusion: we include only withKey interfaces in this
> KIP
> >>>> (I
> >>>>>>>>>>>> updated
> >>>>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>>> KIP), I will try also initiate another KIP for rich
> >>>>>> functions.
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>>>>>>> Jeyhun
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> On Fri, May 19, 2017 at 10:50 PM Matthias J. Sax <
> >>>>>>>>>>>>>> matthias@confluent.io
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> With the current KIP, using ValueMapper and
> >>>>>> ValueMapperWithKey
> >>>>>>>>>>>>>>>>>>> interfaces, RichFunction seems to be an independent
> >>> add-on.
> >>>>>> To
> >>>>>>>>>>> fix
> >>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>>>> original issue to allow key access, RichFunctions are
> not
> >>>>>>>>>>> required
> >>>>>>>>>>>>>>>> IMHO.
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> I initially put the RichFunction idea on the table,
> >>> because
> >>>>>> I
> >>>>>>>>> was
> >>>>>>>>>>>>>>>> hoping
> >>>>>>>>>>>>>>>>>>> to get a uniform API. And I think, is was good to
> >>> consider
> >>>>>>> them
> >>>>>>>>>>> --
> >>>>>>>>>>>>>>>>>>> however, the discussion showed that they are not
> >>> necessary
> >>>>>> for
> >>>>>>>>>>> key
> >>>>>>>>>>>>>>>>>>> access. Thus, it seems to be better to handle
> >>> RichFunctions
> >>>>>> in
> >>>>>>>>> an
> >>>>>>>>>>>> own
> >>>>>>>>>>>>>>>>>>> KIP. The ProcessorContext/RecordContext issues seems to
> >>> be
> >>>> a
> >>>>>>>>> main
> >>>>>>>>>>>>>>>>>>> challenge for this. And introducing RichFunctions with
> >>>>>>>>>>>> parameter-less
> >>>>>>>>>>>>>>>>>>> init() method, seem not to help too much. We would get
> an
> >>>>>>>>>>>>>>>> "intermediate"
> >>>>>>>>>>>>>>>>>>> API that we want to change anyway later on...
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> As you put already much effort into RichFunction, feel
> >>> free
> >>>>>> do
> >>>>>>>>>>> push
> >>>>>>>>>>>>>>>> this
> >>>>>>>>>>>>>>>>>>> further and start a new KIP (we can do this even in
> >>>>>> parallel)
> >>>>>>> --
> >>>>>>>>>>> we
> >>>>>>>>>>>>>>>>>>> don't want to slow you down :) But it make the
> discussion
> >>>>>> and
> >>>>>>>>>>> code
> >>>>>>>>>>>>>>>>>>> review easier, if we separate both IMHO.
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> -Matthias
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> On 5/19/17 2:25 AM, Jeyhun Karimov wrote:
> >>>>>>>>>>>>>>>>>>>> Hi Damian,
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> Thanks for your comments. I think providing to users
> >>>>>>>>> *interface*
> >>>>>>>>>>>>>>>> rather
> >>>>>>>>>>>>>>>>>>>> than *abstract class* should be preferred (Matthias
> also
> >>>>>>> raised
> >>>>>>>>>>>> this
> >>>>>>>>>>>>>>>>>>> issue
> >>>>>>>>>>>>>>>>>>>> ), anyway I changed the corresponding parts of KIP.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> Regarding with passing additional contextual
> >>> information,
> >>>> I
> >>>>>>>>>>> think
> >>>>>>>>>>>> it
> >>>>>>>>>>>>>>>> is a
> >>>>>>>>>>>>>>>>>>>> tradeoff,
> >>>>>>>>>>>>>>>>>>>> 1) first, we fix the context parameter for *init()
> >>> *method
> >>>>>> in
> >>>>>>>>>>>>>> another
> >>>>>>>>>>>>>>>> PR
> >>>>>>>>>>>>>>>>>>>> and solve Rich functions afterwards
> >>>>>>>>>>>>>>>>>>>> 2) first, we fix the requested issues on jira ([1-3])
> >>> with
> >>>>>>>>>>>> providing
> >>>>>>>>>>>>>>>>>>>> (not-complete) Rich functions and integrate the
> context
> >>>>>>>>>>> parameters
> >>>>>>>>>>>>>> to
> >>>>>>>>>>>>>>>>>>> this
> >>>>>>>>>>>>>>>>>>>> afterwards (like improvement)
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> To me, the second approach seems more incremental.
> >>> However
> >>>>>>> you
> >>>>>>>>>>> are
> >>>>>>>>>>>>>>>> right,
> >>>>>>>>>>>>>>>>>>>> the names might confuse the users.
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/KAFKA-4218
> >>>>>>>>>>>>>>>>>>>> [2] https://issues.apache.org/jira/browse/KAFKA-4726
> >>>>>>>>>>>>>>>>>>>> [3] https://issues.apache.org/jira/browse/KAFKA-3745
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>>>>>>>>> Jeyhun
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> On Fri, May 19, 2017 at 10:42 AM Damian Guy <
> >>>>>>>>>>> damian.guy@gmail.com
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> I see you've removed the `ProcessorContext` from the
> >>>>>>>>>>> RichFunction
> >>>>>>>>>>>>>>>> which
> >>>>>>>>>>>>>>>>>>> is
> >>>>>>>>>>>>>>>>>>>>> good, but why is it a `RichFunction`? I'd have
> expected
> >>>> it
> >>>>>>> to
> >>>>>>>>>>>> pass
> >>>>>>>>>>>>>>>> some
> >>>>>>>>>>>>>>>>>>>>> additional contextual information, i.e., the
> >>>>>> `RecordContext`
> >>>>>>>>>>> that
> >>>>>>>>>>>>>>>>>>> contains
> >>>>>>>>>>>>>>>>>>>>> just the topic, partition, timestamp, offset.  I'm ok
> >>>> with
> >>>>>>> it
> >>>>>>>>>>> not
> >>>>>>>>>>>>>>>>>>> passing
> >>>>>>>>>>>>>>>>>>>>> this contextual information, but is the naming
> >>> incorrect?
> >>>>>>> I'm
> >>>>>>>>>>> not
> >>>>>>>>>>>>>>>> sure,
> >>>>>>>>>>>>>>>>>>>>> tbh. I'm wondering if we should drop `RichFunctions`
> >>>> until
> >>>>>>> we
> >>>>>>>>>>> can
> >>>>>>>>>>>>>> do
> >>>>>>>>>>>>>>>> it
> >>>>>>>>>>>>>>>>>>>>> properly with the correct context?
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> Also, i don't like the abstract classes:
> >>> RichValueMapper,
> >>>>>>>>>>>>>>>>>>> RichValueJoiner,
> >>>>>>>>>>>>>>>>>>>>> RichInitializer etc. Why can't they not just be
> >>>>>> interfaces?
> >>>>>>>>>>>>>>>> Generally we
> >>>>>>>>>>>>>>>>>>>>> should provide users with Intefaces to code against,
> >>> not
> >>>>>>>>>>> classes.
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>>>>>>>> Damian
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> On Fri, 19 May 2017 at 00:50 Jeyhun Karimov <
> >>>>>>>>>>>> je.karimov@gmail.com>
> >>>>>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> Thanks. I initiated the PR as well, to get a better
> >>>>>>> overview.
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> The only reason that I used abstract class instead
> of
> >>>>>>>>>>> interface
> >>>>>>>>>>>>>> for
> >>>>>>>>>>>>>>>>>>> Rich
> >>>>>>>>>>>>>>>>>>>>>> functions is that in future if we will have some
> >>>>>>>>>>>>>>>> AbstractRichFunction
> >>>>>>>>>>>>>>>>>>>>>> abstract classes,
> >>>>>>>>>>>>>>>>>>>>>> we can easily extend:
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> public abstract class RichValueMapper<K, V, VR>
> >>>>>> implements
> >>>>>>>>>>>>>>>>>>>>>> ValueMapperWithKey<K, V, VR>, RichFunction *extends
> >>>>>>>>>>>>>>>>>>>>> AbstractRichFunction*{
> >>>>>>>>>>>>>>>>>>>>>> }
> >>>>>>>>>>>>>>>>>>>>>>  With interfaces we are only limited to interfaces
> for
> >>>>>>>>>>>>>> inheritance.
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> However, I think we can easily change it (from
> >>> abstract
> >>>>>>> class
> >>>>>>>>>>> ->
> >>>>>>>>>>>>>>>>>>>>> interface)
> >>>>>>>>>>>>>>>>>>>>>> if you think interface is a better fit.
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>>>>>>>>>>> Jeyhun
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> On Fri, May 19, 2017 at 1:00 AM Matthias J. Sax <
> >>>>>>>>>>>>>>>> matthias@confluent.io
> >>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> Thanks for the update and explanations. The KIP is
> >>>> quite
> >>>>>>>>>>>> improved
> >>>>>>>>>>>>>>>> now
> >>>>>>>>>>>>>>>>>>>>> --
> >>>>>>>>>>>>>>>>>>>>>>> great job!
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> One more question: Why are RichValueMapper etc
> >>> abstract
> >>>>>>>>>>> classes
> >>>>>>>>>>>>>> and
> >>>>>>>>>>>>>>>>>>> not
> >>>>>>>>>>>>>>>>>>>>>>> interfaces?
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> Overall, I like the KIP a lot!
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> -Matthias
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> On 5/16/17 7:03 AM, Jeyhun Karimov wrote:
> >>>>>>>>>>>>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Thanks for your comments.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> I think supporting Lambdas for `withKey` and
> >>>>>>>>>>>>>>>> `AbstractRichFunction`
> >>>>>>>>>>>>>>>>>>>>>>>>> don't go together, as Lambdas are only supported
> >>> for
> >>>>>>>>>>>> interfaces
> >>>>>>>>>>>>>>>>>>>>> AFAIK.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Maybe I misunderstood your comment.
> >>>>>>>>>>>>>>>>>>>>>>>> *withKey* and and *withOnlyValue* are interfaces.
> So
> >>>>>> they
> >>>>>>>>>>>> don't
> >>>>>>>>>>>>>>>> have
> >>>>>>>>>>>>>>>>>>>>>>> direct
> >>>>>>>>>>>>>>>>>>>>>>>> relation with *AbstractRichFunction*.
> >>>>>>>>>>>>>>>>>>>>>>>> *withKey* and and *withOnlyValue* interfaces have
> >>> only
> >>>>>>> one
> >>>>>>>>>>>>>>>> method ,
> >>>>>>>>>>>>>>>>>>>>> so
> >>>>>>>>>>>>>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>>>>>>>>> can use lambdas.
> >>>>>>>>>>>>>>>>>>>>>>>> Where does the *AbstractRichFunction* comes to
> play?
> >>>>>>> Inside
> >>>>>>>>>>>> Rich
> >>>>>>>>>>>>>>>>>>>>>>> functions.
> >>>>>>>>>>>>>>>>>>>>>>>> And we use Rich functions in 2 places:
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> 1. User doesn't use rich functions. Just regular
> >>>>>>> *withKey*
> >>>>>>>>>>> and
> >>>>>>>>>>>>>> and
> >>>>>>>>>>>>>>>>>>>>>>>> *withOnlyValue* interfaces(both support lambdas) .
> >>> We
> >>>>>> get
> >>>>>>>>>>>> those
> >>>>>>>>>>>>>>>>>>>>>>> interfaces
> >>>>>>>>>>>>>>>>>>>>>>>> and wrap into Rich function while building the
> >>>>>> topology,
> >>>>>>>>> and
> >>>>>>>>>>>>>> send
> >>>>>>>>>>>>>>>> to
> >>>>>>>>>>>>>>>>>>>>>>>> Processor.
> >>>>>>>>>>>>>>>>>>>>>>>> 2. User does use rich functions (Rich functions
> >>>>>> implement
> >>>>>>>>>>>>>>>> *withKey*
> >>>>>>>>>>>>>>>>>>>>>>>> interface). As a result no lamdas here by
> >>> definition.
> >>>>>> In
> >>>>>>>>>>> this
> >>>>>>>>>>>>>>>> case,
> >>>>>>>>>>>>>>>>>>>>>> while
> >>>>>>>>>>>>>>>>>>>>>>>> building the topology we do a type check if the
> >>> object
> >>>>>>> type
> >>>>>>>>>>> is
> >>>>>>>>>>>>>>>>>>>>>>>> *withKey* or *RichFunction*.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> So *AbstractRichFunction* is just syntactic sugar
> >>> for
> >>>>>>> Rich
> >>>>>>>>>>>>>>>> functions
> >>>>>>>>>>>>>>>>>>>>>> and
> >>>>>>>>>>>>>>>>>>>>>>>> does not affect using lambdas.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Thus, if we want to support Lambdas for `withKey`,
> >>> we
> >>>>>>> need
> >>>>>>>>>>> to
> >>>>>>>>>>>>>>>> have a
> >>>>>>>>>>>>>>>>>>>>>>>>> interface approach like this
> >>>>>>>>>>>>>>>>>>>>>>>>>   - RichFunction -> only adding init() and
> close()
> >>>>>>>>>>>>>>>>>>>>>>>>>   - ValueMapper
> >>>>>>>>>>>>>>>>>>>>>>>>>   - ValueMapperWithKey
> >>>>>>>>>>>>>>>>>>>>>>>>>   - RichValueMapper extends ValueMapperWithKey,
> >>>>>>>>>>> RichFunction
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> As I said above, currently we support lambdas for
> >>>>>>> *withKey*
> >>>>>>>>>>>>>>>>>>>>> interfaces
> >>>>>>>>>>>>>>>>>>>>>> as
> >>>>>>>>>>>>>>>>>>>>>>>> well.  However, I agree with your idea and I will
> >>>>>> remove
> >>>>>>>>> the
> >>>>>>>>>>>>>>>>>>>>>>>> AbstractRichFunction from the design.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> As an alternative, we could argue, that it is
> >>>>>> sufficient
> >>>>>>> to
> >>>>>>>>>>>>>>>> support
> >>>>>>>>>>>>>>>>>>>>>>>>> Lambdas for the "plain" API only, but not for any
> >>>>>>>>> "extended
> >>>>>>>>>>>>>> API".
> >>>>>>>>>>>>>>>>>>>>> For
> >>>>>>>>>>>>>>>>>>>>>>>>> this, RichFunction could add key+init+close and
> >>>>>>>>>>>>>>>> AbstractRichFunction
> >>>>>>>>>>>>>>>>>>>>>>>>> would allow to only care about getting the key.
> >>>>>>>>>>>>>>>>>>>>>>>>> Not sure, which one is better. I don't like the
> >>> idea
> >>>>>> of
> >>>>>>>>>>> more
> >>>>>>>>>>>>>>>>>>>>>> overloaded
> >>>>>>>>>>>>>>>>>>>>>>>>> methods to get Lambdas for `withKey` interfaces
> too
> >>>>>> much
> >>>>>>>>>>>>>> because
> >>>>>>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>>>>>>> have
> >>>>>>>>>>>>>>>>>>>>>>>>> already so many overlaods. On the other hand, I
> do
> >>>> see
> >>>>>>>>>>> value
> >>>>>>>>>>>> in
> >>>>>>>>>>>>>>>>>>>>>>>>> supporting Lambdas for `withKey`.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Just to clarify, with current design we have only
> >>> one
> >>>>>>> extra
> >>>>>>>>>>>>>>>>>>>>> overloaded
> >>>>>>>>>>>>>>>>>>>>>>>> method per *withOnlyValue* interface:  which is
> >>>>>> *withKey*
> >>>>>>>>>>>>>> version
> >>>>>>>>>>>>>>>> of
> >>>>>>>>>>>>>>>>>>>>>>>> particular interface.
> >>>>>>>>>>>>>>>>>>>>>>>> We don't need extra overload for Rich function as
> >>> Rich
> >>>>>>>>>>>> function
> >>>>>>>>>>>>>>>>>>>>>>> implements
> >>>>>>>>>>>>>>>>>>>>>>>> *withKey* interface as a result they have same
> type.
> >>>> We
> >>>>>>>>>>>>>>>> differentiate
> >>>>>>>>>>>>>>>>>>>>>>> them
> >>>>>>>>>>>>>>>>>>>>>>>> while building the topology.
> >>>>>>>>>>>>>>>>>>>>>>>> We supported lambdas for *withKey* APIs because of
> >>> the
> >>>>>>>>>>>> comment:
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> @Jeyhun: I did not put any thought into this, but
> >>> can
> >>>>>> we
> >>>>>>>>>>> have
> >>>>>>>>>>>> a
> >>>>>>>>>>>>>>>>>>>>> design
> >>>>>>>>>>>>>>>>>>>>>>>>> that allows for both? Also, with regard to
> lambdas,
> >>>> it
> >>>>>>>>>>> might
> >>>>>>>>>>>>>> make
> >>>>>>>>>>>>>>>>>>>>>> sense
> >>>>>>>>>>>>>>>>>>>>>>>>> to allow for both `V -> newV` and `(K, V) ->
> newV`
> >>> ?
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> However, I don't think that this complicates the
> >>>>>> overall
> >>>>>>>>>>>> design
> >>>>>>>>>>>>>>>>>>>>>>>> significantly.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Depending on what we want to support, it might
> make
> >>>>>> sense
> >>>>>>>>> to
> >>>>>>>>>>>>>>>>>>>>>>>>> include/exclude RichFunctions from this KIP --
> and
> >>>>>> thus,
> >>>>>>>>>>> this
> >>>>>>>>>>>>>>>> also
> >>>>>>>>>>>>>>>>>>>>>>>>> determines if we should have a "ProcessorContext
> >>> KIP"
> >>>>>>>>>>> before
> >>>>>>>>>>>>>>>> driving
> >>>>>>>>>>>>>>>>>>>>>>>>> this KIP further.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Based on our discussion I think we should keep
> Rich
> >>>>>>>>>>> functions
> >>>>>>>>>>>>>> as I
> >>>>>>>>>>>>>>>>>>>>>> don't
> >>>>>>>>>>>>>>>>>>>>>>>> think that they bring extra layer of overhead to
> >>>>>> library.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Any comments are appreciated.
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>>>>>>>>>>>>> Jeyhun
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> On Tue, May 16, 2017 at 12:10 AM Matthias J. Sax <
> >>>>>>>>>>>>>>>>>>>>>> matthias@confluent.io>
> >>>>>>>>>>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun,
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> thanks for the update.
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> I think supporting Lambdas for `withKey` and
> >>>>>>>>>>>>>>>> `AbstractRichFunction`
> >>>>>>>>>>>>>>>>>>>>>>>>> don't go together, as Lambdas are only supported
> >>> for
> >>>>>>>>>>>> interfaces
> >>>>>>>>>>>>>>>>>>>>> AFAIK.
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> Thus, if we want to support Lambdas for
> `withKey`,
> >>> we
> >>>>>>> need
> >>>>>>>>>>> to
> >>>>>>>>>>>>>>>> have a
> >>>>>>>>>>>>>>>>>>>>>>>>> interface approach like this
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>   - RichFunction -> only adding init() and
> close()
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>   - ValueMapper
> >>>>>>>>>>>>>>>>>>>>>>>>>   - ValueMapperWithKey
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>   - RichValueMapper extends ValueMapperWithKey,
> >>>>>>>>>>> RichFunction
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> For this approach, AbstractRichFunction does not
> >>> make
> >>>>>>>>> sense
> >>>>>>>>>>>>>>>> anymore,
> >>>>>>>>>>>>>>>>>>>>>> as
> >>>>>>>>>>>>>>>>>>>>>>>>> the only purpose of `RichFunction` is to allow
> the
> >>>>>>>>>>>>>>>> implementation of
> >>>>>>>>>>>>>>>>>>>>>>>>> init() and close() -- if you don't want those,
> you
> >>>>>> would
> >>>>>>>>>>>>>>>> implement a
> >>>>>>>>>>>>>>>>>>>>>>>>> different interface (ie, ValueMapperWithKey)
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> As an alternative, we could argue, that it is
> >>>>>> sufficient
> >>>>>>>>> to
> >>>>>>>>>>>>>>>> support
> >>>>>>>>>>>>>>>>>>>>>>>>> Lambdas for the "plain" API only, but not for any
> >>>>>>>>> "extended
> >>>>>>>>>>>>>> API".
> >>>>>>>>>>>>>>>>>>>>> For
> >>>>>>>>>>>>>>>>>>>>>>>>> this, RichFunction could add key+init+close and
> >>>>>>>>>>>>>>>> AbstractRichFunction
> >>>>>>>>>>>>>>>>>>>>>>>>> would allow to only care about getting the key.
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> Not sure, which one is better. I don't like the
> >>> idea
> >>>>>> of
> >>>>>>>>>>> more
> >>>>>>>>>>>>>>>>>>>>>> overloaded
> >>>>>>>>>>>>>>>>>>>>>>>>> methods to get Lambdas for `withKey` interfaces
> too
> >>>>>> much
> >>>>>>>>>>>>>> because
> >>>>>>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>>>>>>> have
> >>>>>>>>>>>>>>>>>>>>>>>>> already so many overlaods. On the other hand, I
> do
> >>>> see
> >>>>>>>>>>> value
> >>>>>>>>>>>> in
> >>>>>>>>>>>>>>>>>>>>>>>>> supporting Lambdas for `withKey`.
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> Depending on what we want to support, it might
> make
> >>>>>>> sense
> >>>>>>>>>>> to
> >>>>>>>>>>>>>>>>>>>>>>>>> include/exclude RichFunctions from this KIP --
> and
> >>>>>> thus,
> >>>>>>>>>>> this
> >>>>>>>>>>>>>>>> also
> >>>>>>>>>>>>>>>>>>>>>>>>> determines if we should have a "ProcessorContext
> >>> KIP"
> >>>>>>>>>>> before
> >>>>>>>>>>>>>>>> driving
> >>>>>>>>>>>>>>>>>>>>>>>>> this KIP further.
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> Thoughts?
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> -Matthias
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> On 5/15/17 11:01 AM, Jeyhun Karimov wrote:
> >>>>>>>>>>>>>>>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Sorry for super late response. Thanks for your
> >>>>>>> comments.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> I am not an expert on Lambdas. Can you
> elaborate a
> >>>>>>> little
> >>>>>>>>>>>>>> bit? I
> >>>>>>>>>>>>>>>>>>>>>> cannot
> >>>>>>>>>>>>>>>>>>>>>>>>>>> follow the explanation in the KIP to see what
> the
> >>>>>>>>> problem
> >>>>>>>>>>>> is.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> - From [1] says "A functional interface is an
> >>>>>> interface
> >>>>>>>>>>> that
> >>>>>>>>>>>>>> has
> >>>>>>>>>>>>>>>>>>>>> just
> >>>>>>>>>>>>>>>>>>>>>>> one
> >>>>>>>>>>>>>>>>>>>>>>>>>> abstract method, and thus represents a single
> >>>>>> function
> >>>>>>>>>>>>>>>> contract".
> >>>>>>>>>>>>>>>>>>>>>>>>>> So basically once we extend some interface from
> >>>>>> another
> >>>>>>>>>>> (in
> >>>>>>>>>>>>>> our
> >>>>>>>>>>>>>>>>>>>>> case,
> >>>>>>>>>>>>>>>>>>>>>>>>>> ValueMapperWithKey from ValueMapper) we cannot
> use
> >>>>>>>>> lambdas
> >>>>>>>>>>>> in
> >>>>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>>>>>>>>>> extended
> >>>>>>>>>>>>>>>>>>>>>>>>>> interface.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Further comments:
> >>>>>>>>>>>>>>>>>>>>>>>>>>>  - The KIP get a little hard to read -- can you
> >>>>>> maybe
> >>>>>>>>>>>>>> reformat
> >>>>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>>>>>>>> wiki
> >>>>>>>>>>>>>>>>>>>>>>>>>>> page a little bit? I think using `CodeBlock`
> >>> would
> >>>>>>> help.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> - I will work on the KIP.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>  - What about KStream-KTable joins? You don't
> have
> >>>>>>>>>>> overlaods
> >>>>>>>>>>>>>>>> added
> >>>>>>>>>>>>>>>>>>>>>> for
> >>>>>>>>>>>>>>>>>>>>>>>>>>> them. Why? (Even if I still hope that we don't
> >>> need
> >>>>>> to
> >>>>>>>>>>> add
> >>>>>>>>>>>>>> any
> >>>>>>>>>>>>>>>> new
> >>>>>>>>>>>>>>>>>>>>>>>>>>> overloads)
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> - Actually there are more than one Processor and
> >>>>>> public
> >>>>>>>>>>> APIs
> >>>>>>>>>>>>>> to
> >>>>>>>>>>>>>>>> be
> >>>>>>>>>>>>>>>>>>>>>>>>>> changed (KStream-KTable
> >>>>>>>>>>>>>>>>>>>>>>>>>> joins is one case). However all of them has
> >>> similar
> >>>>>>>>>>>> structure:
> >>>>>>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>>>>>>>>>> overload
> >>>>>>>>>>>>>>>>>>>>>>>>>> the *method* with  *methodWithKey*,
> >>>>>>>>>>>>>>>>>>>>>>>>>> wrap it into the Rich function, send to
> processor
> >>>> and
> >>>>>>>>>>> inside
> >>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>>>>>>>>>> processor
> >>>>>>>>>>>>>>>>>>>>>>>>>> call *init* and *close* methods of the Rich
> >>>> function.
> >>>>>>>>>>>>>>>>>>>>>>>>>> As I wrote in KIP, I wanted to demonstrate the
> >>>>>> overall
> >>>>>>>>>>> idea
> >>>>>>>>>>>>>> with
> >>>>>>>>>>>>>>>>>>>>> only
> >>>>>>>>>>>>>>>>>>>>>>>>>> *ValueMapper* as the same can be applied to all
> >>>>>>> changes.
> >>>>>>>>>>>>>>>>>>>>>>>>>> Anyway I will update the KIP.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>  - Why do we need `AbstractRichFunction`?
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Instead of overriding the *init(ProcessorContext
> >>> p)*
> >>>>>>> and*
> >>>>>>>>>>>>>>>> close()*
> >>>>>>>>>>>>>>>>>>>>>>>>> methods
> >>>>>>>>>>>>>>>>>>>>>>>>>> in every Rich function with empty body like:
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> @Override
> >>>>>>>>>>>>>>>>>>>>>>>>>> void init(ProcessorContext context) {}
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> @Override
> >>>>>>>>>>>>>>>>>>>>>>>>>> void close () {}
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> I thought that we can override them once in
> >>>>>>>>>>>>>>>> *AbstractRichFunction*
> >>>>>>>>>>>>>>>>>>>>>> and
> >>>>>>>>>>>>>>>>>>>>>>>>>> extent new Rich functions from
> >>>>>> *AbstractRichFunction*.
> >>>>>>>>>>>>>>>>>>>>>>>>>> Basically this can eliminate code copy-paste and
> >>>> ease
> >>>>>>> the
> >>>>>>>>>>>>>>>>>>>>>> maintenance.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>  - What about interfaces Initializer,
> >>> ForeachAction,
> >>>>>>>>>>> Merger,
> >>>>>>>>>>>>>>>>>>>>>> Predicate,
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Reducer? I don't want to say we should/need to
> >>> add
> >>>>>> to
> >>>>>>>>>>> all,
> >>>>>>>>>>>>>> but
> >>>>>>>>>>>>>>>> we
> >>>>>>>>>>>>>>>>>>>>>>> should
> >>>>>>>>>>>>>>>>>>>>>>>>>>> discuss all of them and add where it does make
> >>>> sense
> >>>>>>>>>>> (e.g.,
> >>>>>>>>>>>>>>>>>>>>>>>>>>> RichForachAction does make sense IMHO)
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Definitely agree. As I said, the same technique
> >>>>>> applies
> >>>>>>>>> to
> >>>>>>>>>>>> all
> >>>>>>>>>>>>>>>> this
> >>>>>>>>>>>>>>>>>>>>>>>>>> interfaces and I didn't want to explode the KIP,
> >>>> just
> >>>>>>>>>>> wanted
> >>>>>>>>>>>>>> to
> >>>>>>>>>>>>>>>>>>>>> give
> >>>>>>>>>>>>>>>>>>>>>>> the
> >>>>>>>>>>>>>>>>>>>>>>>>>> overall intuition.
> >>>>>>>>>>>>>>>>>>>>>>>>>> However, I will update the KIP as I said.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Btw: I like the hierarchy `ValueXX` --
> >>>>>> `ValueXXWithKey`
> >>>>>>>>> --
> >>>>>>>>>>>>>>>>>>>>>>> `RichValueXX`
> >>>>>>>>>>>>>>>>>>>>>>>>>>> in general -- but why can't we do all this with
> >>>>>>>>>>> interfaces
> >>>>>>>>>>>>>>>> only?
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Sure we can. However the main intuition is we
> >>> should
> >>>>>>> not
> >>>>>>>>>>>> force
> >>>>>>>>>>>>>>>>>>>>> users
> >>>>>>>>>>>>>>>>>>>>>> to
> >>>>>>>>>>>>>>>>>>>>>>>>>> implement *init(ProcessorContext)* and *close()*
> >>>>>>>>> functions
> >>>>>>>>>>>>>> every
> >>>>>>>>>>>>>>>>>>>>> time
> >>>>>>>>>>>>>>>>>>>>>>>>> they
> >>>>>>>>>>>>>>>>>>>>>>>>>> use Rich functions.
> >>>>>>>>>>>>>>>>>>>>>>>>>> If one needs, she can override the respective
> >>>>>> methods.
> >>>>>>>>>>>>>> However,
> >>>>>>>>>>>>>>>> I
> >>>>>>>>>>>>>>>>>>>>> am
> >>>>>>>>>>>>>>>>>>>>>>> open
> >>>>>>>>>>>>>>>>>>>>>>>>>> for discussion.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> I'd rather not see the use of
> `ProcessorContext`
> >>>>>>> spread
> >>>>>>>>>>> any
> >>>>>>>>>>>>>>>>>>>>> further
> >>>>>>>>>>>>>>>>>>>>>>> than
> >>>>>>>>>>>>>>>>>>>>>>>>>>> it currently is. So maybe we need another KIP
> >>> that
> >>>>>> is
> >>>>>>>>>>> done
> >>>>>>>>>>>>>>>> before
> >>>>>>>>>>>>>>>>>>>>>>> this?
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Otherwise i think the scope of this KIP is
> >>> becoming
> >>>>>>> too
> >>>>>>>>>>>>>> large.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> That is good point. I wanted to make
> >>>>>>>>>>>> *init(ProcessorContext)*
> >>>>>>>>>>>>>>>>>>>>> method
> >>>>>>>>>>>>>>>>>>>>>>>>>> persistent among the library (which use
> >>>>>>> ProcessorContext
> >>>>>>>>>>> as
> >>>>>>>>>>>> an
> >>>>>>>>>>>>>>>>>>>>>> input),
> >>>>>>>>>>>>>>>>>>>>>>>>>> therefore I put *ProcessorContext* as an input.
> >>>>>>>>>>>>>>>>>>>>>>>>>> So the important question is that (as @dguy and
> >>>>>> @mjsax
> >>>>>>>>>>>>>>>> mentioned)
> >>>>>>>>>>>>>>>>>>>>>>> whether
> >>>>>>>>>>>>>>>>>>>>>>>>>> continue this KIP without providing users an
> >>> access
> >>>>>> to
> >>>>>>>>>>>>>>>>>>>>>>> *ProcessorContext*
> >>>>>>>>>>>>>>>>>>>>>>>>>> (change *init (ProcessorContext)* to * init()* )
> >>> or
> >>>>>>>>>>>>>>>>>>>>>>>>>> initiate another KIP before this.
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> [1]
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>> http://cr.openjdk.java.net/~mr/se/8/java-se-8-pfd-spec/java-
> >>>>>>>>>>>>>>>>>>>>>>>>> se-8-jls-pfd-diffs.pdf
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, May 15, 2017 at 7:15 PM, Damian Guy <
> >>>>>>>>>>>>>>>> damian.guy@gmail.com>
> >>>>>>>>>>>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>> I'd rather not see the use of
> `ProcessorContext`
> >>>>>>> spread
> >>>>>>>>>>>> any
> >>>>>>>>>>>>>>>>>>>>> further
> >>>>>>>>>>>>>>>>>>>>>>>>> than
> >>>>>>>>>>>>>>>>>>>>>>>>>>> it currently is. So maybe we need another KIP
> >>> that
> >>>>>> is
> >>>>>>>>>>> done
> >>>>>>>>>>>>>>>> before
> >>>>>>>>>>>>>>>>>>>>>>> this?
> >>>>>>>>>>>>>>>>>>>>>>>>>>> Otherwise i think the scope of this KIP is
> >>> becoming
> >>>>>>> too
> >>>>>>>>>>>>>> large.
> >>>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, 15 May 2017 at 18:06 Matthias J. Sax <
> >>>>>>>>>>>>>>>>>

-- 
-Cheers

Jeyhun

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message