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 <matth...@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 < >> sop...@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 <j...@confluent.io> wrote: >>>> >>>>> Sounds great to me. Thanks, Matthias! >>>>> -John >>>>> >>>>> On Sun, Mar 10, 2019 at 11:58 PM Matthias J. Sax < >> matth...@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 >>>>>> >>>>>> >>>>> >>>> >>> >>> >> >> >
signature.asc
Description: OpenPGP digital signature
