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.kari...@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 <matth...@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.kari...@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 <matth...@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 < > > matth...@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.kari...@gmail.com > > >>>> > > >>>>>> wrote: > > >>>>>> > > >>>>>>> Thanks for clarification Matthias, now everything is clear. > > >>>>>>> > > >>>>>>> On Sun, May 28, 2017 at 6:21 PM Matthias J. Sax < > > >>> matth...@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 < > > >>>>> matth...@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 < > > >>>>>>> matth...@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 < > > >>>>>>>>>> matth...@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....@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.kari...@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 < > > >>>>>>>>>>>> matth...@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 < > > >>>>>>>>>>>>>>>>>> matth...@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....@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 < > > >>>>>>>>>>>>>>>>> matth...@confluent.io > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> I agree that that `ProcessorContext` interface > is > > >> too > > >>>>>>>> broad > > >>>>>>>>>> in > > >>>>>>>>>>>>>>>>>>> general > > >>>>>>>>>>>>>>>>>>>>>>>> -- this is even true for transform/process, and > > >> it's > > >>>>>>> also > > >>>>>>>>>>>>>>>>> reflected > > >>>>>>>>>>>>>>>>>>> in > > >>>>>>>>>>>>>>>>>>>>>>>> the API improvement list we want to do. > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> https://cwiki.apache.org/ > > confluence/display/KAFKA/ > > >>>>>>>>>>>>>>>>>>>>>>> Kafka+Streams+Discussions > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> So I am wondering, if you question the > > >> `RichFunction` > > >>>>>>>>>>>> approach in > > >>>>>>>>>>>>>>>>>>>>>>>> general? Or if you suggest to either extend the > > >> scope > > >>>>> of > > >>>>>>>>>> this > > >>>>>>>>>>>> KIP > > >>>>>>>>>>>>>>>>>> to > > >>>>>>>>>>>>>>>>>>>>>>>> include this---or maybe better, do another KIP > for > > >> it > > >>>>>>> and > > >>>>>>>>>>>> delay > > >>>>>>>>>>>>>>>>>> this > > >>>>>>>>>>>>>>>>>>>>> KIP > > >>>>>>>>>>>>>>>>>>>>>>>> until the other one is done? > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> -Matthias > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> On 5/15/17 2:35 AM, Damian Guy wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>> Thanks for the KIP. > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> I'm not convinced on the `RichFunction` > approach. > > >> Do > > >>>>> we > > >>>>>>>>>>>> really > > >>>>>>>>>>>>>>>>>> want > > >>>>>>>>>>>>>>>>>>> to > > >>>>>>>>>>>>>>>>>>>>>>>> give > > >>>>>>>>>>>>>>>>>>>>>>>>> every DSL method access to the > `ProcessorContext` > > >> ? > > >>> It > > >>>>>>>> has > > >>>>>>>>>> a > > >>>>>>>>>>>>>>>>> bunch > > >>>>>>>>>>>>>>>>>>> of > > >>>>>>>>>>>>>>>>>>>>>>>>> methods on it that seem in-appropriate for some > > of > > >>> the > > >>>>>>>> DSL > > >>>>>>>>>>>>>>>>>> methods, > > >>>>>>>>>>>>>>>>>>>>>>> i.e, > > >>>>>>>>>>>>>>>>>>>>>>>>> `register`, `getStateStore`, `forward`, > > `schedule` > > >>>>> etc. > > >>>>>>>> It > > >>>>>>>>>> is > > >>>>>>>>>>>>>>>>> far > > >>>>>>>>>>>>>>>>>>> too > > >>>>>>>>>>>>>>>>>>>>>>>>> broad. I think it would be better to have a > > >> narrower > > >>>>>>>>>>>> interface > > >>>>>>>>>>>>>>>>>> like > > >>>>>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>> `RecordContext` - remembering it is easier to > > add > > >>>>>>>>>>>>>>>>>>> methods/interfaces > > >>>>>>>>>>>>>>>>>>>>>>>> later > > >>>>>>>>>>>>>>>>>>>>>>>>> than to remove them > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>> On Sat, 13 May 2017 at 22:26 Matthias J. Sax < > > >>>>>>>>>>>>>>>>>> matth...@confluent.io > > >>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun, > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> 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. > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> For updating the KIP title I don't know -- > guess > > >>> it's > > >>>>>>> up > > >>>>>>>>>> to > > >>>>>>>>>>>>>>>>> you. > > >>>>>>>>>>>>>>>>>>>>>>> Maybe a > > >>>>>>>>>>>>>>>>>>>>>>>>>> committer can comment on this? > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> 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. > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> - 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) > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> - Why do we need `AbstractRichFunction`? > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> - 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) > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> Btw: I like the hierarchy `ValueXX` -- > > >>>>>>> `ValueXXWithKey` > > >>>>>>>> -- > > >>>>>>>>>>>>>>>>>>>>>>> `RichValueXX` > > >>>>>>>>>>>>>>>>>>>>>>>>>> in general -- but why can't we do all this > with > > >>>>>>>> interfaces > > >>>>>>>>>>>>>>>>> only? > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> -Matthias > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> On 5/11/17 7:00 AM, Jeyhun Karimov wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for your comments. I think we cannot > > >> extend > > >>>>>>> the > > >>>>>>>>>> two > > >>>>>>>>>>>>>>>>>>>>> interfaces > > >>>>>>>>>>>>>>>>>>>>>>>> if > > >>>>>>>>>>>>>>>>>>>>>>>>>> we > > >>>>>>>>>>>>>>>>>>>>>>>>>>> want to keep lambdas. I updated the KIP [1]. > > >>> Maybe I > > >>>>>>>>>> should > > >>>>>>>>>>>>>>>>>> change > > >>>>>>>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>>>> title, because now we are not limiting the > KIP > > >> to > > >>>>>>> only > > >>>>>>>>>>>>>>>>>>> ValueMapper, > > >>>>>>>>>>>>>>>>>>>>>>>>>>> ValueTransformer and ValueJoiner. > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Please feel free to comment. > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> [1] > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> https://cwiki.apache.org/ > > >>> confluence/display/KAFKA/KIP- > > >>>>>>>>>>>>>>>>>>>>>>> 149%3A+Enabling+key+access+in+ > ValueTransformer%2C+ > > >>>>>>>>>>>>>>>>>>>>>>> ValueMapper%2C+and+ValueJoiner > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Cheers, > > >>>>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> On Tue, May 9, 2017 at 7:36 PM Matthias J. > Sax > > < > > >>>>>>>>>>>>>>>>>>>>>>> matth...@confluent.io> > > >>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> If `ValueMapperWithKey` extends > `ValueMapper` > > >> we > > >>>>>>> don't > > >>>>>>>>>>>> need > > >>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>> new > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> overlaod. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> And yes, we need to do one check -- but this > > >>>>> happens > > >>>>>>>>>> when > > >>>>>>>>>>>>>>>>>>> building > > >>>>>>>>>>>>>>>>>>>>>>> the > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> topology. At runtime (I mean after > > >>>>>>> KafkaStream#start() > > >>>>>>>>>> we > > >>>>>>>>>>>>>>>>> don't > > >>>>>>>>>>>>>>>>>>>>> need > > >>>>>>>>>>>>>>>>>>>>>>>> any > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> check as we will always use > > >> `ValueMapperWithKey`) > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> -Matthias > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> On 5/9/17 2:55 AM, Jeyhun Karimov wrote: > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi, > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for feedback. > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Then we need to overload method > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> <VR> KStream<K, VR> > mapValues(ValueMapper<? > > >>>>> super > > >>>>>>>> V, > > >>>>>>>>>> ? > > >>>>>>>>>>>>>>>>>> extends > > >>>>>>>>>>>>>>>>>>>>>>> VR> > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> mapper); > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> with > > >>>>>>>>>>>>>>>>>>>>>>>>>>>>> <VR> KStream<K, VR> > > >>>>>>> mapValues(ValueMapperWithKey<? > > >>>>>>>>>>>> super > > >>>>>>>>>>>>>>>>> V, > > >>>>>>>>>>>>>>>>>> ? > > >>>>>>>>>>>>>>>>>>>>>>>> extends > > >>>>>>>>>>>>>>> > -- -- Guozhang
