[GitHub] [kafka] vcrfxia commented on a change in pull request #10091: KAFKA-9524: increase retention time for window and grace periods longer than one day

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org