kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <matth...@confluent.io>
Subject Re: [DISCUSS] KIP-439: Deprecate Interface WindowStoreIterator
Date Thu, 11 Apr 2019 09:26:20 GMT
Thanks for the input.

>> Just to clarify the naming conflicts is between the newly added function
>> and the old functions that we want to deprecate / remove right? The

Yes, the conflict is just fort the existing `fetch()` methods for which
we want to change the return type.

IMHO, we should not make a breaking change in a minor release. Thus, we
could either only deprecate those fetch methods that return
`WindowStoreIterator` and do a "clean cut" in 3.0.

Or we, follow the renaming path. No get a clean renaming, we need to
consider all methods that are called "fetch":

ReadOnlyWindowStore:

> V fetch(K, long)
> WindowStoreIterator<V> fetch(K, Instant, Instant)
> KeyValueIterator<Windowed<K>, V> fetch(K, K, Instant, Instant)

WindowStore:

> WindowStoreIterator<V> fetch(K, long, long)
> WindowStoreIterator<V> fetch(K, Instant, Instant)> KeyValueIterator<Windowed<K>,
V> fetch(K, K, long, long)
> KeyValueIterator<Windowed<K>, V> fetch(K, K, Instant, Instant)

There is also fetchAll(long, long) and fetchAll(Instant, Instant) that
fetch over all keys.

Maybe we could rename `V fetch(K, long)` to `V get(K, long)` and rename
all `fetch()/fetchAll()` to `range()`? There is actually no reason to
have range()/rangeAll().

If we do this, we might consider to rename methods for SessionStore,
too. There is

> ReadOnlySessionStore#fetch(K)
> ReadOnlySessionStore#fetch(K, K)
> SessionStore#findSessions(K, long, long)
> SessionStore#findSessions(K, K, long, long)
> SessionStore#fetchSession(K, long, long);

Consistent renaming might be:

ReadOnlySessionStore#range(K)
ReadOnlySessionStore#range(K, K)
SessionStore#range(K, long, long)
SessionStore#range(K, K, long, long)
SessionStore#get(K, long, long);


Not sure if extensive renaming might have too big of an impact. However,
`range()` and `get()` might actually be better names than `fetch()`,
thus, it might also provide some additional value.

Thoughts?


-Matthias


On 3/27/19 10:19 AM, Guozhang Wang wrote:
> Hello Matthias,
> 
> Just to clarify the naming conflicts is between the newly added function
> and the old functions that we want to deprecate / remove right? The
> existing ones have different signatures with parameters so that they should
> not have conflicts.
> 
> I was thinking about just make the change directly without deprecating
> existing ones which would require users of 2.3 to make code changes -- this
> code change looks reasonably straight-forward to me and not much worth
> deferring to later when the deprecated ones are removed.
> 
> On the other hand, just deprecating "WindowIterator" without add new
> functions seems not very useful for users either since it is only used as
> an indicator but users cannot make code changes during this phase anyways,
> so it is still a `one-cut` deal when we eventually remove the deprecated
> ones and add the new one.
> 
> Hence I'm slightly inclining to trade compatibility and replace it with new
> functions in one release, but if people have a good idea of the renaming
> approach (I do not have a good one on top of my head :) I can also be
> convinced that way.
> 
> Guozhang
> 
> 
> On Mon, Mar 11, 2019 at 10:41 AM Matthias J. Sax <matthias@confluent.io>
> wrote:
> 
>> I am open to change the return type to
>>
>> KeyValueIterator<Windowed<K>, V>
>>
>> However, this requires to rename
>>
>> #fetch(K key, long startTimestamp, long endTimestamp)
>> #fetch(K key, Instant startTimestamp, Instant endTimestamp)
>>
>> to avoid naming conflicts.
>>
>> What new name would you suggest? The existing methods are called
>> `fetch()`, `fetchAll()`, `all()`, `put()`.
>>
>> While I think it would be good to get fully aligned return types, I am
>> not sure how we can get aligned method names (without renaming all of
>> them...)? If we think it's worth to rename all to get this cleaned up, I
>> am no opposed.
>>
>>
>> Thoughts?
>>
>>
>> -Matthias
>>
>>
>> On 3/11/19 10:27 AM, Guozhang Wang wrote:
>>> I was thinking about changing the return type even, to
>>> `KeyValueIterator<Windowed<K>, V>` since it is confusing to users
about
>> the
>>> key typed `Long` (Streams javadoc today did not explain it clearly
>> either),
>>> note it is not backward compatible at all.
>>>
>>> Personally I'd prefer to just deprecate the API and new new ones that
>>> return `KeyValueIterator<Windowed<K>, V>` directly, but if most people
>> felt
>>> it is too intrusive for compatibility I can be convinced with
>>> `KeyValueIterator<Long, V>` as well.
>>>
>>> Guozhang
>>>
>>> On Mon, Mar 11, 2019 at 10:17 AM Sophie Blee-Goldman <
>> sophie@confluent.io>
>>> wrote:
>>>
>>>> I remember thinking this while working on window stores, am definitely
>> for
>>>> it.
>>>>
>>>> On Mon, Mar 11, 2019 at 9:20 AM John Roesler <john@confluent.io> wrote:
>>>>
>>>>> Sounds great to me. Thanks, Matthias!
>>>>> -John
>>>>>
>>>>> On Sun, Mar 10, 2019 at 11:58 PM Matthias J. Sax <
>> matthias@confluent.io>
>>>>> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I would like to propose KIP-439 to deprecate interface
>>>>>> `WindowStoreIterator`.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-439%3A+Deprecate+Interface+WindowStoreIterator
>>>>>>
>>>>>> Looking forward to your feedback.
>>>>>>
>>>>>>
>>>>>> -Matthias
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
> 


Mime
View raw message