MINOR: rework JavaDoc for windowing related public API by mjsax · Pull Request #2337 · apache/kafka

mjsax · 2017-01-09T22:25:11Z

also some code refactoring and bug fixes

mjsax · 2017-01-09T22:25:22Z

asfbot · 2017-01-09T23:04:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/650/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-01-09T23:06:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/649/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-01-09T23:11:03Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/648/
Test FAILed (JDK 7 and Scala 2.10).

guozhangwang · 2017-01-09T23:29:09Z

test this please

asfbot · 2017-01-10T00:10:00Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/652/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-01-10T00:10:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/651/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-01-10T00:15:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/650/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-01-18T03:26:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/971/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-01-18T03:29:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/971/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-01-18T03:31:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/973/
Test FAILed (JDK 8 and Scala 2.11).

dguy · 2017-01-18T08:45:10Z

Test failures look legitimate. Looks like you will need to change the tests as the have TimeWindow with the same start and end time.
Besides that, would it not better to do the javadoc changes and code changes separate PRs?

mjsax · 2017-01-18T10:03:19Z

@dguy you are too fast :)

About splitting code and docs update. It would be better (I did split out already SessionWindow PR), but hard to split up the rest now... (Did not expect to hit so many issues :( ) If you don't insist on it, I would rather keep it as is.

Updated this. @enothereska @guozhangwang

dguy

A few comments mostly about test naming. In general i think more descriptive test names are better.
Besides that my only other concerns are:

this is +1000 lines and it is a MINOR PR.
I already mentioned it is not just javadoc, but code changes.

I'm happy to defer to others judgement on both of those points.

dguy · 2017-01-18T10:16:57Z

maybe rename to sth like: endTimeShouldNotBeBeforeStart

dguy · 2017-01-18T10:17:58Z

i'd rename this to something more descriptive, too. Though i just realised this and the last haven't changed in this PR!

dguy · 2017-01-18T10:18:57Z

What are we testing about until?

dguy · 2017-01-18T10:21:55Z

Again i feel there is a more descriptive name for this test.
sth like: shouldUseWindowSizeForMaintainMsWhenSizeLargerThanDefaultMaintainMs

asfbot · 2017-01-18T10:50:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/982/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-01-18T10:59:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/984/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-18T11:43:18Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/982/
Test FAILed (JDK 7 and Scala 2.10).

enothereska · 2017-01-18T12:03:48Z

+1 on having separate PRs, ideally.

mjsax · 2017-01-18T20:44:24Z

Extracted code changes in #2401
Will rebase this after #2401 got merged.

mjsax · 2017-01-24T08:50:59Z

Updated this. @enothereska @dguy @guozhangwang

dguy · 2017-01-24T09:10:05Z

- * Both values (before and after) must not result in an "inverse" window,
- * i.e., lower-interval-bound must not be larger than upper-interval.bound.
+ * Both values (before and after) must not result in an "inverse" window, i.e., upper-interval-bound cannot be smaller
+ * than lower-interval bound.


do you want this to be "lower-interval-bound" to be consistent with "upper-interval-bound", or vice-versa?

dguy · 2017-01-24T09:11:25Z

-     * Specifies that records of the same key are joinable if their timestamps are within
-     * the join window interval, and if the timestamp of a record from the secondary stream
-     * is later than or equal to the timestamp of a record from the first stream.
+     * Changes the end window boundary to {@code timeDifferenceMs} but keep the start window boundary as-is.


asfbot · 2017-01-24T09:37:26Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1139/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-24T10:13:59Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1137/
Test PASSed (JDK 8 and Scala 2.12).

enothereska · 2017-01-24T10:31:17Z


    /**
-     * Set the window maintain duration in milliseconds of streams time.
+     * Set the window maintain duration (retention time) in milliseconds.


Why do we need "duration" or "maintain" ? Can't we say just ...the window retention time...

We have public method maintainMs() that returns retention time and I would like to mirror this connection in the JavaDocs. Or we rename the method to retentionMs(), but this would need a KIP. For now, I would leave it as is, but will put the renaming ides into the uber API KIP I am going to do.

Does this make sense?

enothereska · 2017-01-24T10:32:25Z

    /**
-     * Specify the number of segments to be used for rolling the window store,
-     * this function is not exposed to users but can be called by developers that extend this JoinWindows specs.
+     * Return the window maintain duration (retention time) in milliseconds.


Same here and below.

enothereska · 2017-01-24T10:34:49Z

Minor nits, otherwise LGTM, thanks @mjsax

asfbot · 2017-01-24T10:35:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1137/
Test PASSed (JDK 7 and Scala 2.10).

guozhangwang · 2017-01-24T17:19:35Z

+ * This implies, that each input record defines its own window with start and end time being relative to the record's
+ * timestamp.
+ * <p>
+ * For time semantics, compare {@link org.apache.kafka.streams.processor.TimestampExtractor TimestampExtractor}.


This is not clear to me: "compare TimestampExtractor"? Or rather "see TimestampExtractor"?

guozhangwang · 2017-01-24T17:20:09Z

-     * Specifies that records of the same key are joinable if their timestamps are within
-     * the join window interval, and if the timestamp of a record from the secondary stream is
-     * earlier than or equal to the timestamp of a record from the first stream.
+     * Changes the start window boundary to {@code timeDifferenceMs} but keep the end window boundary as-is.


guozhangwang · 2017-01-24T17:21:06Z

-     * the join window interval, and if the timestamp of a record from the secondary stream
-     * is later than or equal to the timestamp of a record from the first stream.
+     * Changes the end window boundary to {@code timeDifferenceMs} but keep the start window boundary as-is.
+     * Thus, records of the same key are joinable if the timestamp of a record from the secondary stream is max


max timeDifferenceMs later than -> at most ...

guozhangwang · 2017-01-24T17:27:17Z

+ * maintain duration.
+ * <p>
+ * If not explicitly specified, the default maintain duration is 1 day.
+ * For time semantics, compare {@link org.apache.kafka.streams.processor.TimestampExtractor TimestampExtractor}.


Ditto here.

mjsax · 2017-01-24T18:33:17Z

@guozhangwang @enothereska @dguy Addresses all you comments.

- also some code refactoring and bug fixes Author: Matthias J. Sax <matthias@confluent.io> Reviewers: Damian Guy, Eno Thereska, Guozhang Wang Closes #2337 from mjsax/javaDocImprovements4 (cherry picked from commit b938c03) Signed-off-by: Guozhang Wang <wangguoz@gmail.com>

guozhangwang · 2017-01-24T19:03:10Z

Merged to trunk and 0.10.2.

asfbot · 2017-01-24T19:22:00Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/1157/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-01-24T19:22:45Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/1159/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-01-24T19:23:24Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/1157/
Test PASSed (JDK 7 and Scala 2.10).

- also some code refactoring and bug fixes Author: Matthias J. Sax <matthias@confluent.io> Reviewers: Damian Guy, Eno Thereska, Guozhang Wang Closes apache#2337 from mjsax/javaDocImprovements4

mjsax mentioned this pull request Jan 11, 2017

KAFKA-3452: follow-up -- introduce SesssionWindows #2342

Closed

mjsax force-pushed the javaDocImprovements4 branch from 729eee2 to e2c6fda Compare January 18, 2017 02:38

mjsax force-pushed the javaDocImprovements4 branch from e2c6fda to 60cfa8e Compare January 18, 2017 09:59

dguy reviewed Jan 18, 2017

View reviewed changes

mjsax mentioned this pull request Jan 18, 2017

KAFKA-4671: Fix Streams window retention policy #2401

Closed

MINOR: rework JavaDoc for windowing related public API

e5c2d33

mjsax force-pushed the javaDocImprovements4 branch from 60cfa8e to e5c2d33 Compare January 24, 2017 08:49

dguy reviewed Jan 24, 2017

View reviewed changes

enothereska reviewed Jan 24, 2017

View reviewed changes

guozhangwang reviewed Jan 24, 2017

View reviewed changes

GitHub comments

54edb99

asfgit closed this in b938c03 Jan 24, 2017

mjsax deleted the javaDocImprovements4 branch January 24, 2017 19:23

Conversation

mjsax commented Jan 9, 2017

Uh oh!

mjsax commented Jan 9, 2017

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

asfbot commented Jan 9, 2017

Uh oh!

guozhangwang commented Jan 9, 2017

Uh oh!

asfbot commented Jan 10, 2017

Uh oh!

asfbot commented Jan 10, 2017

Uh oh!

asfbot commented Jan 10, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

dguy commented Jan 18, 2017

Uh oh!

mjsax commented Jan 18, 2017

Uh oh!

dguy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

asfbot commented Jan 18, 2017

Uh oh!

enothereska commented Jan 18, 2017

Uh oh!

mjsax commented Jan 18, 2017

Uh oh!

mjsax commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enothereska commented Jan 24, 2017

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jan 24, 2017

Uh oh!