I like the renaming, since it also aligns with our metrics cleanup (KIP-444) which touches upon the store level metrics as well.
One question: you seems still suggesting to keep "all" with the current name (and also using a separate metric for it), what's the difference between this one and other "range" functions? Guozhang On Thu, Apr 11, 2019 at 2:26 AM Matthias J. Sax <matth...@confluent.io> wrote: > 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 > >>>>>> > >>>>>> > >>>>> > >>>> > >>> > >>> > >> > >> > > > > -- -- Guozhang
