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

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

MarcoLotz commented on pull request #10091:
URL: https://github.com/apache/kafka/pull/10091#issuecomment-776643166


   @vcrfxia I see your point. IMHO usually I refrain from having get() and set() (or equivalent methods) doing tasks other than just accessing fields. In the scenario of some extra processing, I usually change the behaviour name to depict that (e.g. getEffectiveMaintainMs)- so that a reader can guess that there's something going on more than accessing just maintainDurationMs fields.
   
   Having said that, this is an original implementer decision and I see that other methods like gracePeriodMs() also perform computations when returning values - so I will just comply with the coding style here.
   
   I saw that there's a kind of magic number on grace periods, that is -1 when not specified. Because of this, the implementation to just 
   ```java
   public long maintainMs() { 
       Math.max(maintainDurationMs, sizeMs + graceMs);
   }
   ```
   would fail for windows bigger than one day and with no grace period, returning the maintainSizeValue of windowsSize - 1 ms instead of windowsSize.
   
   I have updated the PR to follow your suggestions and coping with the problem mentioned above.


----------------------------------------------------------------
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