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
signature.asc
Description: This is a digitally signed message part
