While working at KIP-444 (https://github.com/apache/kafka/pull/6498) I realized there are a bunch of issues on metric names v.s. function names, e.g. some function named `fetchAll` are actually measure with `fetch`, etc. So in that KIP I proposed to make the function name aligned with metrics name. So suppose we rename the functions from `fetch` to `range` I'd suggest we make this change as part of KIP-444 as well. Note that it means different functions with the same name `range` will be measured under a single metric then.
But still for function named `all` it will be measured under a separate metric named `all`, so I'm just clarifying with you if that's the intention. Guozhang On Thu, Apr 11, 2019 at 2:04 PM Matthias J. Sax <matth...@confluent.io> wrote: > I did not see a reason to rename `all()` to `range()`. `all()` does not > take any parameters to limit a range and is a good name IMHO. But I am > not married to keep `all()` and if we think we should rename it, too, I > am fine with it. > > Not sure what connection you make to metrics though. Can you elaborate? > > > Would be interested to hear others opinions on this, too. > > > -Matthias > > On 4/11/19 8:38 AM, Guozhang Wang wrote: > > 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
