Maybe best to do a POC PR to see if we can do the fix without a KIP? --> Let me give it a try, and let you know! :)
Thank you. Luke On Sat, Jul 24, 2021 at 6:46 AM Matthias J. Sax <[email protected]> wrote: > Oh. I though `TimeWindow` (singular) is part of the public API... For > this case, I agree that we might not need a KIP, if there are no > compatibility concerns to internally switch from `TimeWindow` to (newly > added) `SlidingWindow` (that will also be internal). > > Maybe best to do a POC PR to see if we can do the fix without a KIP of > if we need one? > > @Luke: would this work for you? > > > -Matthias > > > On 7/23/21 1:00 PM, Sophie Blee-Goldman wrote: > > @Matthias that operator doesn't need to be deprecated/updated as the > > argument to it is a SlidingWindow*s*, not > > a SlidingWindow (which is what this KIP is proposing to add). > > The SlidingWindows class which is part of the public > > API is really just a config container class, it doesn't hold the actual > > data like the TimeWindow/SlidingWindow does. > > > > In fact, I'm not sure we even need a KIP at all for this change. The > > Window/TimeWindow/SlidingWindow classes > > are/would be internal, as they are only used within the sliding windowed > > aggregation itself. > > > > On Fri, Jul 23, 2021 at 11:04 AM Matthias J. Sax <[email protected]> > wrote: > > > >> Thanks for the KIP. Would be nice to fix this bug... > >> > >> Couple of comments: > >> > >> (1) nit: constructor should be `SlidingWindow` (not `TimeWindow` -- > >> guess just a c&p error) > >> > >> (2) Adding a new window type it not sufficient. We also need to update > >> `KGroupedStream#windowedBy()` to allow uses to use the newly added > >> window. I don't think we can change `SlidingWindows` in a backward > >> compatible way, thus, we need to add a new class and new overloads for > >> `windowedBy()` to make the transition. (Also for `CogroupedKStream`.) > >> > >> (3) #2 implies we should deprecate the exiting > >> `windowBy(SlidingWindows)` methods. > >> > >> > >> > >> -Matthias > >> > >> > >> > >> On 7/23/21 6:46 AM, Luke Chen wrote: > >>> Hi, Kafka. > >>> > >>> I'd like to discuss the KIP-765: Introduce new SlidingWindow type for > >>> [start,end] time. This is an existing bug in slidingWindows, that we > used > >>> the wrong window type so the window time interval will have 1 ms less > at > >>> the end time. This KIP will fix this issue. > >>> > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-765%3A+Introduce+new+SlidingWindow+type+for+%5Bstart%2Cend%5D+time > >>> > >>> Thank you. > >>> > >>> Luke > >>> > >> > > >
