KAFKA-4671: Fix Streams window retention policy by mjsax · Pull Request #2401 · apache/kafka

mjsax · 2017-01-18T20:42:10Z

No description provided.

mjsax · 2017-01-18T20:43:19Z

Extracted code changes from #2337

Should go into 0.10.2, too.

asfbot · 2017-01-18T21:30:40Z

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

asfbot · 2017-01-18T21:33:32Z

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

asfbot · 2017-01-18T22:05:39Z

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

dguy

Just a general comment on the test names. I see you change some test names in JoinWindowsTest to be more descriptive - thanks.
However, in the other tests, i.e., SessionWindowsTest, TimeWindowsTest etc, we still have non-descriptive test names. It is great that we have the tests, but it is much better if we can just read the test names and work out what it is doing. This is why i almost always start the name of a test with should. I.e, what should it do? I'm sorry to bang on about this, but i find it super useful and important. If the test names are descriptive then you can usually understand what a class does without having to read the code.

mjsax · 2017-01-19T18:53:49Z


    @Override
-    public final boolean equals(Object o) {
+    public Windows<Window> until(final long duration) throws IllegalArgumentException {


Change return type to JoinWindows (same for all other classed inheriting from Windows)

mjsax · 2017-01-20T00:05:52Z

Updated this.
@dguy @enothereska @guozhangwang

rename test methods

asfbot · 2017-01-20T00:54:17Z

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

asfbot · 2017-01-20T00:56:50Z

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

asfbot · 2017-01-20T00:59:15Z

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

mjsax · 2017-01-20T01:54:44Z

test this please

asfbot · 2017-01-20T02:56:04Z

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

asfbot · 2017-01-20T02:57:18Z

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

asfbot · 2017-01-20T03:32:36Z

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

dguy

Thanks @mjsax LGTM

enothereska · 2017-01-20T13:39:00Z

LGTM

guozhangwang · 2017-01-22T05:50:44Z


-    private final long gapMs;
-    private long maintainDurationMs;
+    private final long gap;


Not clear why removing Ms suffix?

guozhangwang · 2017-01-22T05:51:30Z

-    public TimeWindows advanceBy(long interval) {
-        return new TimeWindows(this.size, interval);
+    public TimeWindows advanceBy(final long advance) {
+        if (!(0 < advance && advance <= size)) {


nit: if (advance <= 0 || advance > size)

guozhangwang · 2017-01-22T05:52:31Z

+    static final long DEFAULT_MAINTAIN_DURATION = 24 * 60 * 60 * 1000L; // one day (if time is represented as ms)

-    private long maintainDurationMs;
+    private long maintainDuration;


Ditto above.

guozhangwang · 2017-01-22T05:54:17Z

+    public JoinWindows until(final long duration) throws IllegalArgumentException {
+        if (duration < size()) {
+            throw new IllegalArgumentException("Window retention time (duration) cannot be smaller than the window " +
+                "size.");


nit: this can be a single line.

mjsax · 2017-01-23T03:21:43Z

@guozhangwang Updated.

asfbot · 2017-01-23T04:09:43Z

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

asfbot · 2017-01-23T04:11:58Z

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

asfbot · 2017-01-23T04:17:50Z

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

mjsax · 2017-01-23T18:58:35Z

@guozhangwang Updated all variables with Ms suffix.

asfbot · 2017-01-23T19:00:44Z

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

asfbot · 2017-01-23T19:45:57Z

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

asfbot · 2017-01-23T19:46:53Z

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

guozhangwang · 2017-01-24T00:06:54Z

retest this please

Author: Matthias J. Sax <matthias@confluent.io> Reviewers: Damian Guy, Eno Thereska, Guozhang Wang Closes #2401 from mjsax/kafka-4671-window-retention-policy (cherry picked from commit 7998759) Signed-off-by: Guozhang Wang <wangguoz@gmail.com>

guozhangwang · 2017-01-24T00:32:24Z

LGTM. Merged to trunk and 0.10.2.

asfbot · 2017-01-24T00:55:48Z

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

asfbot · 2017-01-24T01:01:16Z

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

asfbot · 2017-01-24T01:01:26Z

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

Author: Matthias J. Sax <matthias@confluent.io> Reviewers: Damian Guy, Eno Thereska, Guozhang Wang Closes apache#2401 from mjsax/kafka-4671-window-retention-policy

KAFKA-4671: Fix Streams window retention policy

87834a8

mjsax mentioned this pull request Jan 18, 2017

MINOR: rework JavaDoc for windowing related public API #2337

Closed

dguy reviewed Jan 19, 2017

View reviewed changes

mjsax commented Jan 19, 2017

View reviewed changes

fix return type for until()

9488b66

rename test methods

mjsax force-pushed the kafka-4671-window-retention-policy branch from a84369f to 9488b66 Compare January 20, 2017 00:06

dguy approved these changes Jan 20, 2017

View reviewed changes

guozhangwang reviewed Jan 22, 2017

View reviewed changes

Guozhang's comments

aeb3594

updated all variables with Ms suffix

70ac07d

asfgit closed this in 7998759 Jan 24, 2017

mjsax deleted the kafka-4671-window-retention-policy branch January 24, 2017 00:48

Conversation

mjsax commented Jan 18, 2017

Uh oh!

mjsax commented Jan 18, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jan 20, 2017

Uh oh!

asfbot commented Jan 20, 2017

Uh oh!

asfbot commented Jan 20, 2017

Uh oh!

asfbot commented Jan 20, 2017

Uh oh!

mjsax commented Jan 20, 2017

Uh oh!

asfbot commented Jan 20, 2017

Uh oh!

asfbot commented Jan 20, 2017

Uh oh!

asfbot commented Jan 20, 2017

Uh oh!

dguy left a comment

Choose a reason for hiding this comment

Uh oh!

enothereska commented Jan 20, 2017

Uh oh!

guozhangwang Jan 22, 2017

Choose a reason for hiding this comment

Uh oh!

guozhangwang Jan 22, 2017

Choose a reason for hiding this comment

Uh oh!

guozhangwang Jan 22, 2017

Choose a reason for hiding this comment

Uh oh!

guozhangwang Jan 22, 2017

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jan 23, 2017

Uh oh!

asfbot commented Jan 23, 2017

Uh oh!

asfbot commented Jan 23, 2017

Uh oh!

asfbot commented Jan 23, 2017

Uh oh!

mjsax commented Jan 23, 2017

Uh oh!

asfbot commented Jan 23, 2017

Uh oh!

asfbot commented Jan 23, 2017

Uh oh!

asfbot commented Jan 23, 2017

Uh oh!

guozhangwang commented Jan 24, 2017

Uh oh!

guozhangwang commented Jan 24, 2017

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

asfbot commented Jan 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects