vcrfxia commented on a change in pull request #10091:
URL: https://github.com/apache/kafka/pull/10091#discussion_r573909774
##########
File path:
streams/src/test/java/org/apache/kafka/streams/kstream/internals/TimeWindowedKStreamImplTest.java
##########
@@ -91,6 +92,47 @@ public void shouldCountWindowed() {
equalTo(ValueAndTimestamp.make(1L, 500L)));
}
+ @Test
+ public void
shouldSupportWindowAndGracePeriodLongerThanDefaultRetentionTime() {
Review comment:
It's odd to me that test coverage is being added in this file rather
than adding unit tests in `TimeWindowsTest` directly. I understand that this is
where the original error message was being thrown but it seems it should be the
responsibility of `TimeWindows` to properly implement `size()`,
`gracePeriodMs()`, and `maintainMs()`, and `TimeWindowedKStreamImpl` should
call those directly without needing duplicate tests for different combinations
of those values.
WDYT? I'm new to this repo so I defer to @mjsax for more authoritative
comments on test philosophy :)
##########
File path:
streams/src/main/java/org/apache/kafka/streams/kstream/TimeWindows.java
##########
@@ -214,7 +216,10 @@ 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();
+ if (graceMs != EMPTY_GRACE_PERIOD) {
+ return graceMs;
+ }
+ return maintainDurationMs > sizeMs ? maintainDurationMs - sizeMs : 0;
Review comment:
nit: how about
```suggestion
return Math.max(maintainDurationMs - sizeMs, 0);
```
for a slight improvement in readability?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
