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 <j...@confluent.io> 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 <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 >>> >> > > >
signature.asc
Description: OpenPGP digital signature
