I’m also for Approach #1. I like simplifying things.
> On 13 Jan 2016, at 14:25, Vasiliki Kalavri <vasilikikala...@gmail.com> wrote:
>
> Hi,
>
> +1 for removing the Combinable annotation. Approach 1 sounds like the
> best option to me.
>
>
> On 13 January 2016 at 14:11, Till Rohrmann <trohrm...@apache.org> wrote:
>
>> Hi Fabian,
>>
>> thanks for bringing this issue up. I agree with you that it would be nice
>> to remove the Combinable annotation if it is not really needed. Now if
>> people are not aware of the Combinable interface then they might think that
>> they are actually using combiners by simply implementing CombineFunction.
>>
>>
> has happened to me :S
>
>
>
>> I would also be in favour of approach 1 combined with a migration guide
>> where we highlight possible problems with a faulty combine implementation.
>>
>>
> Migration guide is a good idea!
>
>
>
>> Cheers,
>> Till
>>
>>
>>
> -Vasia.
>
>
>
>> On Wed, Jan 13, 2016 at 1:53 PM, Fabian Hueske <fhue...@gmail.com> wrote:
>>
>>> Hi everybody,
>>>
>>>
>>>
>>> As part of the upcoming 1.0 release we are stabilizing Flink's APIs.
>>>
>>> I would like to start a discussion about removing the Combinable
>> annotation
>>> from the DataSet API.
>>>
>>>
>>>
>>> # The Current State:
>>>
>>> The DataSet API offers two interface for combine functions,
>> CombineFunction
>>> and GroupCombineFunction, which can be added to a GroupReduceFunctions
>>> (GRF).
>>>
>>>
>>> However, implementing a combine interface does not make a GRF combinable.
>>> In addition, the GRF class needs to be annotated with the Combinable
>>> annotation. The RichGroupReduceFunction has a default implementation of
>>> combine, which forwards the combine parameters to the reduce method. This
>>> default implementation is not used, unless the class that extends RGRF
>> has
>>> the Combinable annotation.
>>>
>>>
>>>
>>> In addition to the Combinable annotation, the DataSet API
>>> GroupReduceOperator features the setCombinable(boolean) method to
>> override
>>> a missing or existing Combinable annotation.
>>>
>>>
>>>
>>> # My Proposal:
>>>
>>> I propose to remove the Combinable annotation because it is not required
>>> and complicates the definition of combiners. Instead, the combine method
>> of
>>> a GroupReduceFunction should be executed if it implements one of the
>>> combine function interfaces. This would require to remove the default
>>> combine implementation of the RichGroupReduceFunction as well.
>>>
>>> This would be an API breaking change and has a few implications.
>>>
>>>
>>>
>>> # Possible Implementation:
>>>
>>> There are a few ways to implement this change.
>>>
>>> - Remove Combinable annotation or mark it deprecated (and keep effect)
>>>
>>> - Remove default combine method from RichGroupReduceFunction or deprecate
>>> it
>>>
>>>
>>>
>>> Approach 1:
>>> - Remove Combinable annotation
>>> - Remove default combine() method from RichGroupReduceFunction
>>> - Effect:
>>> - All RichGroupReduceFunctions that do either have the Combinable
>>> annotation or implement the combine method do not compile anymore
>>> - GroupReduceFunctions that have the Combinable annotation do not
>>> compile anymore
>>> - GroupReduceFunctions that implement a combine interface without
>>> having the annotation (and not being set to combinable during program
>>> construction) will execute the previously not executed combine method.
>> This
>>> might change the behavior of the program. In worst case, the program
>> might
>>> silently produce wrong results (or crash) if the combiner implementation
>>> was faulty. In best case, the program executes faster.
>>>
>>>
>>>
>>> Approach 2:
>>> - As Approach 1
>>> - In addition extend both combine interfaces with a deprecated marker
>>> method. This will ensure that all functions that implement a combinable
>>> interface do not compile anymore and need to be fixed. This could prevent
>>> the silent failure as in Approach 1, but would also cause an additional
>> API
>>> breaking change once the deprecated marker method is removed again.
>>>
>>>
>>>
>>> Approach 3:
>>> - Mark Combinable annotation deprecated
>>> - Mark combine() method in RichGroupReduceFunction as deprecated
>>> - Effect:
>>> - There'll be a couple of deprecation warnings.
>>> - We face the same problem with silent failures as in Approach 1.
>>> - We have to check if RichGroupReduceFunction's override combine or
>> not
>>> (can be done with reflection). If the method is not overridden we do not
>>> execute it (unless there is a Combinable annotation) and we are fine. If
>> it
>>> is overridden and no Combinable annotation has been defined, we have the
>>> same problem with silent failures as before.
>>> - After we remove the deprecated annotation and method, we have the
>>> same effect as with Approach 1.
>>>
>>>
>>>
>>> There are more alternatives, but these are the most viable, IMO.
>>>
>>>
>>>
>>> I think, if we want to remove the combinable annotation, we should do it
>>> now.
>>>
>>> Given the three options, would go for Approach 1. Yes, breaks a lot of
>> code
>>> and yes there is the possibility of computing incorrect results.
>> Approach 2
>>> is safer but would mean another API breaking change in the future.
>> Approach
>>> 3 comes with fewer breaking changes but has the same problem of silent
>>> failures.
>>>
>>> IMO, the breaking API changes of Approach 1 are even desirable because
>> they
>>> will make users aware that this feature changed.
>>>
>>>
>>>
>>> What do you think?
>>>
>>>
>>>
>>> Cheers, Fabian
>>>
>>
