You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Luke Chen <sh...@gmail.com> on 2021/07/23 13:46:19 UTC
[DISCUSS] KIP-765: Introduce new SlidingWindow type for [start,end] time
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
Re: [DISCUSS] KIP-765: Introduce new SlidingWindow type for
[start,end] time
Posted by Luke Chen <sh...@gmail.com>.
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 <mj...@apache.org> 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 <mj...@apache.org>
> 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
> >>>
> >>
> >
>
Re: [DISCUSS] KIP-765: Introduce new SlidingWindow type for
[start,end] time
Posted by "Matthias J. Sax" <mj...@apache.org>.
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 <mj...@apache.org> 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
>>>
>>
>
Re: [DISCUSS] KIP-765: Introduce new SlidingWindow type for
[start,end] time
Posted by Sophie Blee-Goldman <so...@confluent.io.INVALID>.
@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 <mj...@apache.org> 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
> >
>
Re: [DISCUSS] KIP-765: Introduce new SlidingWindow type for
[start,end] time
Posted by "Matthias J. Sax" <mj...@apache.org>.
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
>