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 >>>>>>>>>> >>>>>>>>>> I missed >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> any): >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> - Windows#windowsFor() >>>>>>>>>>>>>>>>>> - Window#start() >>>>>>>>>>>>>>>>>> - Window#end() >>>>>>>>>>>>>>>>>> - JoinWindows#windowFor() >>>>>>>>>>>>>>>>>> - SessionWindows#inactivitiyGap() >>>>>>>>>>>>>>>>>> - TimeWindows#windowFor() >>>>>>>>>>>>>>>>>> - UnlimitedWindows#windowFor() >>>>>>>>>>>>>>>>>> - ProcessorContext#schedule() >>>>>>>>>>>>>>>>>> - ReadOnlyWindowStore#fetch() (2x) and >> #fetchAll() >>>>>>>>>>>>>>>>>> - SessionStore#findSessions() (2x) >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> maybe >>>>>>>>>>>>>>>>>> - >> TimeWindowedDeserializer#getWindowSize() (it's >>>>>>> >>>>>>> unused >>>>>>>>>> >>>>>>>>>> atm, but I >>>>>>>>>>>>>>>>>> could imagine that it might be use on the >> hot code >>>>>>> >>>>>>> path in >>>>>>>>>> >>>>>>>>>> the furture) >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> So methods have "dual" use and might be >> called >>>>>>> >>>>>>> externally >>>>>>>>>> >>>>>>>>>> and >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> internally: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> - Window#start() >>>>>>>>>>>>>>>>>> - Window#end() >>>>>>>>>>>>>>>>>> - ReadOnlyWindowStore#fetch() (2x) and >> #fetchAll() >>>>>>>>>>>>>>>>>> - SessionStore#findSessions() (2x) >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Thus, it might make sense to keep old and >> just add >>>>>>> >>>>>>> new >>>>>>>>>> >>>>>>>>>> ones? End users >>>>>>>>>>>>>>>>>> can use the "nicer" new ones, while we can >> still >>>>>> >>>>>> use >>>>>>> the >>>>>>>>>> >>>>>>>>>> existing ones >>>>>>>>>>>>>>>>>> internally? Not sure if it would be >> possible to >>>>>> >>>>>> keep >>>>>>> the >>>>>>>>>> >>>>>>>>>> old ones >>>>>>>>>>>>>>>>>> without exposing them as public API? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Let me know what you think about this. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On 8/21/18 11:41 PM, Nikolay Izhikov wrote: >>>>>>>>>>>>>>>>>>> Dear, commiters. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Please, pay attention to this KIP and >> share your >>>>>>> >>>>>>> opinion. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> В Вт, 21/08/2018 в 11:14 -0500, John >> Roesler >>>>>> >>>>>> пишет: >>>>>>>>>>>>>>>>>>>> I'll solicit more reviews. Let's get >> at least >>>>>> >>>>>> one >>>>>>>>>> >>>>>>>>>> committer to >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> chime in >>>>>>>>>>>>>>>>>>>> before we start a vote (since we need >> their >>>>>>> >>>>>>> approval >>>>>>>>>> >>>>>>>>>> anyway). >>>>>>>>>>>>>>>>>>>> -John >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> On Mon, Aug 20, 2018 at 12:39 PM >> Nikolay >>>>>> >>>>>> Izhikov >>>>>>> < >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> nizhi...@apache.org> >>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Hello, Ted. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Thanks for the comment. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> I've edit KIP and change proposal to >>>>>>> >>>>>>> `windowSize`. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Guys, any other comments? >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> В Вс, 19/08/2018 в 14:57 -0700, Ted >> Yu пишет: >>>>>>>>>>>>>>>>>>>>>> bq. // or just Duration >> windowSize(); >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> +1 to the above choice. >>>>>>>>>>>>>>>>>>>>>> The duration is obvious from the >> return >>>>>>> >>>>>>> type. For >>>>>>>>>> >>>>>>>>>> getter >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> methods, we >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> don't >>>>>>>>>>>>>>>>>>>>>> use get as prefix (as least for >> new code). >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Cheers >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> On Sun, Aug 19, 2018 at 8:03 AM >> Nikolay >>>>>>> >>>>>>> Izhikov < >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> nizhi...@apache.org> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Hello, John. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Thank you very much for your >> feedback! >>>>>>>>>>>>>>>>>>>>>>> I've addressed all your comments. >>>>>>>>>>>>>>>>>>>>>>> Please, see my answers and let >> my know is >>>>>>>>>> >>>>>>>>>> anything in KIP >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> [1] needs to >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> be >>>>>>>>>>>>>>>>>>>>>>> improved. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> The correct choice is actually >>>>>>> >>>>>>> "Instant", not> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> "LocalDateTime" >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> I've changed the methods >> proposed in KIP >>>>>>> >>>>>>> [1] to >>>>>>>>>> >>>>>>>>>> use Instant. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> I noticed some recent APIs >> are> missing >>>>>>> >>>>>>> (see >>>>>>>>>> >>>>>>>>>> KIP-328) >>>>>>>>>>>>>>>>>>>>>>>> those APIs were just added and >> have >>>>>>> >>>>>>> never been >>>>>>>>>> >>>>>>>>>> released... >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> you can >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> just >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> replace them. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> I've added new methods to KIP >> [1]. >>>>>>>>>>>>>>>>>>>>>>> Not released methods marked for >> remove. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> any existing method that's >> already >>>>>>> >>>>>>> deprecated, >>>>>>>>>> >>>>>>>>>> don't bother >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> transitioning it to Duration. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Fixed. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> IllegalArgumentException... we >> should >>>>>>> >>>>>>> plan to >>>>>>>>>> >>>>>>>>>> mention this >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> in the >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> javadoc for those methods. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Got it. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> In Stores, windowSize and >>>>>> >>>>>> segmentInterval >>>>>>>>>> >>>>>>>>>> should also be >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> durations. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Fixed. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> StreamsMetrics, recordLatency >> ... this >>>>>>> >>>>>>> one is >>>>>>>>>> >>>>>>>>>> better left >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> alone. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> OK. I removed this method from >> KIP [1]. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Two more questions question about >>>>>>> >>>>>>> implementation: >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> 1. We have serveral methods >> without >>>>>>> >>>>>>> parameters. >>>>>>>>>>>>>>>>>>>>>>> In java we can't have two >> methods with >>>>>>>>>> >>>>>>>>>> parameters with the >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> same name. >>>>>>>>>>>>>>>>>>>>>>> It wouldn't compile. >>>>>>>>>>>>>>>>>>>>>>> So we have to rename new >> methods. Please, >>>>>>> >>>>>>> see >>>>>>>>>> >>>>>>>>>> suggested >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> names and share >>>>>>>>>>>>>>>>>>>>>>> your thoughts: >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Windows { >>>>>>>>>>>>>>>>>>>>>>> long size() -> Duration >> windowSize(); >>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Window { >>>>>>>>>>>>>>>>>>>>>>> long start() -> Instant >> startTime(); >>>>>>>>>>>>>>>>>>>>>>> long end() -> Instant >> endTime(); >>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> SessionWindows { >>>>>>>>>>>>>>>>>>>>>>> long inactivityGap() -> >> Duration >>>>>>>>>> >>>>>>>>>> inactivityGapDuration(); >>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> TimeWindowedDeserializer { >>>>>>>>>>>>>>>>>>>>>>> Long getWindowSize() -> >> Duration >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> getWindowSizeDuration(); // or >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> just >>>>>>>>>>>>>>>>>>>>>>> Duration windowSize(); >>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> SessionBytesStoreSupplier { >>>>>>>>>>>>>>>>>>>>>>> long retentionPeriod() -> >> Duration >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> retentionPeriodDuration(); >>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> WindowBytesStoreSupplier { >>>>>>>>>>>>>>>>>>>>>>> long windowSize() -> Duration >>>>>>>>>> >>>>>>>>>> windowSizeDuration(); >>>>>>>>>>>>>>>>>>>>>>> long retentionPeriod() -> >> Duration >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> retentionPeriodDuration(); >>>>>>>>>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> 2. Do we want to use Duration >> and Instant >>>>>>> >>>>>>> inside >>>>>>>>>> >>>>>>>>>> API >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> implementations? >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> IGNITE-7277: "Durations >> potentially >>>>>> >>>>>> worsen >>>>>>>>>> >>>>>>>>>> memory pressure >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> and gc >>>>>>>>>>>>>>>>>>>>>>> performance, so internally, we >> will still >>>>>>> >>>>>>> use >>>>>>>>>> >>>>>>>>>> longMs as the >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> representation." >>>>>>>>>>>>>>>>>>>>>>> IGNITE-7222: Duration used to >> store >>>>>>> >>>>>>> retention. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> [1] >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- >>>>>>> >>>>>>> 358%3A+Migrate+Streams+API+to+Duration+instead+of+long+ms+times >>>>>>>>>>>>>>>>>>>>>>> [2] >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> https://github.com/apache/kafka/commit/ >>>>>>> >>>>>>> b3771ba22acad7870e38ff7f58820c5b50946787#diff- >>>>>>> 47289575d3e3e2449f27b3a7b6788e1aR64 >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> В Пт, 17/08/2018 в 14:46 -0500, >> John >>>>>>> >>>>>>> Roesler >>>>>>>>>> >>>>>>>>>> пишет: >>>>>>>>>>>>>>>>>>>>>>>> Hi Nikolay, >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> Thanks for this very nice KIP! >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> To answer your questions: >>>>>>>>>>>>>>>>>>>>>>>> 1. Correct, we should not >> delete >>>>>> >>>>>> existing >>>>>>>>>> >>>>>>>>>> methods that >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> have been >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> released, >>>>>>>>>>>>>>>>>>>>>>>> but ... >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> 2. Yes, we should deprecate >> the 'long' >>>>>>>>>> >>>>>>>>>> variants so that we >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> can drop >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> them >>>>>>>>>>>>>>>>>>>>>>>> later on. Personally, I like >> to mention >>>>>>> >>>>>>> which >>>>>>>>>> >>>>>>>>>> version >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> deprecated the >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> method >>>>>>>>>>>>>>>>>>>>>>>> so everyone can see later on >> how long >>>>>>> >>>>>>> it's been >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> deprecated, but this >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> may >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> be >>>>>>>>>>>>>>>>>>>>>>>> controversial, so let's let >> other weigh >>>>>>> >>>>>>> in. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> 3. I think you're asking >> whether it's >>>>>>>>>> >>>>>>>>>> appropriate to drop >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> the "Ms" >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> suffix, >>>>>>>>>>>>>>>>>>>>>>>> and I think yes. So "long >>>>>>> >>>>>>> inactivityGapMs" >>>>>>>>>> >>>>>>>>>> would become >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> "Duration >>>>>>>>>>>>>>>>>>>>>>>> inactivityGap". >>>>>>>>>>>>>>>>>>>>>>>> In the places where the >> parameter's >>>>>> >>>>>> name >>>>>>> is >>>>>>>>>> >>>>>>>>>> just >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> "duration", I think >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> we >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> can >>>>>>>>>>>>>>>>>>>>>>>> pick something more >> descriptive (I >>>>>>> >>>>>>> realize it >>>>>>>>>> >>>>>>>>>> was already >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> "durationMs"; >>>>>>>>>>>>>>>>>>>>>>>> this is just a good time to >> improve >>>>>> >>>>>> it). >>>>>>>>>>>>>>>>>>>>>>>> Also, you're correct that we >> shouldn't >>>>>>> >>>>>>> use a >>>>>>>>>> >>>>>>>>>> Duration to >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> represent a >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> moment >>>>>>>>>>>>>>>>>>>>>>>> in time, like "startTime". The >> correct >>>>>>> >>>>>>> choice >>>>>>>>>> >>>>>>>>>> is actually >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> "Instant", >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> not >>>>>>>>>>>>>>>>>>>>>>>> "LocalDateTime", though. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> https://stackoverflow.com/questions/32437550/whats-the- >>>>>>> >>>>>>> difference-between-instant-and-localdatetime >>>>>>>>>>>>>>>>>>>>>>>> explains why. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> I also had a few notes on the >> KIP >>>>>> >>>>>> itself: >>>>>>>>>>>>>>>>>>>>>>>> 4. You might want to pull >> trunk again. >>>>>> >>>>>> I >>>>>>>>>> >>>>>>>>>> noticed some >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> recent APIs are >>>>>>>>>>>>>>>>>>>>>>>> missing (see KIP-328). >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> 5. Speaking of KIP-328: those >> APIs were >>>>>>> >>>>>>> just >>>>>>>>>> >>>>>>>>>> added and >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> have never >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> been >>>>>>>>>>>>>>>>>>>>>>>> released, so there's no need to >>>>>>> >>>>>>> deprecate the >>>>>>>>>> >>>>>>>>>> methods, you >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> can just >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> replace >>>>>>>>>>>>>>>>>>>>>>>> them. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> 6. For any existing method >> that's >>>>>> >>>>>> already >>>>>>>>>> >>>>>>>>>> deprecated, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> don't bother >>>>>>>>>>>>>>>>>>>>>>>> transitioning it to Duration. >> I think >>>>>> >>>>>> the >>>>>>>>>> >>>>>>>>>> examples I >>>>>>>>>>>>>>>>> >>>>>>>>>>> >
signature.asc
Description: OpenPGP digital signature
