You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/07/21 07:30:30 UTC
[GitHub] [kafka] showuon opened a new pull request #11100: MINOR: update doc to reflect the grace period change
showuon opened a new pull request #11100:
URL: https://github.com/apache/kafka/pull/11100
Found the issue while reading stream doc. We removed default 24 hours grace period in KIP-633, and deprecate some grace methods, but we forgot to update the stream docs.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673733752
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/JoinWindows.java
##########
@@ -138,7 +138,7 @@ public static JoinWindows ofTimeDifferenceWithNoGrace(final Duration timeDiffere
* @param timeDifference
* @return a new JoinWindows object with the window definition with and grace period (default to 24 hours minus {@code timeDifference})
* @throws IllegalArgumentException if {@code timeDifference} is negative or can't be represented as {@code long milliseconds}
- * @deprecated since 3.0 Use {@link #ofTimeDifferenceAndGrace(Duration, Duration)} instead
+ * @deprecated since 3.0 Use {@link #ofTimeDifferenceWithNoGrace(Duration)}} instead
Review comment:
We should use `ofTimeDifferenceWithNoGrace` for original `of` method. Fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#issuecomment-883965278
@ableegoldman @mjsax @cadonna , please take a look. I think this PR should also merge into v3.0. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673833045
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3208,14 +3208,13 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
import org.apache.kafka.streams.kstream.TimeWindows;
// A tumbling time window with a size of 5 minutes (and, by definition, an implicit
-// advance interval of 5 minutes). Note the explicit grace period, as the current
-// default value is 24 hours, which may be larger than needed for smaller windows.
-Duration windowSizeMs = Duration.ofMinutes(5);
-Duration gracePeriodMs = Duration.ofMinutes(1);
-TimeWindows.of(windowSizeMs).grace(gracePeriodMs);
+// advance interval of 5 minutes).
+Duration windowSize = Duration.ofMinutes(5);
+Duration gracePeriod = Duration.ofMinutes(1);
Review comment:
Make sense! Added 1 minute grace period comment:
`A tumbling time window with a size of 5 minutes (and, by definition, an implicit advance interval of 5 minutes), and grace period of 1 minute.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673759493
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/JoinWindows.java
##########
@@ -204,7 +204,7 @@ public long size() {
* @param afterWindowEnd The grace period to admit out-of-order events to a window.
* @return this updated builder
* @throws IllegalArgumentException if the {@code afterWindowEnd} is negative of can't be represented as {@code long milliseconds}
- * @deprecated since 3.0 Use {@link #ofTimeDifferenceWithNoGrace(Duration)} instead
+ * @deprecated since 3.0 Use {@link #ofTimeDifferenceAndGrace(Duration, Duration)} instead
*/
@Deprecated
public JoinWindows grace(final Duration afterWindowEnd) throws IllegalArgumentException {
Review comment:
We should use `ofTimeDifferenceAndGrace` for original `grace` method. Fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
cadonna commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673778552
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3208,14 +3208,13 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
import org.apache.kafka.streams.kstream.TimeWindows;
// A tumbling time window with a size of 5 minutes (and, by definition, an implicit
-// advance interval of 5 minutes). Note the explicit grace period, as the current
-// default value is 24 hours, which may be larger than needed for smaller windows.
-Duration windowSizeMs = Duration.ofMinutes(5);
-Duration gracePeriodMs = Duration.ofMinutes(1);
-TimeWindows.of(windowSizeMs).grace(gracePeriodMs);
+// advance interval of 5 minutes).
+Duration windowSize = Duration.ofMinutes(5);
+Duration gracePeriod = Duration.ofMinutes(1);
Review comment:
Here I would either mention the grace period in the comment above or remove the grace period completely. Otherwise, it comes a bit as a surprise that a grace period is specified.
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3230,13 +3229,13 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
<pre class="line-numbers"><code class="language-java">import org.apache.kafka.streams.kstream.SlidingWindows;
// A sliding time window with a time difference of 10 minutes and grace period of 30 minutes
-Duration timeDifferenceMs = Duration.ofMinutes(10);
-Duration gracePeriodMs = Duration.ofMinutes(30);
-SlidingWindows.withTimeDifferenceAndGrace(timeDifferenceMs,gracePeriodMs);</code></pre>
+Duration timeDifference = Duration.ofMinutes(10);
+Duration gracePeriod = Duration.ofMinutes(30);
+SlidingWindows.ofTimeDifferenceAndGrace(timeDifference, gracePeriod);</code></pre>
<div class="admonition note">
<p><b>Note</b></p>
<p>Sliding windows <em>require</em> that you set a grace period, as shown above. For time windows and session windows,
- setting the grace period is optional and defaults to 24 hours.</p>
+ setting the grace period is optional.</p>
Review comment:
That is not true anymore. More precisely, it is only true for deprecated methods, that we do not want to mention in the docs, right? Maybe you can even remove the note completely, since it explains a difference between sliding windows and other windows that does not exist anymore.
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3322,8 +3321,8 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
</div></div>
<p>The key parts of this program are:
<dl>
- <dt><code>grace(ofMinutes(10))</code></dt>
- <dd>This allows us to bound the lateness of events the window will accept.
+ <dt><code>ofSizeAndGrace(Duration.ofHours(1), ofMinutes(10))</code></dt>
+ <dd>The <code>ofMinutes(10)</code> parameter allows us to bound the lateness of events the window will accept.
Review comment:
```suggestion
<dd>The specified grace period of 10 minutes (i.e., the <code>ofMinutes(10)</code> argument) allows us to bound the lateness of events the window will accept.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673735822
##########
File path: streams/src/main/java/org/apache/kafka/streams/kstream/JoinWindows.java
##########
@@ -138,7 +138,7 @@ public static JoinWindows ofTimeDifferenceWithNoGrace(final Duration timeDiffere
* @param timeDifference
* @return a new JoinWindows object with the window definition with and grace period (default to 24 hours minus {@code timeDifference})
* @throws IllegalArgumentException if {@code timeDifference} is negative or can't be represented as {@code long milliseconds}
- * @deprecated since 3.0 Use {@link #ofTimeDifferenceAndGrace(Duration, Duration)} instead
+ * @deprecated since 3.0 Use {@link #ofTimeDifferenceWithNoGrace(Duration)}} instead
*/
@Deprecated
public static JoinWindows of(final Duration timeDifference) throws IllegalArgumentException {
Review comment:
We should use `ofTimeDifferenceWithNoGrace` for original `of` method. Fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673730072
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -650,7 +650,7 @@ <h4 class="anchor-heading"><a id="streams_concepts_globalktable" class="anchor-l
KTable<byte[], String> table = cogroupedStream.aggregate(initializer);
-KTable<byte[], String> table2 = cogroupedStream.windowedBy(TimeWindows.duration(500ms)).aggregate(initializer);</code></pre>
+KTable<byte[], String> table2 = cogroupedStream.windowedBy(TimeWindows.ofSizeWithNoGrace(Duration.ofMillis(500))).aggregate(initializer);</code></pre>
Review comment:
TimeWindows doesn't have `duration` method. Fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673731818
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3164,9 +3164,9 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
// A hopping time window with a size of 5 minutes and an advance interval of 1 minute.
// The window's name -- the string parameter -- is used to e.g. name the backing state store.
-Duration windowSizeMs = Duration.ofMinutes(5);
-Duration advanceMs = Duration.ofMinutes(1);
-TimeWindows.of(windowSizeMs).advanceBy(advanceMs);</code></pre>
+Duration windowSize = Duration.ofMinutes(5);
+Duration advance = Duration.ofMinutes(1);
+TimeWindows.ofSizeWithNoGrace(windowSize).advanceBy(advance);</code></pre>
Review comment:
These should be Duration type variable, not `Long` type to represent ms value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673731818
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3164,9 +3164,9 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
// A hopping time window with a size of 5 minutes and an advance interval of 1 minute.
// The window's name -- the string parameter -- is used to e.g. name the backing state store.
-Duration windowSizeMs = Duration.ofMinutes(5);
-Duration advanceMs = Duration.ofMinutes(1);
-TimeWindows.of(windowSizeMs).advanceBy(advanceMs);</code></pre>
+Duration windowSize = Duration.ofMinutes(5);
+Duration advance = Duration.ofMinutes(1);
+TimeWindows.ofSizeWithNoGrace(windowSize).advanceBy(advance);</code></pre>
Review comment:
Update the variable name here since these are not `Long` type to represent ms value. They are Duration type.
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3164,9 +3164,9 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
// A hopping time window with a size of 5 minutes and an advance interval of 1 minute.
// The window's name -- the string parameter -- is used to e.g. name the backing state store.
-Duration windowSizeMs = Duration.ofMinutes(5);
-Duration advanceMs = Duration.ofMinutes(1);
-TimeWindows.of(windowSizeMs).advanceBy(advanceMs);</code></pre>
+Duration windowSize = Duration.ofMinutes(5);
+Duration advance = Duration.ofMinutes(1);
+TimeWindows.ofSizeWithNoGrace(windowSize).advanceBy(advance);</code></pre>
Review comment:
Update the variable name here since these are not `Long` type to represent ms value (as before). They are Duration type.
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3164,9 +3164,9 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
// A hopping time window with a size of 5 minutes and an advance interval of 1 minute.
// The window's name -- the string parameter -- is used to e.g. name the backing state store.
-Duration windowSizeMs = Duration.ofMinutes(5);
-Duration advanceMs = Duration.ofMinutes(1);
-TimeWindows.of(windowSizeMs).advanceBy(advanceMs);</code></pre>
+Duration windowSize = Duration.ofMinutes(5);
+Duration advance = Duration.ofMinutes(1);
+TimeWindows.ofSizeWithNoGrace(windowSize).advanceBy(advance);</code></pre>
Review comment:
Update the variable name here since these are not `Long` type to represent ms value (as before). They are Duration type now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673732882
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3208,14 +3208,13 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
import org.apache.kafka.streams.kstream.TimeWindows;
// A tumbling time window with a size of 5 minutes (and, by definition, an implicit
-// advance interval of 5 minutes). Note the explicit grace period, as the current
-// default value is 24 hours, which may be larger than needed for smaller windows.
Review comment:
remove this note:
`Note the explicit grace period, as the current
default value is 24 hours, which may be larger than needed for smaller windows.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna merged pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
cadonna merged pull request #11100:
URL: https://github.com/apache/kafka/pull/11100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#discussion_r673833419
##########
File path: docs/streams/developer-guide/dsl-api.html
##########
@@ -3230,13 +3229,13 @@ <h5><a class="toc-backref" href="#id34">KTable-KTable Foreign-Key
<pre class="line-numbers"><code class="language-java">import org.apache.kafka.streams.kstream.SlidingWindows;
// A sliding time window with a time difference of 10 minutes and grace period of 30 minutes
-Duration timeDifferenceMs = Duration.ofMinutes(10);
-Duration gracePeriodMs = Duration.ofMinutes(30);
-SlidingWindows.withTimeDifferenceAndGrace(timeDifferenceMs,gracePeriodMs);</code></pre>
+Duration timeDifference = Duration.ofMinutes(10);
+Duration gracePeriod = Duration.ofMinutes(30);
+SlidingWindows.ofTimeDifferenceAndGrace(timeDifference, gracePeriod);</code></pre>
<div class="admonition note">
<p><b>Note</b></p>
<p>Sliding windows <em>require</em> that you set a grace period, as shown above. For time windows and session windows,
- setting the grace period is optional and defaults to 24 hours.</p>
+ setting the grace period is optional.</p>
Review comment:
You're right. Remove the note.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
cadonna commented on pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#issuecomment-884117221
Cherry-picked to 3.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11100: MINOR: update doc to reflect the grace period change
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11100:
URL: https://github.com/apache/kafka/pull/11100#issuecomment-884060176
@cadonna , thanks for comments. PR updated. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org