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
>>>
>>
signature.asc
Description: OpenPGP digital signature
