kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nikolay Izhikov <nizhi...@apache.org>
Subject Re: [DISCUSS] KIP-358: Migrate Streams API to Duration instead of long ms times
Date Sun, 16 Sep 2018 18:37:32 GMT
Dear commiters, 

I got two binding +1 in [VOTE] thread for this KIP [1].
I still need one more.

Please, take a look at KIP.

[1] https://lists.apache.org/thread.html/e976352e7e42d459091ee66ac790b6a0de7064eac0c57760d50c983b@%3Cdev.kafka.apache.org%3E

В Чт, 13/09/2018 в 19:33 +0300, Nikolay Izhikov пишет:
> Fixed. 
> 
> Thanks, for help!
> 
> Please, take a look and vote.
> 
> В Чт, 13/09/2018 в 08:40 -0700, Matthias J. Sax пишет:
> > No need to start a new voting thread :)
> > 
> > For the KIP update, I think it should be:
> > 
> > > ReadOnlyWindowStore<K, V> {
> > >     //Deprecated methods.
> > >     WindowStoreIterator<V> fetch(K key, long timeFrom, long timeTo);
> > >     KeyValueIterator<Windowed<K>, V> fetch(K from, K to, long timeFrom, long timeTo);
> > >     KeyValueIterator<Windowed<K>, V> fetchAll(long timeFrom, long timeTo);
> > >  
> > >     //New methods.
> > >     WindowStoreIterator<V> fetch(K key, Instant from, Duration duration) throws IllegalArgumentException;
> > >     KeyValueIterator<Windowed<K>, V> fetch(K from, K to, Instant from, Duration duration) throws IllegalArgumentException;
> > >     KeyValueIterator<Windowed<K>, V> fetchAll(Instant from, Duration duration) throws IllegalArgumentException;
> > > }
> > >  
> > >  
> > > WindowStore {
> > >     //New methods.
> > >     WindowStoreIterator<V> fetch(K key, long timeFrom, long timeTo);
> > >     KeyValueIterator<Windowed<K>, V> fetch(K from, K to, long timeFrom, long timeTo);
> > >     KeyValueIterator<Windowed<K>, V> fetchAll(long timeFrom, long timeTo);
> > > }
> > 
> > Ie, long-versions are replaced with Instant/Duration in
> > `ReadOnlyWindowStore`, and `long` method are added in `WindowStore` --
> > this way, we effectively "move" the long-versions from
> > `ReadOnlyWindowStore` to `WindowStore`.
> > 
> > -Matthias
> > 
> > On 9/13/18 8:08 AM, Nikolay Izhikov wrote:
> > > Hello, Matthias.
> > > 
> > > > I like the KIP as-is. Feel free to start a VOTE thread.
> > > 
> > > 
> > > I'm already started one [1].
> > > Can you vote in it or I should create a new one?
> > > 
> > > 
> > > I've updated KIP.
> > > This has been changed:
> > > 
> > > ReadOnlyWindowStore<K, V> {
> > >     //Deprecated methods.
> > >     WindowStoreIterator<V> fetch(K key, long timeFrom, long timeTo);
> > >     KeyValueIterator<Windowed<K>, V> fetch(K from, K to, long timeFrom, long timeTo);
> > >     KeyValueIterator<Windowed<K>, V> fetchAll(long timeFrom, long timeTo);
> > > }
> > > 
> > > WindowStore {
> > > 	//New methods.
> > >     WindowStoreIterator<V> fetch(K key, Instant from, Duration duration) throws IllegalArgumentException;
> > >     KeyValueIterator<Windowed<K>, V> fetch(K from, K to, Instant from, Duration duration) throws IllegalArgumentException;
> > >     KeyValueIterator<Windowed<K>, V> fetchAll(Instant from, Duration duration) throws IllegalArgumentException;
> > > }
> > > 
> > > [1] https://lists.apache.org/thread.html/e976352e7e42d459091ee66ac790b6a0de7064eac0c57760d50c983b@%3Cdev.kafka.apache.org%3E
> > > 
> > > В Ср, 12/09/2018 в 15:46 -0700, Matthias J. Sax пишет:
> > > > Great!
> > > > 
> > > > I did not double check the ReadOnlySessionStore interface before, and
> > > > just assumed it would take a timestamp, too. My bad.
> > > > 
> > > > Please update the KIP for ReadOnlyWindowStore and WindowStore.
> > > > 
> > > > I like the KIP as-is. Feel free to start a VOTE thread. Even if there
> > > > might be minor follow up comments, we can vote in parallel.
> > > > 
> > > > 
> > > > -Matthias
> > > > 
> > > > On 9/12/18 1:06 PM, John Roesler wrote:
> > > > > Hi Nikolay,
> > > > > 
> > > > > Yes, the changes we discussed for ReadOnlyXxxStore and XxxStore should be
> > > > > in this KIP.
> > > > > 
> > > > > And you're right, it seems like ReadOnlySessionStore is not necessary to
> > > > > touch, since it doesn't reference any `long` timestamps.
> > > > > 
> > > > > Thanks,
> > > > > -John
> > > > > 
> > > > > On Wed, Sep 12, 2018 at 4:36 AM Nikolay Izhikov <nizhikov@apache.org> wrote:
> > > > > 
> > > > > > Hello, Matthias.
> > > > > > 
> > > > > > > His proposal is, to deprecate existing methods on `ReadOnlyWindowStore`>
> > > > > > 
> > > > > > and `ReadOnlySessionStore` and add them to `WindowStore` and> `SessionStore`
> > > > > > > Does this make sense?
> > > > > > 
> > > > > > You both are experienced Kafka developers, so yes, it does make a sense to
> > > > > > me :).
> > > > > > Do we want to make this change in KIP-358 or it required another KIP?
> > > > > > 
> > > > > > > Btw: the KIP misses `ReadOnlySessionStore` atm.
> > > > > > 
> > > > > > Sorry, but I don't understand you.
> > > > > > As far as I can see, there is only 2 methods in `ReadOnlySessionStore`.
> > > > > > Which method should be migrated to Duration?
> > > > > > 
> > > > > > 
> > > > > > https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/state/ReadOnlySessionStore.java
> > > > > > 
> > > > > > В Вт, 11/09/2018 в 09:21 -0700, Matthias J. Sax пишет:
> > > > > > > I talked to John offline about his last suggestions, that I originally
> > > > > > > did not fully understand.
> > > > > > > 
> > > > > > > His proposal is, to deprecate existing methods on `ReadOnlyWindowStore`
> > > > > > > and `ReadOnlySessionStore` and add them to `WindowStore` and
> > > > > > > `SessionStore` (note, all singular -- not to be confused with classes
> > > > > > > names plural).
> > > > > > > 
> > > > > > > Btw: the KIP misses `ReadOnlySessionStore` atm.
> > > > > > > 
> > > > > > > The argument is, that the `ReadOnlyXxxStore` interfaces are only exposed
> > > > > > > via Interactive Queries feature and for this part, using `long` is
> > > > > > > undesired. However, for a `Processor` that reads/writes stores on the
> > > > > > > hot code path, we would like to avoid the object creation overhead and
> > > > > > > stay with `long`. Note, that a `Processor` would use the "read-write"
> > > > > > > interfaces and thus, we can add the more efficient read methods using
> > > > > > > `long` there.
> > > > > > > 
> > > > > > > Does this make sense?
> > > > > > > 
> > > > > > > 
> > > > > > > -Matthias
> > > > > > > 
> > > > > > > On 9/11/18 12:20 AM, Nikolay Izhikov wrote:
> > > > > > > > Hello, Guozhang, Bill.
> > > > > > > > 
> > > > > > > > > 1) I'd suggest keeping `Punctuator#punctuate(long timestamp)` as is
> > > > > > > > 
> > > > > > > > I am agree with you.
> > > > > > > > Currently, `Punctuator` edits are not included in KIP.
> > > > > > > > 
> > > > > > > > > 2) I'm fine with keeping KeyValueStore extending
> > > > > > 
> > > > > > ReadOnlyKeyValueStore
> > > > > > > > 
> > > > > > > > Great, currently, there is no suggested API change in `KeyValueStore`
> > > > > > 
> > > > > > or `ReadOnlyKeyValueStore`.
> > > > > > > > 
> > > > > > > > Seems, you agree with all KIP details.
> > > > > > > > Can you vote, please? [1]
> > > > > > > > 
> > > > > > > > [1]
> > > > > > 
> > > > > > https://lists.apache.org/thread.html/e976352e7e42d459091ee66ac790b6a0de7064eac0c57760d50c983b@%3Cdev.kafka.apache.org%3E
> > > > > > > > 
> > > > > > > > 
> > > > > > > > В Пн, 10/09/2018 в 19:49 -0400, Bill Bejeck пишет:
> > > > > > > > > Hi Nikolay,
> > > > > > > > > 
> > > > > > > > > I'm a +1 to points 1 and 2 above from Guozhang.
> > > > > > > > > 
> > > > > > > > > Thanks,
> > > > > > > > > Bill
> > > > > > > > > 
> > > > > > > > > On Mon, Sep 10, 2018 at 6:58 PM Guozhang Wang <wangguoz@gmail.com>
> > > > > > 
> > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > > Hello Nikolay,
> > > > > > > > > > 
> > > > > > > > > > Thanks for picking this up! Just sharing my two cents:
> > > > > > > > > > 
> > > > > > > > > > 1) I'd suggest keeping `Punctuator#punctuate(long timestamp)` as
> > > > > > 
> > > > > > is since
> > > > > > > > > > comparing with other places where we are replacing with Duration
> > > > > > 
> > > > > > and
> > > > > > > > > > Instant, this is not a user specified value as part of the DSL but
> > > > > > 
> > > > > > rather a
> > > > > > > > > > passed-in parameter, plus with high punctuation frequency creating
> > > > > > 
> > > > > > a new
> > > > > > > > > > instance of Instant may be costly.
> > > > > > > > > > 
> > > > > > > > > > 2) I'm fine with keeping KeyValueStore extending
> > > > > > 
> > > > > > ReadOnlyKeyValueStore with
> > > > > > > > > > APIs of `long` as well as inheriting APIs of `Duration`.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Guozhang
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > On Mon, Sep 10, 2018 at 11:11 AM, Nikolay Izhikov <
> > > > > > 
> > > > > > nizhikov@apache.org>
> > > > > > > > > > wrote:
> > > > > > > > > > 
> > > > > > > > > > > Hello, Matthias.
> > > > > > > > > > > 
> > > > > > > > > > > > (4) While I agree that we might want to deprecate it, I am not
> > > > > > 
> > > > > > sure if
> > > > > > > > > > > 
> > > > > > > > > > > this should be part of this KIP?
> > > > > > > > > > > > Seems to be unrelated?
> > > > > > > > > > > > Should this have been part of KIP-319?
> > > > > > > > > > > > If yes, we might still want to updated this other KIP? WDYT?
> > > > > > > > > > > 
> > > > > > > > > > > OK, I removed this deprecation from this KIP.
> > > > > > > > > > > 
> > > > > > > > > > > Please, tell me, is there anything else that should be improved
> > > > > > 
> > > > > > to make
> > > > > > > > > > > this KIP ready to be implemented.
> > > > > > > > > > > 
> > > > > > > > > > > В Пт, 07/09/2018 в 17:06 -0700, Matthias J. Sax пишет:
> > > > > > > > > > > > (1) Sounds good to me, to just use IllegalArgumentException
> > > > > > 
> > > > > > for both --
> > > > > > > > > > > > and thanks for pointing out that Duration can be negative and
> > > > > > 
> > > > > > we need
> > > > > > > > > > 
> > > > > > > > > > to
> > > > > > > > > > > > check for this. For the KIP, it would be nice to add to all
> > > > > > 
> > > > > > methods
> > > > > > > > > > 
> > > > > > > > > > than
> > > > > > > > > > > > (even if we don't do it in the code but only document in
> > > > > > 
> > > > > > JavaDocs).
> > > > > > > > > > > > 
> > > > > > > > > > > > (2) I would argue for a new single method interface. Not sure
> > > > > > 
> > > > > > about the
> > > > > > > > > > > > name though.
> > > > > > > > > > > > 
> > > > > > > > > > > > (3) Even if `#fetch(K, K, long, long)` and `#fetchAll(long,
> > > > > > 
> > > > > > long)` is
> > > > > > > > > > > > _currently_ not used internally, I would still argue they are
> > > > > > 
> > > > > > both dual
> > > > > > > > > > > > use -- we might all a new DSL operator at any point that uses
> > > > > > 
> > > > > > those
> > > > > > > > > > > > methods. Thus to be "future prove" I would consider them dual
> > > > > > 
> > > > > > use.
> > > > > > > > > > > > 
> > > > > > > > > > > > > Since the ReadOnlyWindowStore is only used by IQ,
> > > > > > > > > > > > 
> > > > > > > > > > > > This contradicts your other statement:
> > > > > > > > > > > > 
> > > > > > > > > > > > > org.apache.kafka.streams.state.ReadOnlyWindowStore#fetch(K,
> > > > > > 
> > > > > > long) is
> > > > > > > > > > > 
> > > > > > > > > > > used
> > > > > > > > > > > > > in KStreamWindowAggregate.
> > > > > > > > > > > > 
> > > > > > > > > > > > Or do you suggest to move `fetch(K, long)` from
> > > > > > 
> > > > > > `ReadOnlyWindowStore`
> > > > > > > > > > 
> > > > > > > > > > to
> > > > > > > > > > > > `WindowStore` ? This would not make sense IMHO, as
> > > > > > 
> > > > > > `WindowStore extends
> > > > > > > > > > > > ReadOnlyWindowStore` and thus, we would loose this method for
> > > > > > 
> > > > > > IQ.
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > (4) While I agree that we might want to deprecate it, I am not
> > > > > > 
> > > > > > sure if
> > > > > > > > > > > > this should be part of this KIP? Seems to be unrelated? Should
> > > > > > 
> > > > > > this
> > > > > > > > > > 
> > > > > > > > > > have
> > > > > > > > > > > > been part of KIP-319? If yes, we might still want to updated
> > > > > > 
> > > > > > this other
> > > > > > > > > > > > KIP? WDYT?
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > -Matthias
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > On 9/7/18 12:09 PM, John Roesler wrote:
> > > > > > > > > > > > > Hey all,
> > > > > > > > > > > > > 
> > > > > > > > > > > > > (1): Duration can be negative, just like long. We need to
> > > > > > 
> > > > > > enforce any
> > > > > > > > > > > > > bounds that we currently enforce. We don't need the `throws`
> > > > > > > > > > > 
> > > > > > > > > > > declaration
> > > > > > > > > > > > > for runtime exceptions, but the potential
> > > > > > 
> > > > > > IllegalArgumentException
> > > > > > > > > > > 
> > > > > > > > > > > should
> > > > > > > > > > > > > be documented in the javadoc for these methods. I still feel
> > > > > > 
> > > > > > that
> > > > > > > > > > > 
> > > > > > > > > > > surfacing
> > > > > > > > > > > > > the ArithmeticException directly would not be a great
> > > > > > 
> > > > > > experience, so
> > > > > > > > > > 
> > > > > > > > > > I
> > > > > > > > > > > > > still advocate for wrapping it in an
> > > > > > 
> > > > > > IllegalArgumentException that
> > > > > > > > > > > 
> > > > > > > > > > > explains
> > > > > > > > > > > > > our upper bound for Duration is "max-long number of
> > > > > > 
> > > > > > milliseconds"
> > > > > > > > > > > > > 
> > > > > > > > > > > > > (2): I agree with your performance intuition. I don't think
> > > > > > 
> > > > > > creating
> > > > > > > > > > > 
> > > > > > > > > > > one
> > > > > > > > > > > > > object per call to punctuate is going to substantially
> > > > > > 
> > > > > > affect the
> > > > > > > > > > > > > performance.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I think the deeper problem with Punctuator is that it's
> > > > > > 
> > > > > > currently a
> > > > > > > > > > 
> > > > > > > > > > SAM
> > > > > > > > > > > > > interface. If we add a new method to it, we break the source
> > > > > > 
> > > > > > code of
> > > > > > > > > > > 
> > > > > > > > > > > anyone
> > > > > > > > > > > > > passing a function. We can add the new method with a default
> > > > > > > > > > > > > implementation, as Nikolay suggested, but then you get into
> > > > > > 
> > > > > > figuring
> > > > > > > > > > > 
> > > > > > > > > > > out
> > > > > > > > > > > > > which one to default, and no one's happy. Alternatively, we
> > > > > > 
> > > > > > can just
> > > > > > > > > > > 
> > > > > > > > > > > make a
> > > > > > > > > > > > > brand new interface that is still a single method (but an
> > > > > > 
> > > > > > Instant)
> > > > > > > > > > 
> > > > > > > > > > and
> > > > > > > > > > > add
> > > > > > > > > > > > > the appropriate overloads and deprecate the old ones.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > (3): I disagree. I think only two methods are dual use, and
> > > > > > 
> > > > > > we should
> > > > > > > > > > > > > separate the internal from external usages. The internal
> > > > > > 
> > > > > > usage should
> > > > > > > > > > > 
> > > > > > > > > > > be
> > > > > > > > > > > > > added to WindowStore.
> > > > > > > > > > > > > org.apache.kafka.streams.state.ReadOnlyWindowStore#fetch(K,
> > > > > > 
> > > > > > long) is
> > > > > > > > > > > 
> > > > > > > > > > > used
> > > > > > > > > > > > > in KStreamWindowAggregate.
> > > > > > > > > > > > > org.apache.kafka.streams.state.ReadOnlyWindowStore#fetch(K,
> > > > > > 
> > > > > > long,
> > > > > > > > > > > 
> > > > > > > > > > > long) is
> > > > > > > > > > > > > used in KStreamKStreamJoin.
> > > > > > > > > > > > > Both of these usages are as WindowStore, so adding these
> > > > > > 
> > > > > > interfaces
> > > > > > > > > > 
> > > > > > > > > > to
> > > > > > > > > > > > > WindowStore would be transparent.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > org.apache.kafka.streams.state.ReadOnlyWindowStore#fetch(K,
> > > > > > 
> > > > > > K, long,
> > > > > > > > > > > 
> > > > > > > > > > > long)
> > > > > > > > > > > > > is only used for IQ
> > > > > > > > > > > > > 
> > > > > > 
> > > > > > org.apache.kafka.streams.state.ReadOnlyWindowStore#fetchAll(long,
> > > > > > > > > > > 
> > > > > > > > > > > long) is
> > > > > > > > > > > > > only used for IQ
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Since the ReadOnlyWindowStore is only used by IQ, we can
> > > > > > 
> > > > > > safely say
> > > > > > > > > > > 
> > > > > > > > > > > that
> > > > > > > > > > > > > *all* of its methods are external use and can be deprecated
> > > > > > 
> > > > > > and
> > > > > > > > > > > 
> > > > > > > > > > > replaced.
> > > > > > > > > > > > > The first two usages I noted are WindowStore usages, not
> > > > > > > > > > > > > ReadOnlyWindowStores, and WindowStore is only used
> > > > > > 
> > > > > > *internally*, so
> > > > > > > > > > > 
> > > > > > > > > > > it's
> > > > > > > > > > > > > free to offer `long` methods if needed for performance
> > > > > > 
> > > > > > reasons.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Does this make sense? The same reasoning extends to the
> > > > > > 
> > > > > > other stores.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > (4) Yes, that was my suggestion. I'm not sure if anyone is
> > > > > > 
> > > > > > actually
> > > > > > > > > > > 
> > > > > > > > > > > using
> > > > > > > > > > > > > this variant, so it seemed like a good time to just
> > > > > > 
> > > > > > deprecate it and
> > > > > > > > > > > 
> > > > > > > > > > > see.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Thoughts?
> > > > > > > > > > > > > -John
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > On Fri, Sep 7, 2018 at 10:21 AM Nikolay Izhikov <
> > > > > > 
> > > > > > nizhikov@apache.org
> > > > > > > > > > > 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > Hello, Matthias.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Thanks, for feedback.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > (1) Some methods declare `throws
> > > > > > 
> > > > > > IllegalArgumentException`,
> > > > > > > > > > 
> > > > > > > > > > others>
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > don't.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > `duration.toMillis()` can throw ArithmeticException.
> > > > > > > > > > > > > > It can happen if overflow occurs during conversion.
> > > > > > > > > > > > > > Please, see source of jdk method Duration#toMillis.
> > > > > > > > > > > > > > Task author suggest to wrap it to IllegalArgumentException.
> > > > > > > > > > > > > > I think we should add `throws IllegalArgumentException`
> > > > > > 
> > > > > > for all
> > > > > > > > > > > 
> > > > > > > > > > > method
> > > > > > > > > > > > > > with Duration parameter.
> > > > > > > > > > > > > > (I updated KIP with this throws)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > What do you think?
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > (3) ReadOnlyWindowStore: All three methods are dual use
> > > > > > 
> > > > > > and I
> > > > > > > > > > > 
> > > > > > > > > > > think we
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > should not deprecate them.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > This is my typo, already fixed.
> > > > > > > > > > > > > > I propose to add new methods to `ReadOnlyWindowStore`.
> > > > > > > > > > > > > > No methods will become deprecated.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > (4) Stores: 3 methods are listed as deprecated but only
> > > > > > 
> > > > > > 2 new
> > > > > > > > > > > 
> > > > > > > > > > > methods
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > are added.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > My proposal based on John Roesler mail [1]:
> > > > > > > > > > > > > > "10. Stores: I think we can just deprecate without
> > > > > > 
> > > > > > replacement the
> > > > > > > > > > > 
> > > > > > > > > > > method
> > > > > > > > > > > > > > that takes `segmentInterval`."
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Is it wrong?
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > [1]
> > > > > > > > > > 
> > > > > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg91348.html
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > В Чт, 06/09/2018 в 21:04 -0700, Matthias J. Sax пишет:
> > > > > > > > > > > > > > > Thanks for updating the KIP!
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Couple of minor follow ups:
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > (1) Some methods declare `throws
> > > > > > 
> > > > > > IllegalArgumentException`,
> > > > > > > > > > 
> > > > > > > > > > others
> > > > > > > > > > > > > > > don't. It's runtime exception and thus it's not required
> > > > > > 
> > > > > > to
> > > > > > > > > > > 
> > > > > > > > > > > declare it
> > > > > > > > > > > > > > > -- it just looks inconsistent in the KIP and maybe it's
> > > > > > > > > > > 
> > > > > > > > > > > inconsistent in
> > > > > > > > > > > > > > > the code, too. I am not sure if it is possible to
> > > > > > 
> > > > > > provide a
> > > > > > > > > > > 
> > > > > > > > > > > negative
> > > > > > > > > > > > > > > Duration? If not, we would not need to check the
> > > > > > 
> > > > > > provided value
> > > > > > > > > > > 
> > > > > > > > > > > and can
> > > > > > > > > > > > > > > remove the declaration.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > (2) About punctuations: I still think, it would be ok to
> > > > > > 
> > > > > > change
> > > > > > > > > > 
> > > > > > > > > > the
> > > > > > > > > > > > > > > callback from `long` to `Instance` -- even if it is
> > > > > > 
> > > > > > possible to
> > > > > > > > > > > 
> > > > > > > > > > > register
> > > > > > > > > > > > > > > a punctuation on a ms-basis, in practice many people used
> > > > > > > > > > > 
> > > > > > > > > > > schedules in
> > > > > > > > > > > > > > > the range of seconds or larger. Thus, I don't think
> > > > > > 
> > > > > > there will
> > > > > > > > > > 
> > > > > > > > > > be a
> > > > > > > > > > > > > > > performance penalty. Of course, we can still revisit
> > > > > > 
> > > > > > this later,
> > > > > > > > > > > 
> > > > > > > > > > > too.
> > > > > > > > > > > > > > > John and Bill, you did not comment on this. Would also
> > > > > > 
> > > > > > be good to
> > > > > > > > > > > 
> > > > > > > > > > > get
> > > > > > > > > > > > > > > feedback from Guozhang about this.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > (3) ReadOnlyWindowStore: All three methods are dual use
> > > > > > 
> > > > > > and I
> > > > > > > > > > > 
> > > > > > > > > > > think we
> > > > > > > > > > > > > > > should not deprecate them. However, we can add the new
> > > > > > 
> > > > > > proposed
> > > > > > > > > > > 
> > > > > > > > > > > methods
> > > > > > > > > > > > > > > in parallel -- the names can be the same without
> > > > > > 
> > > > > > conflict as the
> > > > > > > > > > > > > > > parameter lists are different. (Or did you just forget
> > > > > > 
> > > > > > to remove
> > > > > > > > > > > 
> > > > > > > > > > > the
> > > > > > > > > > > > > > > comment line?)
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > (4) Stores: 3 methods are listed as deprecated but only
> > > > > > 
> > > > > > 2 new
> > > > > > > > > > > 
> > > > > > > > > > > methods
> > > > > > > > > > > > > > > are added. Maybe this was discussed already, but I can't
> > > > > > 
> > > > > > recall
> > > > > > > > > > > 
> > > > > > > > > > > why? Can
> > > > > > > > > > > > > > > you elaborate? Or should this deprecation be actually be
> > > > > > 
> > > > > > part of
> > > > > > > > > > > 
> > > > > > > > > > > KIP-328
> > > > > > > > > > > > > > > (\cc John)?
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > -Matthias
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > ps: there are many KIPs in-flight in parallel, and it
> > > > > > 
> > > > > > takes some
> > > > > > > > > > > 
> > > > > > > > > > > time to
> > > > > > > > > > > > > > > get around. Please be patient :)
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > On 9/5/18 12:25 AM, Nikolay Izhikov wrote:
> > > > > > > > > > > > > > > > Hello, Guys.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > I've started a VOTE [1], but seems commiters have no
> > > > > > 
> > > > > > chance to
> > > > > > > > > > > 
> > > > > > > > > > > look at
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > KIP for now.
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > Can you tell me, is it OK?
> > > > > > > > > > > > > > > > Should I wait for feedback? For how long?
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > Or something in KIP should be improved before voting?
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > [1]
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > 
> > > > > > https://lists.apache.org/thread.html/e976352e7e42d459091ee66ac790b6
> > > > > > > > > > > a0de7064eac0c57760d50c983b@%3Cdev.kafka.apache.org%3E
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > В Пт, 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 <
> > > > > > > > > > > 
> > > > > > > > > > > john@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 <
> > > > > > > > > > > 
> > > > > > > > > > > john@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 <
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > nizhikov@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
> > > > > > > > > > > <
> > > > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > > > nizhikov@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 <
> > > > > > > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > > > > > > nizhikov@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
> > > > > > > > > > > > > > > > > > > > > 
Mime
View raw message