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 >>>>>>>>>>>>>> >>>>>>>>>>>>>> noticed were >>>>>>>>>>>>>>>>>>>>> deprecated in KIP-328, so you'll see >>>> >>>> what I'm >>>>>>> >>>>>>> talking >>>>>>>>>>>>>> >>>>>>>>>>>>>> about when you >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> pull >>>>>>>>>>>>>>>>>>>>> trunk again. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 7. Any method taking a Duration >>> >>> argument >>>> may >>>>>>> >>>>>>> throw an >>>>>>>>>>>>>>>>>>>>> IllegalArgumentException (we choose to >>>> >>>> convert >>>>>>>>>>>>>> >>>>>>>>>>>>>> ArithmeticException to >>>>>>>>>>>>>>>>>>>>> IllegalArgumentException, as I >>> >>> mentioned >>>> in >>>>>>> >>>>>>> the Jira >>>>>>>>>>>>>> >>>>>>>>>>>>>> ticket). We >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> don't >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> need >>>>>>>>>>>>>>>>>>>>> a "throws" declaration, but we should >>>> >>>> plan to >>>>>>> >>>>>>> mention this >>>>>>>>>>>>>> >>>>>>>>>>>>>> in the >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> javadoc >>>>>>>>>>>>>>>>>>>>> for those methods. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 8. In Stores, windowSize and >>>> >>>> segmentInterval >>>>>>> >>>>>>> should also be >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> durations. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 9. In StreamsMetrics, recordLatency >>>> >>>> could be >>>>>>> >>>>>>> just a >>>>>>>>>>>>>> >>>>>>>>>>>>>> Duration, but I >>>>>>>>>>>>>>>>>>>>> actually think this one is better left >>>> >>>> alone. >>>>>>> >>>>>>> IMO, it's >>>>>>>>>>>>>> >>>>>>>>>>>>>> more effort >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> for >>>>>>>>>>>>>>>>>>>>> little gain to require users to >>>> >>>> construct a >>>>>>> >>>>>>> Duration >>>>>>>>>>>>>> >>>>>>>>>>>>>> before they >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> call the >>>>>>>>>>>>>>>>>>>>> method, since they vary likely call >>>>>>>>>>>>>> >>>>>>>>>>>>>> System.currentTimeNanos before >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>>>>>>> after the code in question. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> These are quite a few notes, but >>> >>> they're >>>> all >>>>>>> >>>>>>> minor. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Overall the KIP >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> looks >>>>>>>>>>>>>>>>>>>>> really good to me. Thanks for picking >>>> >>>> this up! >>>>>>>>>>>>>>>>>>>>> -John >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> On Thu, Aug 16, 2018 at 9:55 AM Nikolay >>>>>>> >>>>>>> Izhikov < >>>>>>>>>>>>>> >>>>>>>>>>>>>> nizhi...@apache.org >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Hello, Kafka developers. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> I would like to start a discussion of >>>>>>> >>>>>>> KIP-358 [1]. >>>>>>>>>>>>>>>>>>>>>> It based on a ticket KAFKA-7277 [2]. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> I crawled through Stream API and made >>>> >>>> my >>>>>>> >>>>>>> suggestions for >>>>>>>>>>>>>> >>>>>>>>>>>>>> API >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> changes. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> I have several questions about >>> >>> changes. >>>>>>>>>>>>>>>>>>>>>> Please, share your comments: >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> 1. I propose do not remove existing >>> >>> API >>>>>>> >>>>>>> methods with >>>>>>>>>>>>>> >>>>>>>>>>>>>> long ms >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> parameters. >>>>>>>>>>>>>>>>>>>>>> Is it correct? >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> 2. Should we mark existing methods as >>>>>>> >>>>>>> deprecated? >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> 3. Suggested changes in ticket >>>> >>>> description >>>>>>> >>>>>>> are `long >>>>>>>>>>>>>> >>>>>>>>>>>>>> durationMs` to >>>>>>>>>>>>>>>>>>>>>> `Duration duration` and similar. >>>>>>>>>>>>>>>>>>>>>> I suggest to change 'long >>> >>> startTimeMs` >>>> to >>>>>>> >>>>>>> `LocalDateTime >>>>>>>>>>>>>> >>>>>>>>>>>>>> startTime` >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> also. >>>>>>>>>>>>>>>>>>>>>> Should we do it? >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Please, note, it very first KIP for >>>> >>>> me, so >>>>>>> >>>>>>> tell me if I >>>>>>>>>>>>>> >>>>>>>>>>>>>> miss >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> something >>>>>>>>>>>>>>>>>>>>>> obvious for experienced Kafka >>>> >>>> developers. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> [1] >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>> >>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- >>>> >>>> 358%3A+Migrate+Streams+API+to+Duration+instead+of+long+ms+times >>>>>>>>>>>>>>>>>>>>>> [2] >>>>>>> >>>>>>> https://issues.apache.org/jira/browse/KAFKA-7277 >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>> >>>>>>>> >>>>> >>>>> >>> >>> >>> >>> -- >>> -- Guozhang
signature.asc
Description: OpenPGP digital signature
