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
