kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <matth...@confluent.io>
Subject Re: [DISCUSS] KIP-439: Deprecate Interface WindowStoreIterator
Date Thu, 20 Jun 2019 22:04:13 GMT
I have performance concerns about this proposal, because each time a
window/session store is accessed, a new (unnecessary) object needs to be
created, and accessing the store in the processors is on the hot code path.


-Matthias


On 6/20/19 10:57 AM, John Roesler wrote:
> Hi again, all,
> 
> After wrestling with some other issues around the window interface,
> I'd propose that we consider normalizing the WindowStore (and
> SessionStore) interfaces with respect to KeyValueStore. We can't
> actually make the classes related because they'll clash on the
> deprecated methods, but we can align them. Doing so would be an
> ergonomic advantage, since all our provided store interfaces would
> have the same "shape".
> 
> Specifically, this means that we would deprecate any method that takes
> a raw "K key, long windowStartTime" or returns a raw "K key", and
> instead always take as arguments Windowed<K> keys and also return
> Windowed<K> results. This proposal already comes close to this, by
> re-using the KeyValueIterator<Windowed<K, V>>, so it would just be a
> few tweaks to "perfect" it.
> 
> What do you think?
> 
> Thanks,
> -John
> 
> On Tue, May 28, 2019 at 2:58 PM Matthias J. Sax <matthias@confluent.io> wrote:
>>
>> Let's see what other think about (2)
>>
>>
>>
>> (3) Interesting. My reasoning follows the code:
>>
>> For example, `RocksDbKeyValueBytesStoreSupplier#metricsScope()`returns
>> "rocksdb-state" and this is concatenated later in `MeteredKeyValueStore`
>> that adds a tag
>>
>>   key:       metricScope + "-id"
>>   value:     name()                  // this is the store name
>>
>> Hence, the `-state` part comes from the supplier and thus it seems to be
>> part of `[store-type]` (ie, `store-type` == metricScope()` -- not sure
>> if this interpretation holds).
>>
>> If it's not part of `[store-type]` the supplier should not return it as
>> `metricScope()` IMHO, but the `MeteredKeyValueStore` should add it
>> together with `-id` suffix from my understanding.
>>
>> Thoughts?
>>
>>
>>
>> (5) Renaming to `streams` might imply that we need to rename _all_
>> metrics, not just the store metrics that are affected, to avoid
>> inconsistencies.
>>
>> If we really want to rename it, I would rather suggest to do it as part
>> of KIP-444, that is a larger rework anyway. It seems to be out of scope
>> for this KIP.
>>
>>
>> -Matthias
>>
>>
>> On 5/28/19 12:30 PM, Bruno Cadonna wrote:
>>> Hi Matthias,
>>>
>>> 2)
>>> Yes, this is how I meant it.
>>> I am still in favour of `value-by-...` instead of `get`, because it
>>> gives you immediately the meaning of the metric without the need to
>>> know the API of the stores.
>>>
>>> 3)
>>> I asked to avoid misunderstandings because in KIP-444, `-state-` is
>>> stated explicitly in the tag.
>>>
>>> 5)
>>> The question was not about correctness. I know that we use `stream` in
>>> the metrics group. The question was rather if we want to change it.
>>> The streams metrics interface is called `StreamsMetrics`. I understand
>>> that this has low priority. Just wanted to mention it.
>>>
>>> Best,
>>> Bruno
>>>
>>>
>>> On Tue, May 28, 2019 at 9:08 PM Matthias J. Sax <matthias@confluent.io>
wrote:
>>>>
>>>> Thanks Bruno.
>>>>
>>>> I updated the KIP without changing the metric name yet -- want to
>>>> discuss this first.
>>>>
>>>>
>>>>
>>>> 1) I am also ok to keep `all()`
>>>>
>>>>
>>>> 2) That's a good idea. To make sure I understand your suggestion
>>>> correctly, below the mapping between metric name an methods.
>>>> (Some metric names are use by multiple stores):
>>>>
>>>> (ReadOnly)KeyValueStore:
>>>>
>>>>>> value-by-key : ReadOnlyKeyValueStore#get(K key)
>>>>
>>>> -> replaces `get` metric
>>>>
>>>> I am actually not sure if I like this. Just keeping `get` instead of
>>>> `value-by-key`, `value-by-key-and-time` seems more reasonable to me.
>>>>
>>>>
>>>>
>>>> (ReadOnly)WindowStore:
>>>>
>>>>>> value-by-key-and-time : ReadOnlyWindowStore#get(K key, long windowStartTime)
>>>>
>>>> I think a simple `get` might be better
>>>>
>>>>>> range-by-key-and-time-range : ReadOnlyWindowStore#range(K key, Instant
from, Instant to)
>>>>>> range-by-key-and-time-range : WindowStore#range(K key, long fromTime,
long toTime)
>>>>
>>>>>> range-by-key-range-and-time-range : ReadOnlyWindowStore#range(K from,
K to, Instant from, Instant to)
>>>>>> range-by-key-range-and-time-range : WindowStore#range(K from, K to,
long fromTime, long toTime)
>>>>
>>>>>> range-by-time-range : ReadOnlyWindowStore#range(Instant from, Instant
to)
>>>>>> range-by-time-range : WindowStore#range(long fromTime, long toTime)
>>>>
>>>>>> range-all : ReadOnlyWindowStore#all()
>>>>
>>>>
>>>> (ReadOnly)SessionStore:
>>>>
>>>>>> range-by-key : ReadOnlySessionStore#range(K key)
>>>>
>>>>>> range-by-key-range : ReadOnlySessionStore#range(K from, K to)
>>>>>> range-by-key-and-time-range : SessionStore#range(K key, long earliestSessionEndTime,
long latestSessionStartTime)
>>>>
>>>>>> range-by-key-range-and-time-range : SessionStore#range(K keyFrom,
K keyTo, long earliestSessionEndTime, long latestSessionStartTime)
>>>>
>>>>>> value-by-key-and-time : SessionStore#get(K key, long sessionStartTime,
long sessionEndTime)
>>>>
>>>> I think a simple `get` might be better
>>>>
>>>>>> range-all : ReadOnlyKeyValueStore#all()
>>>>
>>>>
>>>>
>>>>
>>>> 3) the `state-` part is already contained in `[storeType]` do I think
>>>> it's correct as-is
>>>>
>>>>
>>>> 4) Ack. Fixed.
>>>>
>>>>
>>>> 5) I think `stream` (plural) is correct. Cf
>>>> https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java#L87
>>>>
>>>>
>>>>
>>>> -Matthias
>>>>
>>>>
>>>> On 5/28/19 3:26 AM, Bruno Cadonna wrote:
>>>>> Hi all,
>>>>>
>>>>> My comments on this KIP:
>>>>>
>>>>> 1. I would use `all()` instead of `range()` because the functionality
>>>>> is immediately clear without the need to look at the parameter list.
>>>>>
>>>>> 2. I would decouple method names from metrics name, because this
>>>>> allows us to change one naming independently from the other in the
>>>>> future. From the previous discussion on this thread, I have also the
>>>>> feeling that the naming requirements differ between Java code and
>>>>> metrics.
>>>>>
>>>>> My proposal for the metric names is the following:
>>>>>
>>>>> value-by-key
>>>>> range-by-key
>>>>> value-by-key-and-time
>>>>> range-by-key-range
>>>>> range-by-time-range
>>>>> range-by-key-and-time-range
>>>>> range-by-key-range-and-time-range
>>>>> range-all
>>>>>
>>>>> I omitted the metric types like latency-avg, etc. The first word
>>>>> denotes whether the query is a point query (`value`) or a range query
>>>>> (`range`). The words after the `by` denote the parameter types of the
>>>>> query. `range-all` is a special case. I hope, I did not miss any.
>>>>> We could also prefix the above names with `get-` if you think, it
>>>>> would make the names clearer.
>>>>>
>>>>> 3. Should `[storeType]-id` be `[storeType]-state-id`?
>>>>>
>>>>> 4. Some method signatures in the KIP under comment `// new` have still
>>>>> the `@Deprecated` annotation. Copy&Paste error?
>>>>>
>>>>> 5. Should `type = stream-[storeType]-metrics` be `type =
>>>>> streams-[storeType]-metrics` or do we need to keep `stream` for
>>>>> historical/backward-compatibility reasons?
>>>>>
>>>>> Best,
>>>>> Bruno
>>>>>
>>>>> On Fri, May 24, 2019 at 5:46 AM Matthias J. Sax <matthias@confluent.io>
wrote:
>>>>>>
>>>>>> Thanks John and Guozhang.
>>>>>>
>>>>>> I also lean towards different metric names.
>>>>>>
>>>>>> While updating the KIP, I also realized that the whole store API
for
>>>>>> window and session store is actually rather inconsistent. Hence,
I
>>>>>> extended the scope of this KIP to cleanup both interfaces and changed
>>>>>> the KIP name to
>>>>>>
>>>>>>    "Cleanup built-in Store interfaces"
>>>>>>
>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=103094198
>>>>>>
>>>>>> I also included a small change to KeyValueStore to align all built-in
>>>>>> stores holistically.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Looking forward to your feedback.
>>>>>>
>>>>>>
>>>>>>
>>>>>> -Matthias
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 5/7/19 1:19 AM, Guozhang Wang wrote:
>>>>>>> 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 <john@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 <matthias@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 <matthias@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 <
>>>>>>>> matthias@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 <
>>>>>>>>>>> matthias@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 <
>>>>>>>>>>>>>>> sophie@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 <john@confluent.io
>>>>>>>>>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Sounds great to me.
Thanks, Matthias!
>>>>>>>>>>>>>>>>>> -John
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Sun, Mar 10, 2019
at 11:58 PM Matthias J. Sax <
>>>>>>>>>>>>>>> matthias@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
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>


Mime
View raw message