[
https://issues.apache.org/jira/browse/KAFKA-8924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16934571#comment-16934571
]
John Roesler commented on KAFKA-8924:
-------------------------------------
Hey [~atais],
Thanks for being proactive about improving this situation!
You can certainly prepare a PR, but also be aware that before we can really
consider merging it, this proposal would need a KIP. The reason is that this is
a change to the public API, and we want to give everyone who may be affected a
chance to comment on the design.
The process is documented on
https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals .
I'd recommend looking through a few recent KIPs that relate to Streams to get a
feel for them, and then just copy one that you like as a template. Let me know
if you run into any roadblocks, I'm happy to help you through the process.
Specifically on the design, I think people would be more comfortable with it if
you make it a static factory method instead of the constructor, just because
all the other "config objects" in Streams use this pattern. So something like
`TimeWindows.of(size, advance, grace)`.
I wasn't sure about your second question. Are you asking if you should avoid
using deprecated methods (yes), or if you should deprecate the other static
factories in TimeWindows (probably not, although we can discuss it further)?
Thanks!
-John
> Default grace period (-1) of TimeWindows causes suppress to never emit events
> ------------------------------------------------------------------------------
>
> Key: KAFKA-8924
> URL: https://issues.apache.org/jira/browse/KAFKA-8924
> Project: Kafka
> Issue Type: Bug
> Components: streams
> Affects Versions: 2.3.0
> Reporter: Michał
> Priority: Major
>
> h2. Problem
> The default creation of TimeWindows, like
> {code}
> TimeWindows.of(ofMillis(xxx))
> {code}
> calls an internal constructor
> {code}
> return new TimeWindows(sizeMs, sizeMs, -1, DEFAULT_RETENTION_MS);
> {code}
> And the *-1* parameter is the default grace period which I think is here for
> backward compatibility
> {code}
> @SuppressWarnings("deprecation") // continuing to support
> Windows#maintainMs/segmentInterval in fallback mode
> @Override
> public long gracePeriodMs() {
> // NOTE: in the future, when we remove maintainMs,
> // we should default the grace period to 24h to maintain the default
> behavior,
> // or we can default to (24h - size) if you want to be super accurate.
> return graceMs != -1 ? graceMs : maintainMs() - size();
> }
> {code}
> The problem is that if you use a TimeWindows with gracePeriod of *-1*
> together with suppress *untilWindowCloses*, it never emits an event.
> You can check the Suppress tests
> (SuppressScenarioTest.shouldSupportFinalResultsForTimeWindows), where
> [~vvcephei] was (maybe) aware of that and all the scenarios specify the
> gracePeriod.
> I will add a test without it on my branch and it will fail.
> The test:
> https://github.com/atais/kafka/commit/221a04dc40d636ffe93a0ad95dfc6bcad653f4db
>
> h2. Now what can be done
> One easy fix would be to change the default value to 0, which works fine for
> me in my project, however, I am not aware of the impact it would have done
> due to the changes in the *gracePeriodMs* method mentioned before.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
