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
>>>>>>>>>>>>
>>>>>>>>>>>> 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
