Thanks for the feedback, Bill and Guozhang, I've updated the KIP accordingly.
Thanks, -John On Fri, Jun 22, 2018 at 5:51 PM Guozhang Wang <wangg...@gmail.com> wrote: > Thanks for the KIP. I'm +1 on the proposal. One minor comment on the wiki: > the `In Windows, we will:` section code snippet is empty. > > On Fri, Jun 22, 2018 at 3:29 PM, Bill Bejeck <bbej...@gmail.com> wrote: > > > 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 > > > >> > > > > > > > > > > > > > -- > -- Guozhang >
