Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
gt;> On Sun, Jun 24, 2018 at 5:16 PM, Matthias J. Sax < >>>>> matth...@confluent.io> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Thanks for the KIP. Overall, I think it makes sense to clean up the >>>>>>> API. >>>>>>>>> >>>>>>>>> Couple of comments: >>>>>>>>> >>>>>>>>>> Sadly there's no way to "deprecate" this >>>>>>>>>> exposure >>>>>>>>> >>>>>>>>> I disagree. We can just mark the variable as deprecated and I >>>>> advocate >>>>>>>>> to do this. When the deprecation period passed, we can make it >>>>> private >>>>>>>>> (or actually remove it; cf. my next comment). >>>>>>>>> >>>>>>>>> >>>>>>>>> Parameter, `segmentInterval` is semantically not a "window" >>>>>>>>> specification parameter but an implementation detail and thus a >> store >>>>>>>>> parameter. Would it be better to add it to `Materialized`? >>>>>>>>> >>>>>>>>> >>>>>>>>> -Matthias >>>>>>>>> >>>>>>>>> On 6/22/18 5:13 PM, Guozhang Wang wrote: >>>>>>>>>> Thanks John. >>>>>>>>>> >>>>>>>>>> On Fri, Jun 22, 2018 at 5:05 PM, John Roesler >>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> 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 >> >>>>>>>>> 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 >>>>>>>>>
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
t;>>>>> Sadly there's no way to "deprecate" this > >>>>>>>> exposure > >>>>>>> > >>>>>>> I disagree. We can just mark the variable as deprecated and I > >>> advocate > >>>>>>> to do this. When the deprecation period passed, we can make it > >>> private > >>>>>>> (or actually remove it; cf. my next comment). > >>>>>>> > >>>>>>> > >>>>>>> Parameter, `segmentInterval` is semantically not a "window" > >>>>>>> specification parameter but an implementation detail and thus a > store > >>>>>>> parameter. Would it be better to add it to `Materialized`? > >>>>>>> > >>>>>>> > >>>>>>> -Matthias > >>>>>>> > >>>>>>> On 6/22/18 5:13 PM, Guozhang Wang wrote: > >>>>>>>> Thanks John. > >>>>>>>> > >>>>>>>> On Fri, Jun 22, 2018 at 5:05 PM, John Roesler > >>>>>> wrote: > >>>>>>>> > >>>>>>>>> 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 > > >>>>>>> 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 > >>>> > >>>>>>>>>
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
>>>>>>> >>>>>>> -Matthias >>>>>>> >>>>>>> On 6/22/18 5:13 PM, Guozhang Wang wrote: >>>>>>>> Thanks John. >>>>>>>> >>>>>>>> On Fri, Jun 22, 2018 at 5:05 PM, John Roesler >>>>>> wrote: >>>>>>>> >>>>>>>>> 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 >>>>>>> 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 >>>>>>> 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 >>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> I've updated the KIP and draft PR accordingly. >>>>>>>>>>>> >>>>>>>>>>>> On Thu, Jun 21, 2018 at 2:03 PM John Roesler >>> >>>>>>>>>> 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 >>>>>>>>>> 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 >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> -- Guozhang >>>>>> >>>>> >>>> >>> >>> > signature.asc Description: OpenPGP digital signature
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
>>>>> Thanks, >> >>>>>> -John >> >>>>>> >> >>>>>> On Fri, Jun 22, 2018 at 5:51 PM Guozhang Wang >> >>>> 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 >> >>>> 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 >> >>>>>> wrote: >> >>>>>>>> >> >>>>>>>>> I've updated the KIP and draft PR accordingly. >> >>>>>>>>> >> >>>>>>>>> On Thu, Jun 21, 2018 at 2:03 PM John Roesler > > >> >>>>>>> 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 >> >>>>>>> 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 >> >>>>>>> >> >>>>>> >> >>>>> >> >>>>> >> >>>>> >> >>>> >> >>>> >> >>> >> >>> >> >>> -- >> >>> -- Guozhang >> >>> >> >> >> > >> >>
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
>>>>> 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 > >>>>>> wrote: > >>>>>>>> > >>>>>>>>> I've updated the KIP and draft PR accordingly. > >>>>>>>>> > >>>>>>>>> On Thu, Jun 21, 2018 at 2:03 PM John Roesler > >>>>>>> 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 > >>>>>>> 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 > >>>>>>> > >>>>>> > >>>>> > >>>>> > >>>>> > >>>> > >>>> > >>> > >>> > >>> -- > >>> -- Guozhang > >>> > >> > > > >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
t;>>>>>>> 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 >>>>>> wrote: >>>>>>>> >>>>>>>>> I've updated the KIP and draft PR accordingly. >>>>>>>>> >>>>>>>>> On Thu, Jun 21, 2018 at 2:03 PM John Roesler >>>>>>> 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 >>>>>>> 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 >>>>>>> >>>>>> >>>>> >>>>> >>>>> >>>> >>>> >>> >>> >>> -- >>> -- Guozhang >>> >> > signature.asc Description: OpenPGP digital signature
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
;>> Thanks, > > > >>>> Bill > > > >>>> > > > >>>> > > > >>>> > > > >>>> On Thu, Jun 21, 2018 at 3:32 PM John Roesler > > > >> wrote: > > > >>>> > > > >>>>> I've updated the KIP and draft PR accordingly. > > > >>>>> > > > >>>>> On Thu, Jun 21, 2018 at 2:03 PM John Roesler > > > >>> 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 > > > >>> 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 > > > >>> > > > >> > > > > > > > > > > > > > > > > > > > > > > > > -- > > -- Guozhang > > >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
;>>>>> 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 > > > >>>>> 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 > > > >>>>> 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 > > >>> 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 > > >>> > > >> > > > > > > > > > > > > > > > > -- > -- Guozhang >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
rface. > >>>>>> > >>>>>> On Wed, Jun 20, 2018 at 5:06 PM Guozhang Wang > >>>>> 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 > >>>>> 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 > >>> 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 > >>> > >> > > > > > > > > -- -- Guozhang
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
Thanks for the KIP. Overall, I think it makes sense to clean up the API. Couple of comments: > Sadly there's no way to "deprecate" this > exposure I disagree. We can just mark the variable as deprecated and I advocate to do this. When the deprecation period passed, we can make it private (or actually remove it; cf. my next comment). Parameter, `segmentInterval` is semantically not a "window" specification parameter but an implementation detail and thus a store parameter. Would it be better to add it to `Materialized`? -Matthias On 6/22/18 5:13 PM, Guozhang Wang wrote: > Thanks John. > > On Fri, Jun 22, 2018 at 5:05 PM, John Roesler wrote: > >> 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 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 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 >> wrote: >>>> >>>>> I've updated the KIP and draft PR accordingly. >>>>> >>>>> On Thu, Jun 21, 2018 at 2:03 PM John Roesler >>> 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 >>>>> 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 >>>>> 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". >>>>>
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
Thanks John. On Fri, Jun 22, 2018 at 5:05 PM, John Roesler wrote: > 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 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 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 > wrote: > > > > > > > I've updated the KIP and draft PR accordingly. > > > > > > > > On Thu, Jun 21, 2018 at 2:03 PM John Roesler > > 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 > > > > 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 > > > > 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 > > 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 > > > -- -- Guozhang
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
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 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 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 wrote: > > > > > I've updated the KIP and draft PR accordingly. > > > > > > On Thu, Jun 21, 2018 at 2:03 PM John Roesler > 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 > > > 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 > > > 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 > 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 >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
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 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 wrote: > > > I've updated the KIP and draft PR accordingly. > > > > On Thu, Jun 21, 2018 at 2:03 PM John Roesler 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 > > 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 > > 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 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
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
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 wrote: > I've updated the KIP and draft PR accordingly. > > On Thu, Jun 21, 2018 at 2:03 PM John Roesler 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 > 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 > 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 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 > >> > > >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
I've updated the KIP and draft PR accordingly. On Thu, Jun 21, 2018 at 2:03 PM John Roesler 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 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 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 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 >> >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
bq. propose just to make it private +1 On Thu, Jun 21, 2018 at 12:03 PM, John Roesler 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 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 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 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 > > >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
Hi Ted, Ok, I also prefer it, and I find this reasoning compelling. Thanks, -John On Thu, Jun 21, 2018 at 3:40 AM Ted Yu wrote: > Window is a common term used in various streaming processing systems whose > unit is time unit. > > Segment doesn't seem to be as widely used in such context. > I think using interval in the method name would clearly convey the meaning > intuitively. > > Thanks > > > On Wed, Jun 20, 2018 at 1:31 PM, John Roesler 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 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 > > > > > >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
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 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 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 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 >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
Window is a common term used in various streaming processing systems whose unit is time unit. Segment doesn't seem to be as widely used in such context. I think using interval in the method name would clearly convey the meaning intuitively. Thanks On Wed, Jun 20, 2018 at 1:31 PM, John Roesler 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 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 > > 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 > > >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
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 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 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 > > 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
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
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 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 > 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 >
Re: [DISCUSS] KIP-319: Replace segments with segmentSize in WindowBytesStoreSupplier
Normally size is not measured in time unit, such as milliseconds. How about naming the new method segmentInterval ? Thanks Original message From: John Roesler 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
[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
