Thanks for the input Guozhang. I was not aware of those dependencies. It might be good to align this KIP with the metrics cleanup. Not sure atm, if we should use different metric names for different overloads, even if those have the same method name?
If we rename all method to `range()` and use the same metric name for all, one could argue that this is still fine, because the metric collects how often a range query is executed (regardless of the range itself). On the other hand, this would basically be a "roll up". It could still be valuable to distinguish between single-key-time-range, key-range-time-range, and all-range queries. Users could still aggregate those later if they are not interested in the details, while it's not possible for user to split a pre-aggregated metric into it's component. Input from others might be helpful here, too. -Matthias On 4/11/19 6:00 PM, Guozhang Wang wrote: > 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 >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>> >> >> >
signature.asc
Description: OpenPGP digital signature
