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