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 >>>>>>>>>>>>>>> VR> >>>>>>>>>>>>>>>> mapper); >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> and in runtime (inside processor) we still have to check it >>>> is >>>>>>>>>>>>>>> ValueMapper >>>>>>>>>>>>>>>> or ValueMapperWithKey before wrapping it into the rich >>>>> function. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Please correct me if I am wrong. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Cheers, >>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Tue, May 9, 2017 at 10:56 AM Michal Borowiecki < >>>>>>>>>>>>>>>> michal.borowie...@openbet.com> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> +1 :) >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On 08/05/17 23:52, Matthias J. Sax wrote: >>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> I was reading the updated KIP and I am wondering, if we >>>>> should >>>>>>>> do >>>>>>>>>>> the >>>>>>>>>>>>>>>>>> design a little different. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Instead of distinguishing between a RichFunction and >>>>>>>>>>> non-RichFunction >>>>>>>>>>>>>>> at >>>>>>>>>>>>>>>>>> runtime level, we would use RichFunctions all the time. >>>>> Thus, >>>>>> on >>>>>>>>>>> the >>>>>>>>>>>>>>> DSL >>>>>>>>>>>>>>>>>> entry level, if a user provides a non-RichFunction, we >>>> wrap >>>>> it >>>>>>>>>> by a >>>>>>>>>>>>>>>>>> RichFunction that is fully implemented by Streams. This >>>>>>>>>>> RichFunction >>>>>>>>>>>>>>>>>> would just forward the call omitting the key: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Just to sketch the idea (incomplete code snippet): >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> public StreamsRichValueMapper implements >>>> RichValueMapper() >>>>> { >>>>>>>>>>>>>>>>>>> private ValueMapper userProvidedMapper; // set by >>>>>>>> constructor >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> public VR apply(final K key, final V1 value1, final V2 >>>>>>>>>> value2) >>>>>>>>>>> { >>>>>>>>>>>>>>>>>>> return userProvidedMapper(value1, value2); >>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> From a performance point of view, I am not sure if the >>>>>>>>>>>>>>>>>> "if(isRichFunction)" including casts etc would have more >>>>>>>> overhead >>>>>>>>>>>>> than >>>>>>>>>>>>>>>>>> this approach (we would do more nested method call for >>>>>>>>>>>>> non-RichFunction >>>>>>>>>>>>>>>>>> which should be more common than RichFunctions). >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> This approach should not effect lambdas (or do I miss >>>>>>>> something?) >>>>>>>>>>> and >>>>>>>>>>>>>>>>>> might be cleaner, as we could have one more top level >>>>>> interface >>>>>>>>>>>>>>>>>> `RichFunction` with methods `init()` and `close()` and >>>> also >>>>>>>>>>>>> interfaces >>>>>>>>>>>>>>>>>> for `RichValueMapper` etc. (thus, no abstract classes >>>>>> required). >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Any thoughts? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On 5/6/17 5:29 PM, Jeyhun Karimov wrote: >>>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Thanks for comments. I extended PR and KIP to include >>>> rich >>>>>>>>>>>>> functions. >>>>>>>>>>>>>>> I >>>>>>>>>>>>>>>>>>> will still have to evaluate the cost of deep copying of >>>>> keys. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Cheers, >>>>>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> On Fri, May 5, 2017 at 8:02 PM Mathieu Fenniak < >>>>>>>>>>>>>>>>> mathieu.fenn...@replicon.com> >>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Hey Matthias, >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> My opinion would be that documenting the immutability of >>>>> the >>>>>>>>>> key >>>>>>>>>>> is >>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>> best approach available. Better than requiring the key >>>> to >>>>>> be >>>>>>>>>>>>>>>>> serializable >>>>>>>>>>>>>>>>>>>> (as with Jeyhun's last pass at the PR), no performance >>>>> risk. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> It'd be different if Java had immutable type constraints >>>>> of >>>>>>>>>> some >>>>>>>>>>>>>>> kind. >>>>>>>>>>>>>>>>> :-) >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Mathieu >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> On Fri, May 5, 2017 at 11:31 AM, Matthias J. Sax < >>>>>>>>>>>>>>>>> matth...@confluent.io> >>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Agreed about RichFunction. If we follow this path, it >>>>>> should >>>>>>>>>>> cover >>>>>>>>>>>>>>>>>>>>> all(?) DSL interfaces. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> About guarding the key -- I am still not sure what to >>>> do >>>>>>>> about >>>>>>>>>>>>> it... >>>>>>>>>>>>>>>>>>>>> Maybe it might be enough to document it (and name the >>>> key >>>>>>>>>>>>> parameter >>>>>>>>>>>>>>>>> like >>>>>>>>>>>>>>>>>>>>> `readOnlyKey` to make is very clear). Ultimately, I >>>> would >>>>>>>>>> prefer >>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>> guard against any modification, but I have no good idea >>>>> how >>>>>>>> to >>>>>>>>>>> do >>>>>>>>>>>>>>>>> this. >>>>>>>>>>>>>>>>>>>>> Not sure what others think about the risk of corrupted >>>>>>>>>>>>> partitioning >>>>>>>>>>>>>>>>>>>>> (what would be a user error and we could say, well, bad >>>>>> luck, >>>>>>>>>>> you >>>>>>>>>>>>>>> got >>>>>>>>>>>>>>>>> a >>>>>>>>>>>>>>>>>>>>> bug in your code, that's not our fault), vs deep copy >>>>> with >>>>>> a >>>>>>>>>>>>>>> potential >>>>>>>>>>>>>>>>>>>>> performance hit (that we can't quantity atm without any >>>>>>>>>>>>> performance >>>>>>>>>>>>>>>>>>>> test). >>>>>>>>>>>>>>>>>>>>> We do have a performance system test. Maybe it's worth >>>>> for >>>>>>>> you >>>>>>>>>>> to >>>>>>>>>>>>>>>>> apply >>>>>>>>>>>>>>>>>>>>> the deep copy strategy and run the test. It's very >>>> basic >>>>>>>>>>>>> performance >>>>>>>>>>>>>>>>>>>>> test only, but might give some insight. If you want to >>>> do >>>>>>>>>> this, >>>>>>>>>>>>> look >>>>>>>>>>>>>>>>>>>>> into folder "tests" for general test setup, and into >>>>>>>>>>>>>>>>>>>>> "tests/kafaktests/benchmarks/streams" to find find the >>>>> perf >>>>>>>>>>> test. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> On 5/5/17 8:55 AM, Jeyhun Karimov wrote: >>>>>>>>>>>>>>>>>>>>>> Hi Matthias, >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> I think extending KIP to include RichFunctions totally >>>>>>>> makes >>>>>>>>>>>>>>> sense. >>>>>>>>>>>>>>>>>>>> So, >>>>>>>>>>>>>>>>>>>>>> we don't want to guard the keys because it is >>>> costly. >>>>>>>>>>>>>>>>>>>>>> If we introduce RichFunctions I think it should not be >>>>>>>>>> limited >>>>>>>>>>>>> only >>>>>>>>>>>>>>>>>>>> the 3 >>>>>>>>>>>>>>>>>>>>>> interfaces proposed in KIP but to wide range of >>>>>> interfaces. >>>>>>>>>>>>>>>>>>>>>> Please correct me if I am wrong. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Cheers, >>>>>>>>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> On Fri, May 5, 2017 at 12:04 AM Matthias J. Sax < >>>>>>>>>>>>>>>>> matth...@confluent.io >>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> One follow up. There was this email on the user list: >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>> http://search-hadoop.com/m/Kafka/uyzND17KhCaBzPSZ1?subj= >>>>>>>>>>>>>>>>>>>>> >>>>> Shouldn+t+the+initializer+of+a+stream+aggregate+accept+the+ >>>>>>>>>> key+ >>>>>>>>>>>>>>>>>>>>>>> It might make sense so include Initializer, Adder, >>>> and >>>>>>>>>>>>> Substractor >>>>>>>>>>>>>>>>>>>>>>> inferface, too. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> And we should double check if there are other >>>> interface >>>>>> we >>>>>>>>>>> might >>>>>>>>>>>>>>>>> miss >>>>>>>>>>>>>>>>>>>>> atm. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> On 5/4/17 1:31 PM, Matthias J. Sax wrote: >>>>>>>>>>>>>>>>>>>>>>>> Thanks for updating the KIP. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> Deep copying the key will work for sure, but I am >>>>>> actually >>>>>>>>>> a >>>>>>>>>>>>>>> little >>>>>>>>>>>>>>>>>>>> bit >>>>>>>>>>>>>>>>>>>>>>>> worried about performance impact... We might want to >>>>> do >>>>>>>>>> some >>>>>>>>>>>>> test >>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>>>> quantify this impact. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> Btw: this remind me about the idea of `RichFunction` >>>>>>>>>>> interface >>>>>>>>>>>>>>> that >>>>>>>>>>>>>>>>>>>>>>>> would allow users to access record metadata (like >>>>>>>>>> timestamp, >>>>>>>>>>>>>>>>> offset, >>>>>>>>>>>>>>>>>>>>>>>> partition etc) within DSL. This would be a similar >>>>>>>> concept. >>>>>>>>>>>>>>> Thus, I >>>>>>>>>>>>>>>>>>>> am >>>>>>>>>>>>>>>>>>>>>>>> wondering, if it would make sense to enlarge the >>>> scope >>>>>> of >>>>>>>>>>> this >>>>>>>>>>>>>>> KIP >>>>>>>>>>>>>>>>> by >>>>>>>>>>>>>>>>>>>>>>>> that? WDYT? >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> On 5/3/17 2:08 AM, Jeyhun Karimov wrote: >>>>>>>>>>>>>>>>>>>>>>>>> Hi Mathieu, >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> Thanks for feedback. I followed similar approach >>>> and >>>>>>>>>> updated >>>>>>>>>>>>> PR >>>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>>>>>>> KIP >>>>>>>>>>>>>>>>>>>>>>>>> accordingly. I tried to guard the key in Processors >>>>>>>>>> sending >>>>>>>>>>> a >>>>>>>>>>>>>>> copy >>>>>>>>>>>>>>>>>>>> of >>>>>>>>>>>>>>>>>>>>> an >>>>>>>>>>>>>>>>>>>>>>>>> actual key. >>>>>>>>>>>>>>>>>>>>>>>>> Because I am doing deep copy of an object, I think >>>>>> memory >>>>>>>>>>> can >>>>>>>>>>>>> be >>>>>>>>>>>>>>>>>>>>>>> bottleneck >>>>>>>>>>>>>>>>>>>>>>>>> in some use-cases. >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> Cheers, >>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> On Tue, May 2, 2017 at 5:10 PM Mathieu Fenniak < >>>>>>>>>>>>>>>>>>>>>>> mathieu.fenn...@replicon.com> >>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> Hi Jeyhun, >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> This approach would change ValueMapper (...etc) to >>>>> be >>>>>>>>>>>>> classes, >>>>>>>>>>>>>>>>>>>> rather >>>>>>>>>>>>>>>>>>>>>>> than >>>>>>>>>>>>>>>>>>>>>>>>>> interfaces, which is also a backwards incompatible >>>>>>>>>> change. >>>>>>>>>>>>> An >>>>>>>>>>>>>>>>>>>>>>> alternative >>>>>>>>>>>>>>>>>>>>>>>>>> approach that would be backwards compatible would >>>> be >>>>>> to >>>>>>>>>>>>> define >>>>>>>>>>>>>>>>> new >>>>>>>>>>>>>>>>>>>>>>>>>> interfaces, and provide overrides where those >>>>>> interfaces >>>>>>>>>>> are >>>>>>>>>>>>>>>>> used. >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> Unfortunately, making the key parameter as "final" >>>>>>>>>> doesn't >>>>>>>>>>>>>>> change >>>>>>>>>>>>>>>>>>>>> much >>>>>>>>>>>>>>>>>>>>>>>>>> about guarding against key change. It only >>>> prevents >>>>>> the >>>>>>>>>>>>>>>>> parameter >>>>>>>>>>>>>>>>>>>>>>> variable >>>>>>>>>>>>>>>>>>>>>>>>>> from being reassigned. If the key type is a >>>> mutable >>>>>>>>>> object >>>>>>>>>>>>>>> (eg. >>>>>>>>>>>>>>>>>>>>>>> byte[]), >>>>>>>>>>>>>>>>>>>>>>>>>> it can still be mutated. (eg. key[0] = 0). But >>>> I'm >>>>>> not >>>>>>>>>>>>> really >>>>>>>>>>>>>>>>> sure >>>>>>>>>>>>>>>>>>>>>>> there's >>>>>>>>>>>>>>>>>>>>>>>>>> much that can be done about that. >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> Mathieu >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, May 1, 2017 at 5:39 PM, Jeyhun Karimov < >>>>>>>>>>>>>>>>>>>> je.kari...@gmail.com >>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks for comments. >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> The concerns makes sense. Although we can guard >>>> for >>>>>>>>>>>>> immutable >>>>>>>>>>>>>>>>> keys >>>>>>>>>>>>>>>>>>>>> in >>>>>>>>>>>>>>>>>>>>>>>>>>> current implementation (with few changes), I >>>> didn't >>>>>>>>>>> consider >>>>>>>>>>>>>>>>>>>>> backward >>>>>>>>>>>>>>>>>>>>>>>>>>> compatibility. >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> In this case 2 solutions come to my mind. In both >>>>>>>> cases, >>>>>>>>>>>>> user >>>>>>>>>>>>>>>>>>>>> accesses >>>>>>>>>>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>>>>>>> key in Object type, as passing extra type >>>> parameter >>>>>>>> will >>>>>>>>>>>>> break >>>>>>>>>>>>>>>>>>>>>>>>>>> backwards-compatibility. So user has to cast to >>>>>> actual >>>>>>>>>>> key >>>>>>>>>>>>>>>>> type. >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> 1. Firstly, We can overload apply method with 2 >>>>>>>> argument >>>>>>>>>>>>> (key >>>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>>>>>>>>> value) >>>>>>>>>>>>>>>>>>>>>>>>>>> and force key to be *final*. By doing this, I >>>>> think >>>>>> we >>>>>>>>>>> can >>>>>>>>>>>>>>>>>>>> address >>>>>>>>>>>>>>>>>>>>>>> both >>>>>>>>>>>>>>>>>>>>>>>>>>> backward-compatibility and guarding against key >>>>>> change. >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> 2. Secondly, we can create class KeyAccess like: >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> public class KeyAccess { >>>>>>>>>>>>>>>>>>>>>>>>>>> Object key; >>>>>>>>>>>>>>>>>>>>>>>>>>> public void beforeApply(final Object key) { >>>>>>>>>>>>>>>>>>>>>>>>>>> this.key = key; >>>>>>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>>>>>> public Object getKey() { >>>>>>>>>>>>>>>>>>>>>>>>>>> return key; >>>>>>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> We can extend *ValueMapper, ValueJoiner* and >>>>>>>>>>>>>>> *ValueTransformer* >>>>>>>>>>>>>>>>>>>> from >>>>>>>>>>>>>>>>>>>>>>>>>>> *KeyAccess*. Inside processor (for example >>>>>>>>>>>>>>>>>>>>> *KTableMapValuesProcessor*) >>>>>>>>>>>>>>>>>>>>>>>>>>> before calling *mapper.apply(value)* we can set >>>> the >>>>>>>>>> *key* >>>>>>>>>>> by >>>>>>>>>>>>>>>>>>>>>>>>>>> *mapper.beforeApply(key)*. As a result, user can >>>>> use >>>>>>>>>>>>>>> *getKey()* >>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>>> access >>>>>>>>>>>>>>>>>>>>>>>>>>> the key inside *apply(value)* method. >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> Cheers, >>>>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, May 1, 2017 at 7:24 PM Matthias J. Sax < >>>>>>>>>>>>>>>>>>>>> matth...@confluent.io >>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun, >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> thanks a lot for the KIP! >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> I think there are two issues we need to address: >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> (1) The KIP does not consider backward >>>>>> compatibility. >>>>>>>>>>> Users >>>>>>>>>>>>>>> did >>>>>>>>>>>>>>>>>>>>>>>>>> complain >>>>>>>>>>>>>>>>>>>>>>>>>>>> about this in past releases already, and as the >>>>> user >>>>>>>>>> base >>>>>>>>>>>>>>>>> grows, >>>>>>>>>>>>>>>>>>>> we >>>>>>>>>>>>>>>>>>>>>>>>>>>> should not break backward compatibility in >>>> future >>>>>>>>>>> releases >>>>>>>>>>>>>>>>>>>> anymore. >>>>>>>>>>>>>>>>>>>>>>>>>>>> Thus, we should think of a better way to allow >>>> key >>>>>>>>>>> access. >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> Mathieu's comment goes into the same direction >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On the other hand, the number of compile >>>>> failures >>>>>>>>>> that >>>>>>>>>>>>>>> would >>>>>>>>>>>>>>>>>>>> need >>>>>>>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>>>>>>> be >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> fixed from this change is unfortunate. :-) >>>>>>>>>>>>>>>>>>>>>>>>>>>> (2) Another concern is, that there is no guard >>>> to >>>>>>>>>> prevent >>>>>>>>>>>>>>> user >>>>>>>>>>>>>>>>>>>> code >>>>>>>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>>>>>>>> modify the key. This might corrupt partitioning >>>> if >>>>>>>>>> users >>>>>>>>>>> do >>>>>>>>>>>>>>>>> alter >>>>>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>>>>>>>> key (accidentally -- or users are just not aware >>>>>> that >>>>>>>>>>> they >>>>>>>>>>>>>>> are >>>>>>>>>>>>>>>>>>>> not >>>>>>>>>>>>>>>>>>>>>>>>>>>> allowed to modify the provided key object) and >>>>> thus >>>>>>>>>> break >>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>>>>>>>> application. (This was the original motivation >>>> to >>>>>> not >>>>>>>>>>>>> provide >>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>> key >>>>>>>>>>>>>>>>>>>>>>>>>> in >>>>>>>>>>>>>>>>>>>>>>>>>>>> the first place -- it's guards against >>>>>> modification.) >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> On 5/1/17 6:31 AM, Mathieu Fenniak wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hi Jeyhun, >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> I just want to add my voice that, I too, have >>>>>> wished >>>>>>>>>> for >>>>>>>>>>>>>>>>> access >>>>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>>>>>>>>> record key during a mapValues or similar >>>>> operation. >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> On the other hand, the number of compile >>>> failures >>>>>>>> that >>>>>>>>>>>>> would >>>>>>>>>>>>>>>>>>>> need >>>>>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>>>>>> be >>>>>>>>>>>>>>>>>>>>>>>>>>>>> fixed from this change is unfortunate. :-) But >>>>> at >>>>>>>>>> least >>>>>>>>>>>>> it >>>>>>>>>>>>>>>>>>>> would >>>>>>>>>>>>>>>>>>>>>>> all >>>>>>>>>>>>>>>>>>>>>>>>>>> be >>>>>>>>>>>>>>>>>>>>>>>>>>>> a >>>>>>>>>>>>>>>>>>>>>>>>>>>>> pretty clear and easy change. >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Mathieu >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>> On Mon, May 1, 2017 at 6:55 AM, Jeyhun Karimov >>>> < >>>>>>>>>>>>>>>>>>>>>>> je.kari...@gmail.com >>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dear community, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I want to share KIP-149 [1] based on issues >>>>>>>>>> KAFKA-4218 >>>>>>>>>>>>> [2], >>>>>>>>>>>>>>>>>>>>>>>>>> KAFKA-4726 >>>>>>>>>>>>>>>>>>>>>>>>>>>> [3], >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> KAFKA-3745 [4]. The related PR can be found at >>>>>> [5]. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I would like to get your comments. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [1] >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> https://cwiki.apache.org/ >>>>>>>>>> confluence/display/KAFKA/KIP- >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>> 149%3A+Enabling+key+access+in+ValueTransformer%2C+ >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ValueMapper%2C+and+ValueJoiner >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [2] https://issues.apache.org/jira >>>>>>>> /browse/KAFKA-4218 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [3] https://issues.apache.org/jira >>>>>>>> /browse/KAFKA-4726 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [4] https://issues.apache.org/jira >>>>>>>> /browse/KAFKA-3745 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> [5] https://github.com/apache/kafka/pull/2946 >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Cheers, >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -Cheers >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>>>>>>>>>>> -Cheers >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>>>>>> -Cheers >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>> -Cheers >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> -- >>>>>>>>>>>>>> -Cheers >>>>>>>>>>>>>> >>>>>>>>>>>>>> Jeyhun >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> -- >>>>> -Cheers >>>>> >>>>> Jeyhun >>>>> >>>> >> >> -- > -Cheers > > Jeyhun >
signature.asc
Description: OpenPGP digital signature
