KAFKA-10847: Remove internal config for enabling the fix #10941

guozhangwang · 2021-06-29T21:05:52Z

Also update the upgrade guide indicating about the grace period KIP and its indication on the fix with throughput impact.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…emove-internal-config

guozhangwang · 2021-06-29T21:06:25Z

ping @mjsax @spena .

showuon

@guozhangwang , find a typo in upgrade doc. Thanks.

showuon · 2021-06-30T10:25:20Z

docs/streams/upgrade-guide.html

+        Records coming in after the grace period has elapsed will be dropped from those windows.
+        With a long grace period, though Kafka Streams can handle out-of-order data up to that amount of time, it will also incur a high and confusing latency for users,
+        e.g. suppression operators with the default won't emit results up for 24 hours, while lso in practice out-of-order data usually has a much smaller time-skewness.
+        Instead of abstracting this config from users with a long default value, we introduced new constructs such as <code>TimeWindows#ofSizeWithNoGrace</code> to let callers always set it upon constructing the windows;


I think you're referring TimeWindows#ofSizeAndGrace, so that user can set the grace value.

It's also TimeDiffernce, not Size

TimeDiffernce is only in JoinWindows.

In the old API, yes, but KIP-633 change it from size to timeDifference (https://cwiki.apache.org/confluence/display/KAFKA/KIP-633%3A+Drop+24+hour+default+of+grace+period+in+Streams)

mjsax · 2021-07-01T04:54:42Z

docs/streams/upgrade-guide.html

@@ -117,6 +117,21 @@ <h3><a id="streams_api_changes_300" href="#streams_api_changes_300">Streams API
    <p>
        We removed the default implementation of <code>RocksDBConfigSetter#close()</code>.
    </p>
+    <p>
+        We dropped the default 24 hours grace period for windowed operations such as Window or Session aggregates, or stream-stream joins.
+        This period determines how long after a window ends any late arrived records will still be processed.


late -> out-of-order

Record are "late" by definition if they are dropped because they arrive after the grace period passed.

mjsax · 2021-07-01T04:55:37Z

docs/streams/upgrade-guide.html

+        This period determines how long after a window ends any late arrived records will still be processed.
+        Records coming in after the grace period has elapsed will be dropped from those windows.
+        With a long grace period, though Kafka Streams can handle out-of-order data up to that amount of time, it will also incur a high and confusing latency for users,
+        e.g. suppression operators with the default won't emit results up for 24 hours, while lso in practice out-of-order data usually has a much smaller time-skewness.


lso ? Should it be also?

mjsax · 2021-07-01T04:57:59Z

docs/streams/upgrade-guide.html

@@ -117,6 +117,21 @@ <h3><a id="streams_api_changes_300" href="#streams_api_changes_300">Streams API
    <p>
        We removed the default implementation of <code>RocksDBConfigSetter#close()</code>.
    </p>
+    <p>
+        We dropped the default 24 hours grace period for windowed operations such as Window or Session aggregates, or stream-stream joins.


Why do we include this in this PR? Seems it should be part of KIP-663 doc updates? \cc @izzyacademy

Since we are piggy-backing the fix on KIP-663 now, I want to incorporate the change along with this PR.

mjsax · 2021-07-01T05:03:32Z

docs/streams/upgrade-guide.html

+        e.g. suppression operators with the default won't emit results up for 24 hours, while lso in practice out-of-order data usually has a much smaller time-skewness.
+        Instead of abstracting this config from users with a long default value, we introduced new constructs such as <code>TimeWindows#ofSizeWithNoGrace</code> to let callers always set it upon constructing the windows;
+        the other setters such as <code>TimeWindows#grace</code> are deprecated and will be removed in the future.
+        Also when the new construct API are used for left/outer stream-stream joins, Kafka Streams would fix emitting spurious join results which may have an impact on the throughput.


would fix emitting spurious join results -- not sure if users would understand this.

Maybe better:

In older versions, Kafka Streams emitted stream-stream left/outer join results eagerly. This behavior may lead to spurious left/outer join result records. In this release, we changed the behavior to avoid spurious results and left/outer join result are only emitted after the join window is closed, i.e., after the grace period elapsed. To maintain backward compatibility, the old API <code>JoinWindows.of(size)</code> preserves the old eager-emit behavior and only the new API `<code>JoinWindows.ofTimeDifferenceAndGrace()</code> (and <code>JoinsWindows#ofTimeDifferenceNoGrace</code>) enable the new behavior.

izzyacademy

@guozhangwang could you rebase with trunk and resubmit with the requested changes? It is currently in conflict and can't be merged as is.

guozhangwang

Addressed comments on docs, and rebased on trunk cc @mjsax @showuon @izzyacademy

guozhangwang · 2021-07-15T04:13:29Z

docs/streams/upgrade-guide.html

@@ -117,6 +117,21 @@ <h3><a id="streams_api_changes_300" href="#streams_api_changes_300">Streams API
    <p>
        We removed the default implementation of <code>RocksDBConfigSetter#close()</code>.
    </p>
+    <p>
+        We dropped the default 24 hours grace period for windowed operations such as Window or Session aggregates, or stream-stream joins.


Since we are piggy-backing the fix on KIP-663 now, I want to incorporate the change along with this PR.

guozhangwang · 2021-07-15T04:16:11Z

docs/streams/upgrade-guide.html

+        Records coming in after the grace period has elapsed will be dropped from those windows.
+        With a long grace period, though Kafka Streams can handle out-of-order data up to that amount of time, it will also incur a high and confusing latency for users,
+        e.g. suppression operators with the default won't emit results up for 24 hours, while lso in practice out-of-order data usually has a much smaller time-skewness.
+        Instead of abstracting this config from users with a long default value, we introduced new constructs such as <code>TimeWindows#ofSizeWithNoGrace</code> to let callers always set it upon constructing the windows;


TimeDiffernce is only in JoinWindows.

guozhangwang · 2021-07-15T04:16:25Z

docs/streams/upgrade-guide.html

+        e.g. suppression operators with the default won't emit results up for 24 hours, while lso in practice out-of-order data usually has a much smaller time-skewness.
+        Instead of abstracting this config from users with a long default value, we introduced new constructs such as <code>TimeWindows#ofSizeWithNoGrace</code> to let callers always set it upon constructing the windows;
+        the other setters such as <code>TimeWindows#grace</code> are deprecated and will be removed in the future.
+        Also when the new construct API are used for left/outer stream-stream joins, Kafka Streams would fix emitting spurious join results which may have an impact on the throughput.


mjsax

LGTM. Thanks for updating the PR.

Also update the upgrade guide indicating about the grace period KIP and its indication on the fix with throughput impact. Reviewers: Luke Chen <[email protected]>, Matthias J. Sax <[email protected]>

guozhangwang · 2021-07-15T17:59:22Z

Merged to trunk; cherry-picked to 3.0.

Also update the upgrade guide indicating about the grace period KIP and its indication on the fix with throughput impact. Reviewers: Luke Chen <[email protected]>, Matthias J. Sax <[email protected]>

guozhangwang added 3 commits June 29, 2021 14:02

remove the internal config

77a442e

Merge branch 'trunk' of https://github.com/apache/kafka into K10847-r…

7098941

…emove-internal-config

clarify the impact on throughput

d59c65b

guozhangwang mentioned this pull request Jun 30, 2021

fix: enable KAFKA-10847 bugfix when GRACE PERIOD is used on Joins confluentinc/ksql#7739

Merged

2 tasks

showuon reviewed Jun 30, 2021

View reviewed changes

mjsax reviewed Jul 1, 2021

View reviewed changes

izzyacademy suggested changes Jul 5, 2021

View reviewed changes

rebase from trunk

7397d0d

guozhangwang commented Jul 15, 2021

View reviewed changes

github comments

c8f5a94

mjsax approved these changes Jul 15, 2021

View reviewed changes

guozhangwang merged commit 3e32647 into apache:trunk Jul 15, 2021

guozhangwang deleted the K10847-remove-internal-config branch July 15, 2021 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-10847: Remove internal config for enabling the fix #10941

KAFKA-10847: Remove internal config for enabling the fix #10941

guozhangwang commented Jun 29, 2021

guozhangwang commented Jun 29, 2021

showuon left a comment

showuon Jun 30, 2021

mjsax Jul 1, 2021

guozhangwang Jul 15, 2021

mjsax Jul 15, 2021

mjsax Jul 1, 2021

mjsax Jul 1, 2021

mjsax Jul 1, 2021

guozhangwang Jul 15, 2021

mjsax Jul 1, 2021

guozhangwang Jul 15, 2021

izzyacademy left a comment

guozhangwang left a comment

guozhangwang Jul 15, 2021

guozhangwang Jul 15, 2021

guozhangwang Jul 15, 2021

mjsax left a comment

guozhangwang commented Jul 15, 2021

KAFKA-10847: Remove internal config for enabling the fix #10941

KAFKA-10847: Remove internal config for enabling the fix #10941

Conversation

guozhangwang commented Jun 29, 2021

Committer Checklist (excluded from commit message)

guozhangwang commented Jun 29, 2021

showuon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izzyacademy left a comment

Choose a reason for hiding this comment

guozhangwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax left a comment

Choose a reason for hiding this comment

guozhangwang commented Jul 15, 2021