Thanks John.
I think I'm convinced about not collapsing the metrics measuring for
various function calls. Regarding the possible solution I'm a bit inclined
to just distinguish on the metric names directly over on tags: since our
current metrics naming are a tad messed up anyways, fixing them in one shot
as a breaking change sounds reasonable to me.
Guozhang
On Mon, May 6, 2019 at 9:14 AM John Roesler <j...@confluent.io> wrote:
> Thanks all (or range? ;) ) for the discussion. Good points all around.
>
> Although I see the allure of naming the metrics the same as the things
> they're measuring, it seems not to be perfect. Seconding Matthias's latter
> thought, I think it's likely you'd want to measure the method calls
> independently, since the different range variants would have wildly
> different characteristics, which could then lead you to want to orient the
> storage differently to support particular use cases.
>
> Pointing out some structural characteristics (I know you all know this
> stuff, I'm just constructing a table for analysis):
> * Java supports method-name overloading. *Different* methods can have the
> same names, distinguished by arg lists; it doesn't change the fact that
> they are different methods.
> * Metrics does not support metric-name overloading, but metric names do
> have some structure we could exploit, if you consider the tags.
>
> It seems to me that there's actually more domain mismatch if we just have
> one metric named "range", since (e.g., in the SessionStore proposal above)
> the Java API has *four* methods named "range".
>
> Two potential solutions I see:
> * hierarchical metric names: "range-single-key-all-time",
> "range-key-range-all-time", "range-single-key-time-range",
> "range-key-range-time-range", maybe with better names... I'm not the best
> at this stuff. Hopefully, you see the point, though... they all start with
> "range", which provides the association to the method, and all have a
> suffix which identifies the overload being measured.
> * tagged metric names: "range" {"variant": "single-key-all-time"}, "range"
> {"variant": "key-range-all-time"}, "range" {"variant":
> "single-key-time-range"}, "range" {"variant": "key-range-time-range"} . Or
> you could even split the tags up semantically, but my instinct says that
> that would just make it harder to digest the metrics later on.
>
> Just some ideas.
> -John
>
> On Fri, Apr 26, 2019 at 3:51 AM Matthias J. Sax <matth...@confluent.io>
> wrote:
>
> > 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
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>>
> > >>>
> > >>
> > >>
> > >
> >
> >
>
--
-- Guozhang
