Re: [DISCUSS] KIP-450: Sliding Windows

Thanks for the nits Matthias, I've updated the examples and language accordingly. Leah On Tue, Jul 28, 2020 at 6:43 PM Matthias J. Sax wrote: > Thanks Leah. Overall LGTM. > > A few nits: > > - the first figure shows window [9,19] but the window is not aligned > properly (it should be 1ms to

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks Leah. Overall LGTM. A few nits: - the first figure shows window [9,19] but the window is not aligned properly (it should be 1ms to the right; right now, it's aligned to window [8,18]) - in the second figure, a hopping window would create more windows, ie, the first window would be

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks for the update Leah -- I think that all makes sense. Cheers, Sophie On Tue, Jul 28, 2020 at 3:55 PM Leah Thomas wrote: > Another minor tweak, instead of defining the window by the *size*, it will > be defined by *timeDifference*, which is the maximum time difference > between two

Re: [DISCUSS] KIP-450: Sliding Windows

Another minor tweak, instead of defining the window by the *size*, it will be defined by *timeDifference*, which is the maximum time difference between two events. This is a more precise way to define a window due to its inclusive ends, while allowing the user to create the window they expect.

Re: [DISCUSS] KIP-450: Sliding Windows

A small tweak - to make it more clear to users that grace is required, as well as cleaning up some of the confusing grammar semantics of windows, the main builder method for *slidingWindows* will be *withSizeAndGrace* instead of *of*. This looks like it'll be the last change (for now) on the

Re: [DISCUSS] KIP-450: Sliding Windows

To accommodate the change to a final class, I've added another *windowedBy()* function in *CogroupedKStream.java *to handle SlidingWindows. As far as the discussion goes, I think this is the last change we've talked about. If anyone has other comments or concerns, please let me know! Cheers,

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks for the discussion about extending TimeWindows. I agree that making it future proof is important, and the implementation of SlidingWindows is unique enough that it seems logical to make it its own final class. On that note, I've updated the KIP to make SlidingWindows a stand alone final

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks for the reply, Sophie. That all sounds about right to me. The Windows “interface”/algorithm is quite flexible, so it makes sense for it to be extensible. Different implementations can (and do) enumerate different windows to suit different use cases. On the other hand, I can’t think

Re: [DISCUSS] KIP-450: Sliding Windows

> > Users could pass in a custom `SessionWindows` as > long as the session algorithm works correctly for it. Well not really, SessionWindows is a final class. TimeWindows is also a final class, so neither of these can be extended/customized. For a given Windows then there would only be three

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks, all, I can see how my conclusion was kind of a leap. What Matthias said is indeed what I was thinking. When you provide a window definition to the windowBy() method, you are selecting an algorithm that will be used to compute the windows from the input data. I didn’t mean the code

Re: [DISCUSS] KIP-450: Sliding Windows

I think what John tries to say is the following: We have `windowedBy(Windows)` that accept hopping/tumbling windows but also custom window and we use a specific algorithm. Note, that custom windows must "work" based on the used algorithm. For session windows we have `windowedBy(SessionWindows)`

Re: [DISCUSS] KIP-450: Sliding Windows

Hey John, Just a few follow-up questions/comments about the whole Windows thing: That's a good way of looking at things; in particular the point about SessionWindows for example requiring a Merger while other "statically enumerable" windows require only an adder seems to touch on the heart of

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks Leah! 5) Regarding the empty windows, I'm wondering if we should simply propose that the windows should not be emitted downstream of the operator or visible in IQ. Then, it'll be up to the implementation to make it happen. I'm personally not concerned about it, since it seems like there

Re: [DISCUSS] KIP-450: Sliding Windows

Hi Matthias, Thanks for the suggestions, I've updated the KIP and child page accordingly and addressed some below. 1) For the mandatory grace period, we should use a static builder method > that take two parameters. > That makes sense, I've changed that in the public API. Btw: this

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks for updating the KIP. Couple of follow up comments: 1) For the mandatory grace period, we should use a static builder method that take two parameters. This provides a better API as user cannot forget to set the grace period. Throwing a runtime exception seems not to be the best way to

Re: [DISCUSS] KIP-450: Sliding Windows

Hi Leah, Thanks for the updated KIP. I agree that extending SlidingWindows from Windows is fine for the sake of not introducing more public APIs (and their internal xxxImpl classes), and its cons is small enough to tolerate to me. Guozhang On Mon, Jul 20, 2020 at 1:49 PM Leah Thomas wrote:

Re: [DISCUSS] KIP-450: Sliding Windows

Hi all, Thanks for the feedback on the KIP. I've updated the KIP page to address these points and have created a child page

Re: [DISCUSS] KIP-450: Sliding Windows

Hello all, Thanks for the KIP, Leah! Regarding (1): I'd go farther actually. Making Windows an abstract class was a mistake from the beginning that led to us not being able to fix a very confusing situation for users around retention times, final results emitting, etc. Thus, I would not suggest

Re: [DISCUSS] KIP-450: Sliding Windows

Hello Leah, Thanks for the nice written KIP. A few thoughts: 1) I echo the other reviewer's comments regarding the typing: why extending TimeWindow instead of just extending Window? 2) I also feel that emitting policy for this type of windowing aggregation may be different from the existing

Re: [DISCUSS] KIP-450: Sliding Windows

Hi Leah, Thank you for the KIP! Here is my feedback: 1. The KIP would benefit from some code examples that show how to use sliding windows in aggregations. 2. The different sliding windows in Figure 1 and 2 are really hard to distinguish. Could you please try to make them graphically better

Re: [DISCUSS] KIP-450: Sliding Windows

Leah, thanks for your update. However, it does not completely answer my question. In our current window implementations, we emit a window result update record (ie, early/partial result) for each input record. When an out-of-order record arrives, we just update to corresponding old window and

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks Leah and Sophie for the KIP. 1. I'm a bit surprised that we don't have an advance time. Could we elaborate how the storage layer is structured? 2. IIUC, there will be extra cost in terms of fetching aggregation results, since we couldn't pre-aggregate until the user asks for it. Would be

Re: [DISCUSS] KIP-450: Sliding Windows

Hey Matthias, Thanks for pointing that out. I added the following to the Propose Changes section of the KIP: "Records that come out of order will be processed the same way as in-order records, as long as they fall within the grace period. Any new windows created by the late record will still be

Re: [DISCUSS] KIP-450: Sliding Windows

Thanks Leah! This kind of assumes an implicit answer to Matthias's question, but I was wondering if we should take this opportunity to choose a better default value for the grace period. Note that the default of -1 in the TimeWindows class, for example, ultimately gets translated into a default

Re: [DISCUSS] KIP-450: Sliding Windows

Leah, thanks a lot for the KIP. Very well written. The KIP does not talk about the handling of out-of-order data though. How do you propose to address this? -Matthias On 7/8/20 5:33 PM, Leah Thomas wrote: > Hi all, > I'd like to kick-off the discussion for KIP-450, adding sliding window >