Hi John, Thanks for the KIP, and overall it's a +1 for me.
In the JavaDoc for the segmentInterval method, there's no mention of the number of segments there can be at any one time. While it's implied that the number of segments is potentially unbounded, would be better to explicitly state that the previous limit on the number of segments is going to be removed as well? I have a couple of nit comments. The method name is still segmentSize in the code block vs segmentInterval and the order of the parameters for the third persistentWindowStore don't match the order in the JavaDoc. Thanks, Bill On Thu, Jun 21, 2018 at 3:32 PM John Roesler <j...@confluent.io> wrote: > I've updated the KIP and draft PR accordingly. > > On Thu, Jun 21, 2018 at 2:03 PM John Roesler <j...@confluent.io> wrote: > > > Interesting... I did not initially consider it because I didn't want to > > have an impact on anyone's Streams apps, but now I see that unless > > developers have subclassed `Windows`, the number of segments would always > > be 3! > > > > There's one caveat to this, which I think was a mistake. The field > > `segments` in Windows is public, which means that anyone can actually set > > it directly on any Window instance like: > > > > TimeWindows tw = TimeWindows.of(100); > > tw.segments = 12345; > > > > Bypassing the bounds check and contradicting the javadoc in Windows that > > says users can't directly set it. Sadly there's no way to "deprecate" > this > > exposure, so I propose just to make it private. > > > > With this new knowledge, I agree, I think we can switch to > > "segmentInterval" throughout the interface. > > > > On Wed, Jun 20, 2018 at 5:06 PM Guozhang Wang <wangg...@gmail.com> > wrote: > > > >> Hello John, > >> > >> Thanks for the KIP. > >> > >> Should we consider making the change on `Stores#persistentWindowStore` > >> parameters as well? > >> > >> > >> Guozhang > >> > >> > >> On Wed, Jun 20, 2018 at 1:31 PM, John Roesler <j...@confluent.io> > wrote: > >> > >> > Hi Ted, > >> > > >> > Ah, when you made that comment to me before, I thought you meant as > >> opposed > >> > to "segments". Now it makes sense that you meant as opposed to > >> > "segmentSize". > >> > > >> > I named it that way to match the peer method "windowSize", which is > >> also a > >> > quantity of milliseconds. > >> > > >> > I agree that "interval" is more intuitive, but I think I favor > >> consistency > >> > in this case. Does that seem reasonable? > >> > > >> > Thanks, > >> > -John > >> > > >> > On Wed, Jun 20, 2018 at 1:06 PM Ted Yu <yuzhih...@gmail.com> wrote: > >> > > >> > > Normally size is not measured in time unit, such as milliseconds. > >> > > How about naming the new method segmentInterval ? > >> > > Thanks > >> > > -------- Original message --------From: John Roesler < > >> j...@confluent.io> > >> > > Date: 6/20/18 10:45 AM (GMT-08:00) To: dev@kafka.apache.org > >> Subject: > >> > > [DISCUSS] KIP-319: Replace segments with segmentSize in > >> > > WindowBytesStoreSupplier > >> > > Hello All, > >> > > > >> > > I'd like to propose KIP-319 to fix an issue I identified in > >> KAFKA-7080. > >> > > Specifically, we're creating CachingWindowStore with the *number of > >> > > segments* instead of the *segment size*. > >> > > > >> > > Here's the jira: https://issues.apache.org/jira/browse/KAFKA-7080 > >> > > Here's the KIP: https://cwiki.apache.org/confluence/x/mQU0BQ > >> > > > >> > > additionally, here's a draft PR for clarity: > >> > > https://github.com/apache/kafka/pull/5257 > >> > > > >> > > Please let me know what you think! > >> > > > >> > > Thanks, > >> > > -John > >> > > > >> > > >> > >> > >> > >> -- > >> -- Guozhang > >> > > >
