Thanks +1 (binding) On Sun, 16 Sep 2018 at 19:37 Nikolay Izhikov <nizhi...@apache.org> wrote:
> 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 < > nizhi...@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 < > wangg...@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 < > > > > > > > > > > > > > > nizhi...@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 < > > > > > > > > > > > > > > nizhi...@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 < > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > >
