Hello, I want to start VOTE for this KIP today. Any objections?
В Пн, 27/08/2018 в 10:20 +0300, Nikolay Izhikov пишет:
> Hello, Matthias, John.
>
> Thanks in advance.
>
> > I wanted to let you know that we have dropped the `grace(long)` method from
> > the Windows interface
>
> `grace(long)` removed from the KIP.
>
> > It seems like, if we want to use long millis internally, then we just need
> > to leave Windows alone.
>
> `Windows` removed from proposed API changes.
>
> > In SessionWindows, inactivityGap is Streams-facing.
>
> `inactivityGap` removed from proposed API changes.
>
> > it seems the KIP does not mention `Punctuator#punctuate(long)` should we
> > add it?
>
> Actually, I think we shouldn't do it.
>
> 1. If I understand correctly, user callback may be called every 1 millisecond
> and many callbacks can be instantiated.
> Do we want to wrap every `long timestamp` into Instant in that case?
>
> 2. If we introduce a new method `Punctuator.punctuate(Instant timestamp`
> we should either break backward compatibility with new interface method or
> provide default implementation:
>
> public interface Punctuator {
> void punctuate(Instant timestmp);
>
> default void punctuate(Instant timestamp) {
> punctuate(timestamp.toEpochMilli());
> }
> }
>
> This doesn't seem right to me.
> What do you think?
>
> > I think it's best, if the KIPs gets update with a proposal on how to handle
> > "dual use" parts.
> > It's easier to discuss if it's written down IMHO.
>
> My proposal(copy of "Proposed Changes" section from KIP):
>
> For the methods that used both: internally and as a part of public API the
> proposal is:
>
> 1. In this scope keep existing methods as is.
> Try to reduce the visibility of methods in next tickets.
> 2. Introduce finer methods with Instant and Duration.
>
> В Пт, 24/08/2018 в 10:36 -0700, Matthias J. Sax пишет:
> > It's tricky... :)
> >
> > Some APIs have "dual use" as I mentioned in my first reply. I agree that
> > it would be good to avoid abstract class and use interfaces if possible.
> > As long as the change is source code compatible, it should be fine IMHO
> > -- we need to document binary incompatibility of course.
> >
> > I think it's best, if the KIPs gets update with a proposal on how to
> > handle "dual use" parts. It's easier to discuss if it's written down IMHO.
> >
> > For `ProcessorContext#schedule()`, you are right John: it's seems fine
> > to use `Duration`, as it won't be called often (usually only within
> > `Processor#init()`) -- I mixed it up with `Punctuator#punctuate(long)`.
> > However, thinking about this twice, we might even want to update both
> > methods. Punctuation callbacks don't happen every millisecond and thus
> > the overhead to use `Instance` should not be a problem.
> >
> > @Nikolay: it seems the KIP does not mention `Punctuator#punctuate(long)`
> > -- should we add it?
> >
> >
> > -Matthias
> >
> >
> > On 8/24/18 10:11 AM, John Roesler wrote:
> > > Quick afterthought: I guess that `Window` is exposed to the API via
> > > `Windowed` keys. I think it would be fine to not deprecate the `long`
> > > start
> > > and end, but add `Instant` variants for people preferring that interface.
> > >
> > > On Fri, Aug 24, 2018 at 11:10 AM John Roesler <j...@confluent.io> wrote:
> > >
> > > > Hey Matthias,
> > > >
> > > > Thanks for pointing that out. I agree that we only really need to change
> > > > methods that are API-facing, and we probably want to avoid using
> > > > Duration/Instant for Streams-facing members.
> > > >
> > > > Like I said in my last email, I think the whole Windows interface is
> > > > Streams-facing, and the builders we provide are otherwise API-facing.
> > > > Likewise, `Window` is Streams-facing, so start and end should not use
> > > > Duration. In SessionWindows, inactivityGap is Streams-facing.
> > > >
> > > > I actually think that ProcessorContext#schedule() is API-facing, so it
> > > > should use Duration. The rationale is that streams processing doesn't
> > > > call
> > > > this method, only implementer of Processor do. Does that seem right?
> > > >
> > > > Also, it seems like ReadOnlyWindowStore#fetch() (2x) and #fetchAll()
> > > > are
> > > > API-facing (for IQ). When we call fetch() during processing, it's
> > > > actually
> > > > `WindowStore#fetch()`. Maybe we should move "WindowStoreIterator<V>
> > > > fetch(K
> > > > key, long timeFrom, long timeTo)" to the WindowStore interface and make
> > > > all the ReadOnlyWindowStore methods take Durations. And likewise with
> > > > the
> > > > SessionStore interfaces.
> > > >
> > > > What do you think?
> > > >
> > > > Thanks,
> > > > -John
> > > >
> > > >
> > > >
> > > >
> > > > On Fri, Aug 24, 2018 at 10:51 AM John Roesler <j...@confluent.io> wrote:
> > > >
> > > > > Hi Nikolay,
> > > > >
> > > > > First: I wanted to let you know that we have dropped the `grace(long)`
> > > > > method from the Windows interface, but we do still need to transition
> > > > > the
> > > > > same method on TimeWindows and JoinWindows (
> > > > > https://github.com/apache/kafka/pull/5536)
> > > > >
> > > > > I have also been thinking it would be nice to replace `Windows` with
> > > > > an
> > > > > interface, but for different reasons. I think we can even do it
> > > > > without
> > > > > breaking source compatibility (but it would break binary
> > > > > compatibility):
> > > > > create a new interface `WindowSpec`, deprecate `Windows` and make it
> > > > > implement `WindowSpec`, add a new method:
> > > > > `KGroupedStream#windowedBy(WindowSpec)`, and deprecate the old one.
> > > > >
> > > > > However, I don't think this would solve your problem, since the
> > > > > Windows
> > > > > interface has two audiences: the DSL user and the implementer who
> > > > > wishes to
> > > > > provide a new kind of windowing. I think we want to provide Duration
> > > > > to the
> > > > > former, and long or Duration is fine for the latter. However, both of
> > > > > these
> > > > > audiences are "external", so having an "internal" interface won't fit
> > > > > the
> > > > > bill.
> > > > >
> > > > > I think my last PR #5536 actually helps the situation quite a bit.
> > > > > Let's
> > > > > forget about the deprecated members. Now, all the public members of
> > > > > Windows
> > > > > are abstract methods, so Windows is effectively an interface now.
> > > > > Here's
> > > > > how it looks:
> > > > >
> > > > > public abstract class Windows<W extends Window> {
> > > > > public abstract Map<Long, W> windowsFor(final long timestamp);
> > > > > public abstract long size();
> > > > > public abstract long gracePeriodMs();
> > > > > }
> > > > >
> > > > > Notice that there is no part of this involved with the DSL. When
> > > > > you're
> > > > > writing a topology, you don't call any of these methods. It's
> > > > > strictly an
> > > > > interface that tells a Windows implementation what Streams expects
> > > > > from it.
> > > > > A very simple implementation could have no builder methods at all and
> > > > > just
> > > > > return fixed answers to these method calls (this is basically what
> > > > > UnlimitedWindows does). It seems like, if we want to use long millis
> > > > > internally, then we just need to leave Windows alone.
> > > > >
> > > > > What we do want to change is the builder methods in TimeWindows,
> > > > > JoinWindows, and UnlimitedWindows. For example, `TimeWindows#of(long)`
> > > > > would become `TimeWindows#of(Duration)`, etc. These are the DSL
> > > > > methods.
> > > > >
> > > > > Does that make sense?
> > > > > -John
> > > > >
> > > > >
> > > > >
> > > > > On Thu, Aug 23, 2018 at 8:59 AM Nikolay Izhikov <nizhi...@apache.org>
> > > > > wrote:
> > > > >
> > > > > > Hello, Mathias.
> > > > > >
> > > > > > Thanks for your feedback.
> > > > > >
> > > > > > > Thus, it might make sense to keep old and just add new ones?
> > > > > >
> > > > > > As far as I understand, we will keep old methods anyway to prevent
> > > > > > public API backward compatibility.
> > > > > > I agree with you, methods that used internally shouldn't be
> > > > > > deprecated.
> > > > > >
> > > > > > > End users can use the "nicer" new ones, while we can still use the
> > > > > >
> > > > > > existing ones internally?
> > > > > > > Not sure if it would be possible to keep the old ones without
> > > > > > > exposing
> > > > > >
> > > > > > them as public API?
> > > > > >
> > > > > > I think, when we decide to remove methods with `long` from public
> > > > > > API,
> > > > > > we can do the following:
> > > > > >
> > > > > > 1. Create an interface like `WindowsInternal`.
> > > > > > 2. Change Windows to an interface.
> > > > > > 3. Create package-private implementation `WindowsImpl`.
> > > > > >
> > > > > > ```
> > > > > > package org.apache.kafka.streams.kstream.internals;
> > > > > > public interface WindowsInternal {
> > > > > > public long start();
> > > > > > public long end();
> > > > > > //etc...
> > > > > > }
> > > > > >
> > > > > > package org.apache.kafka.streams.kstream;
> > > > > > public interface Windows<W extends Window> {
> > > > > > public Instant start();
> > > > > > public Instant end();
> > > > > > //...
> > > > > > }
> > > > > >
> > > > > > class WindowsImpl<W extends Window> implements Windows<W>,
> > > > > > WindowsInternal {
> > > > > >
> > > > > > }
> > > > > > ```
> > > > > >
> > > > > > So, in public API we will expose only `Windows` interface and
> > > > > > internally
> > > > > > we can use `WindowsInternal`
> > > > > > But, of course, this will be huge changes in public API.
> > > > > >
> > > > > > > Let me know what you think about this.
> > > > > >
> > > > > > I think in this KIP we shouldn't deprecate methods, that are used
> > > > > > internally.
> > > > > > I changed it, now my proposal is just add new methods.
> > > > > >
> > > > > > Please, let me know if anything more need to be done.
> > > > > >
> > > > > > В Ср, 22/08/2018 в 17:29 -0700, Matthias J. Sax пишет:
> > > > > > > Thanks a lot for the KIP.
> > > > > > >
> > > > > > > From my understanding, the idea of the KIP is to improve the
> > > > > > > public API
> > > > > > > at DSL level. However, not all public methods listed are part of
> > > > > > > DSL
> > > > > > > level API, but part of runtime API. Those methods are called
> > > > > > > during
> > > > > > > processing and are on the hot code path. I am not sure, if we
> > > > > > > want to
> > > > > > > update those methods. We should carefully think about this, and
> > > > > >
> > > > > > consider
> > > > > > > to keep Long/long type to keep runtime overhead small. Note, that
> > > > > > > the
> > > > > > > methods I mention are not required to specify a program using the
> > > > > > > DSL
> > > > > > > and thus is questionable if the DSL API would be improved if we
> > > > > > > change
> > > > > > > the methods.
> > > > > > >
> > > > > > > It's unfortunate, that some part of the public API stretch the DSL
> > > > > > > builder part as well as the runtime part...
> > > > > > >
> > > > > > > This affects the following methods (please double check if I
> > > > > > > missed
> > > > > >
> > > > > > any):
> > > > > > >
> > > > > > > - Windows#windowsFor()
> > > > > > > - Window#start()
> > > > > > > - Window#end()
> > > > > > > - JoinWindows#windowFor()
> > > > > > > - SessionWindows#inactivitiyGap()
> > > > > > > - TimeWindows#windowFor()
> > > > > > > - UnlimitedWindows#windowFor()
> > > > > > > - ProcessorContext#schedule()
> > > > > > > - ReadOnlyWindowStore#fetch() (2x) and #fetchAll()
> > > > > > > - SessionStore#findSessions() (2x)
> > > > > > >
> > > > > > > maybe
> > > > > > > - TimeWindowedDeserializer#getWindowSize() (it's unused atm, but
> > > > > > > I
> > > > > > > could imagine that it might be use on the hot code path in the
> > > > > > > furture)
> > > > > > >
> > > > > > > So methods have "dual" use and might be called externally and
> > > > > >
> > > > > > internally:
> > > > > > >
> > > > > > > - Window#start()
> > > > > > > - Window#end()
> > > > > > > - ReadOnlyWindowStore#fetch() (2x) and #fetchAll()
> > > > > > > - SessionStore#findSessions() (2x)
> > > > > > >
> > > > > > > Thus, it might make sense to keep old and just add new ones? End
> > > > > > > users
> > > > > > > can use the "nicer" new ones, while we can still use the existing
> > > > > > > ones
> > > > > > > internally? Not sure if it would be possible to keep the old ones
> > > > > > > without exposing them as public API?
> > > > > > >
> > > > > > > Let me know what you think about this.
> > > > > > >
> > > > > > >
> > > > > > > -Matthias
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 8/21/18 11:41 PM, Nikolay Izhikov wrote:
> > > > > > > > Dear, commiters.
> > > > > > > >
> > > > > > > > Please, pay attention to this KIP and share your opinion.
> > > > > > > >
> > > > > > > > В Вт, 21/08/2018 в 11:14 -0500, John Roesler пишет:
> > > > > > > > > I'll solicit more reviews. Let's get at least one committer to
> > > > > >
> > > > > > chime in
> > > > > > > > > before we start a vote (since we need their approval anyway).
> > > > > > > > > -John
> > > > > > > > >
> > > > > > > > > On Mon, Aug 20, 2018 at 12:39 PM Nikolay Izhikov <
> > > > > >
> > > > > > nizhi...@apache.org>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hello, Ted.
> > > > > > > > > >
> > > > > > > > > > Thanks for the comment.
> > > > > > > > > >
> > > > > > > > > > I've edit KIP and change proposal to `windowSize`.
> > > > > > > > > >
> > > > > > > > > > Guys, any other comments?
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > В Вс, 19/08/2018 в 14:57 -0700, Ted Yu пишет:
> > > > > > > > > > > bq. // or just Duration windowSize();
> > > > > > > > > > >
> > > > > > > > > > > +1 to the above choice.
> > > > > > > > > > > The duration is obvious from the return type. For getter
> > > > > >
> > > > > > methods, we
> > > > > > > > > >
> > > > > > > > > > don't
> > > > > > > > > > > use get as prefix (as least for new code).
> > > > > > > > > > >
> > > > > > > > > > > Cheers
> > > > > > > > > > >
> > > > > > > > > > > On Sun, Aug 19, 2018 at 8:03 AM Nikolay Izhikov <
> > > > > >
> > > > > > nizhi...@apache.org>
> > > > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hello, John.
> > > > > > > > > > > >
> > > > > > > > > > > > Thank you very much for your feedback!
> > > > > > > > > > > > I've addressed all your comments.
> > > > > > > > > > > > Please, see my answers and let my know is anything in
> > > > > > > > > > > > KIP
> > > > > >
> > > > > > [1] needs to
> > > > > > > > > >
> > > > > > > > > > be
> > > > > > > > > > > > improved.
> > > > > > > > > > > >
> > > > > > > > > > > > > The correct choice is actually "Instant", not>
> > > > > >
> > > > > > "LocalDateTime"
> > > > > > > > > > > >
> > > > > > > > > > > > I've changed the methods proposed in KIP [1] to use
> > > > > > > > > > > > Instant.
> > > > > > > > > > > >
> > > > > > > > > > > > > I noticed some recent APIs are> missing (see KIP-328)
> > > > > > > > > > > > > those APIs were just added and have never been
> > > > > > > > > > > > > released...
> > > > > >
> > > > > > you can
> > > > > > > > > >
> > > > > > > > > > just
> > > > > > > > > > > >
> > > > > > > > > > > > replace them.
> > > > > > > > > > > >
> > > > > > > > > > > > I've added new methods to KIP [1].
> > > > > > > > > > > > Not released methods marked for remove.
> > > > > > > > > > > >
> > > > > > > > > > > > > any existing method that's already deprecated, don't
> > > > > > > > > > > > > bother
> > > > > > > > > > > >
> > > > > > > > > > > > transitioning it to Duration.
> > > > > > > > > > > >
> > > > > > > > > > > > Fixed.
> > > > > > > > > > > >
> > > > > > > > > > > > > IllegalArgumentException... we should plan to mention
> > > > > > > > > > > > > this
> > > > > >
> > > > > > in the
> > > > > > > > > > > >
> > > > > > > > > > > > javadoc for those methods.
> > > > > > > > > > > >
> > > > > > > > > > > > Got it.
> > > > > > > > > > > >
> > > > > > > > > > > > > In Stores, windowSize and segmentInterval should also
> > > > > > > > > > > > > be
> > > > > >
> > > > > > durations.
> > > > > > > > > > > >
> > > > > > > > > > > > Fixed.
> > > > > > > > > > > >
> > > > > > > > > > > > > StreamsMetrics, recordLatency ... this one is better
> > > > > > > > > > > > > left
> > > > > >
> > > > > > alone.
> > > > > > > > > > > >
> > > > > > > > > > > > OK. I removed this method from KIP [1].
> > > > > > > > > > > >
> > > > > > > > > > > > Two more questions question about implementation:
> > > > > > > > > > > >
> > > > > > > > > > > > 1. We have serveral methods without parameters.
> > > > > > > > > > > > In java we can't have two methods with parameters with
> > > > > > > > > > > > the
> > > > > >
> > > > > > same name.
> > > > > > > > > > > > It wouldn't compile.
> > > > > > > > > > > > So we have to rename new methods. Please, see suggested
> > > > > >
> > > > > > names and share
> > > > > > > > > > > > your thoughts:
> > > > > > > > > > > >
> > > > > > > > > > > > Windows {
> > > > > > > > > > > > long size() -> Duration windowSize();
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > Window {
> > > > > > > > > > > > long start() -> Instant startTime();
> > > > > > > > > > > > long end() -> Instant endTime();
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > SessionWindows {
> > > > > > > > > > > > long inactivityGap() -> Duration
> > > > > > > > > > > > inactivityGapDuration();
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > TimeWindowedDeserializer {
> > > > > > > > > > > > Long getWindowSize() -> Duration
> > > > > >
> > > > > > getWindowSizeDuration(); // or
> > > > > > > > > >
> > > > > > > > > > just
> > > > > > > > > > > > Duration windowSize();
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > SessionBytesStoreSupplier {
> > > > > > > > > > > > long retentionPeriod() -> Duration
> > > > > >
> > > > > > retentionPeriodDuration();
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > WindowBytesStoreSupplier {
> > > > > > > > > > > > long windowSize() -> Duration windowSizeDuration();
> > > > > > > > > > > > long retentionPeriod() -> Duration
> > > > > >
> > > > > > retentionPeriodDuration();
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > 2. Do we want to use Duration and Instant inside API
> > > > > >
> > > > > > implementations?
> > > > > > > > > > > >
> > > > > > > > > > > > IGNITE-7277: "Durations potentially worsen memory
> > > > > > > > > > > > pressure
> > > > > >
> > > > > > and gc
> > > > > > > > > > > > performance, so internally, we will still use longMs as
> > > > > > > > > > > > the
> > > > > > > > > >
> > > > > > > > > > representation."
> > > > > > > > > > > > IGNITE-7222: Duration used to store retention.
> > > > > > > > > > > >
> > > > > > > > > > > > [1]
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > >
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-358%3A+Migrate+Streams+API+to+Duration+instead+of+long+ms+times
> > > > > > > > > > > > [2]
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > >
> > > > > > https://github.com/apache/kafka/commit/b3771ba22acad7870e38ff7f58820c5b50946787#diff-47289575d3e3e2449f27b3a7b6788e1aR64
> > > > > > > > > > > >
> > > > > > > > > > > > В Пт, 17/08/2018 в 14:46 -0500, John Roesler пишет:
> > > > > > > > > > > > > Hi Nikolay,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for this very nice KIP!
> > > > > > > > > > > > >
> > > > > > > > > > > > > To answer your questions:
> > > > > > > > > > > > > 1. Correct, we should not delete existing methods that
> > > > > >
> > > > > > have been
> > > > > > > > > > > >
> > > > > > > > > > > > released,
> > > > > > > > > > > > > but ...
> > > > > > > > > > > > >
> > > > > > > > > > > > > 2. Yes, we should deprecate the 'long' variants so
> > > > > > > > > > > > > that we
> > > > > >
> > > > > > can drop
> > > > > > > > > >
> > > > > > > > > > them
> > > > > > > > > > > > > later on. Personally, I like to mention which version
> > > > > >
> > > > > > deprecated the
> > > > > > > > > > > >
> > > > > > > > > > > > method
> > > > > > > > > > > > > so everyone can see later on how long it's been
> > > > > >
> > > > > > deprecated, but this
> > > > > > > > > >
> > > > > > > > > > may
> > > > > > > > > > > >
> > > > > > > > > > > > be
> > > > > > > > > > > > > controversial, so let's let other weigh in.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 3. I think you're asking whether it's appropriate to
> > > > > > > > > > > > > drop
> > > > > >
> > > > > > the "Ms"
> > > > > > > > > > > >
> > > > > > > > > > > > suffix,
> > > > > > > > > > > > > and I think yes. So "long inactivityGapMs" would
> > > > > > > > > > > > > become
> > > > > >
> > > > > > "Duration
> > > > > > > > > > > > > inactivityGap".
> > > > > > > > > > > > > In the places where the parameter's name is just
> > > > > >
> > > > > > "duration", I think
> > > > > > > > > >
> > > > > > > > > > we
> > > > > > > > > > > >
> > > > > > > > > > > > can
> > > > > > > > > > > > > pick something more descriptive (I realize it was
> > > > > > > > > > > > > already
> > > > > > > > > >
> > > > > > > > > > "durationMs";
> > > > > > > > > > > > > this is just a good time to improve it).
> > > > > > > > > > > > > Also, you're correct that we shouldn't use a Duration
> > > > > > > > > > > > > to
> > > > > >
> > > > > > represent a
> > > > > > > > > > > >
> > > > > > > > > > > > moment
> > > > > > > > > > > > > in time, like "startTime". The correct choice is
> > > > > > > > > > > > > actually
> > > > > >
> > > > > > "Instant",
> > > > > > > > > >
> > > > > > > > > > not
> > > > > > > > > > > > > "LocalDateTime", though.
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > >
> > > > > > https://stackoverflow.com/questions/32437550/whats-the-difference-between-instant-and-localdatetime
> > > > > > > > > > > > > explains why.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I also had a few notes on the KIP itself:
> > > > > > > > > > > > > 4. You might want to pull trunk again. I noticed some
> > > > > >
> > > > > > recent APIs are
> > > > > > > > > > > > > missing (see KIP-328).
> > > > > > > > > > > > >
> > > > > > > > > > > > > 5. Speaking of KIP-328: those APIs were just added and
> > > > > >
> > > > > > have never
> > > > > > > > > >
> > > > > > > > > > been
> > > > > > > > > > > > > released, so there's no need to deprecate the
> > > > > > > > > > > > > methods, you
> > > > > >
> > > > > > can just
> > > > > > > > > > > >
> > > > > > > > > > > > replace
> > > > > > > > > > > > > them.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 6. For any existing method that's already deprecated,
> > > > > >
> > > > > > don't bother
> > > > > > > > > > > > > transitioning it to Duration. I think the examples I
> > > > > >
> > > > > > noticed were
> > > > > > > > > > > > > deprecated in KIP-328, so you'll see what I'm talking
> > > > > >
> > > > > > about when you
> > > > > > > > > >
> > > > > > > > > > pull
> > > > > > > > > > > > > trunk again.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 7. Any method taking a Duration argument may throw an
> > > > > > > > > > > > > IllegalArgumentException (we choose to convert
> > > > > >
> > > > > > ArithmeticException to
> > > > > > > > > > > > > IllegalArgumentException, as I mentioned in the Jira
> > > > > >
> > > > > > ticket). We
> > > > > > > > > >
> > > > > > > > > > don't
> > > > > > > > > > > >
> > > > > > > > > > > > need
> > > > > > > > > > > > > a "throws" declaration, but we should plan to mention
> > > > > > > > > > > > > this
> > > > > >
> > > > > > in the
> > > > > > > > > >
> > > > > > > > > > javadoc
> > > > > > > > > > > > > for those methods.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 8. In Stores, windowSize and segmentInterval should
> > > > > > > > > > > > > also be
> > > > > > > > > >
> > > > > > > > > > durations.
> > > > > > > > > > > > >
> > > > > > > > > > > > > 9. In StreamsMetrics, recordLatency could be just a
> > > > > >
> > > > > > Duration, but I
> > > > > > > > > > > > > actually think this one is better left alone. IMO,
> > > > > > > > > > > > > it's
> > > > > >
> > > > > > more effort
> > > > > > > > > >
> > > > > > > > > > for
> > > > > > > > > > > > > little gain to require users to construct a Duration
> > > > > >
> > > > > > before they
> > > > > > > > > >
> > > > > > > > > > call the
> > > > > > > > > > > > > method, since they vary likely call
> > > > > >
> > > > > > System.currentTimeNanos before
> > > > > > > > > >
> > > > > > > > > > and
> > > > > > > > > > > > > after the code in question.
> > > > > > > > > > > > >
> > > > > > > > > > > > > These are quite a few notes, but they're all minor.
> > > > > >
> > > > > > Overall the KIP
> > > > > > > > > >
> > > > > > > > > > looks
> > > > > > > > > > > > > really good to me. Thanks for picking this up!
> > > > > > > > > > > > > -John
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Aug 16, 2018 at 9:55 AM Nikolay Izhikov <
> > > > > >
> > > > > > nizhi...@apache.org
> > > > > > > > > > > >
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Hello, Kafka developers.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I would like to start a discussion of KIP-358 [1].
> > > > > > > > > > > > > > It based on a ticket KAFKA-7277 [2].
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I crawled through Stream API and made my
> > > > > > > > > > > > > > suggestions for
> > > > > >
> > > > > > API
> > > > > > > > > >
> > > > > > > > > > changes.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I have several questions about changes.
> > > > > > > > > > > > > > Please, share your comments:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 1. I propose do not remove existing API methods with
> > > > > >
> > > > > > long ms
> > > > > > > > > > > >
> > > > > > > > > > > > parameters.
> > > > > > > > > > > > > > Is it correct?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 2. Should we mark existing methods as deprecated?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 3. Suggested changes in ticket description are `long
> > > > > >
> > > > > > durationMs` to
> > > > > > > > > > > > > > `Duration duration` and similar.
> > > > > > > > > > > > > > I suggest to change 'long startTimeMs` to
> > > > > > > > > > > > > > `LocalDateTime
> > > > > >
> > > > > > startTime`
> > > > > > > > > > > >
> > > > > > > > > > > > also.
> > > > > > > > > > > > > > Should we do it?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Please, note, it very first KIP for me, so tell me
> > > > > > > > > > > > > > if I
> > > > > >
> > > > > > miss
> > > > > > > > > >
> > > > > > > > > > something
> > > > > > > > > > > > > > obvious for experienced Kafka developers.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > [1]
> > > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > >
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-358%3A+Migrate+Streams+API+to+Duration+instead+of+long+ms+times
> > > > > > > > > > > > > > [2] https://issues.apache.org/jira/browse/KAFKA-7277
> > > > > > >
> > > > > > >
> > > > >
> > > > >
signature.asc
Description: This is a digitally signed message part
