[DISCUSS] Removing delete*Timer from the WindowOperator.Context

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[DISCUSS] Removing delete*Timer from the WindowOperator.Context

Kostas Kloudas
Hi all,

As the title of this email suggests, I am proposing to remove the  methods
deleteProcessingTimeTimer(long time) and deleteEventTimeTimer(long time)
from the WindowOperator.Context. With this change, registered timers that
have nothing to do (e.g. because their state has already been cleaned up)
will be simply ignored by the windowOperator, when their time comes.

The reason for the change is that by allowing custom user code, e.g. a custom Trigger,
to delete timers we may have unpredictable behavior.

As an example, one can imagine the case where we have allowed_lateness = 0 and the cleanup
timer for a window collides with the end_of_window one. In this case, by deleting the end_of_window
timer from the trigger (possibly a custom one), we end up also deleting the cleanup one,
which in turn can lead to the window state never being garbage collected.

To see what can be the consequences apart from memory leaks, this can easily lead
to wrong session windows, as a session that should have been garbage collected, will
still be around and ready to accept new data.

With this change, timers that should correctly be deleted will now remain in the queue of
pending timers, but they will do nothing, while cleanup timers will cleanup the state of their
corresponding window.

Other possible solutions like keeping a separate list for cleanup timers would complicate
the codebase and also introduce memory overheads which can be avoided using the
solution above (i.e. just ignoring timers the have nothing to do anymore).

What do you think?

Kostas

mxm
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Removing delete*Timer from the WindowOperator.Context

mxm
What are the use cases where you actually need to delete a timer? How
about we only let users delete timers which they created themselves?

I guessing most of these use cases will be obsolete with the new
Trigger DSL because the trigger logic can be expressed more easily. So
+1 for removing the delete methods from the context.

On Tue, Sep 27, 2016 at 3:43 PM, Kostas Kloudas
<[hidden email]> wrote:

> Hi all,
>
> As the title of this email suggests, I am proposing to remove the  methods
> deleteProcessingTimeTimer(long time) and deleteEventTimeTimer(long time)
> from the WindowOperator.Context. With this change, registered timers that
> have nothing to do (e.g. because their state has already been cleaned up)
> will be simply ignored by the windowOperator, when their time comes.
>
> The reason for the change is that by allowing custom user code, e.g. a custom Trigger,
> to delete timers we may have unpredictable behavior.
>
> As an example, one can imagine the case where we have allowed_lateness = 0 and the cleanup
> timer for a window collides with the end_of_window one. In this case, by deleting the end_of_window
> timer from the trigger (possibly a custom one), we end up also deleting the cleanup one,
> which in turn can lead to the window state never being garbage collected.
>
> To see what can be the consequences apart from memory leaks, this can easily lead
> to wrong session windows, as a session that should have been garbage collected, will
> still be around and ready to accept new data.
>
> With this change, timers that should correctly be deleted will now remain in the queue of
> pending timers, but they will do nothing, while cleanup timers will cleanup the state of their
> corresponding window.
>
> Other possible solutions like keeping a separate list for cleanup timers would complicate
> the codebase and also introduce memory overheads which can be avoided using the
> solution above (i.e. just ignoring timers the have nothing to do anymore).
>
> What do you think?
>
> Kostas
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Removing delete*Timer from the WindowOperator.Context

Aljoscha Krettek-2
+Konstantin Knauf <[hidden email]> looping you in directly
because you used the "delete timer" feature in the past and even did some
changes to the timer system. Are you still relying on the fact that deleted
timers are actually deleted.

The main reason for wanting to get rid of delete timer is IMHO that
deleting a timer is difficult, depending on the data structure that you use
for timers. Especially if you want a data structure that can grow out of
core. By the way, the current data structure for timers is a Java Queue (a
heap) so deletes from this are O(n), i.e. possibly slow.

On Wed, 28 Sep 2016 at 15:21 Maximilian Michels <[hidden email]> wrote:

> What are the use cases where you actually need to delete a timer? How
>
> about we only let users delete timers which they created themselves?
>
>
>
> I guessing most of these use cases will be obsolete with the new
>
> Trigger DSL because the trigger logic can be expressed more easily. So
>
> +1 for removing the delete methods from the context.
>
>
>
> On Tue, Sep 27, 2016 at 3:43 PM, Kostas Kloudas
>
> <[hidden email]> wrote:
>
> > Hi all,
>
> >
>
> > As the title of this email suggests, I am proposing to remove the
> methods
>
> > deleteProcessingTimeTimer(long time) and deleteEventTimeTimer(long time)
>
> > from the WindowOperator.Context. With this change, registered timers that
>
> > have nothing to do (e.g. because their state has already been cleaned up)
>
> > will be simply ignored by the windowOperator, when their time comes.
>
> >
>
> > The reason for the change is that by allowing custom user code, e.g. a
> custom Trigger,
>
> > to delete timers we may have unpredictable behavior.
>
> >
>
> > As an example, one can imagine the case where we have allowed_lateness =
> 0 and the cleanup
>
> > timer for a window collides with the end_of_window one. In this case, by
> deleting the end_of_window
>
> > timer from the trigger (possibly a custom one), we end up also deleting
> the cleanup one,
>
> > which in turn can lead to the window state never being garbage collected.
>
> >
>
> > To see what can be the consequences apart from memory leaks, this can
> easily lead
>
> > to wrong session windows, as a session that should have been garbage
> collected, will
>
> > still be around and ready to accept new data.
>
> >
>
> > With this change, timers that should correctly be deleted will now
> remain in the queue of
>
> > pending timers, but they will do nothing, while cleanup timers will
> cleanup the state of their
>
> > corresponding window.
>
> >
>
> > Other possible solutions like keeping a separate list for cleanup timers
> would complicate
>
> > the codebase and also introduce memory overheads which can be avoided
> using the
>
> > solution above (i.e. just ignoring timers the have nothing to do
> anymore).
>
> >
>
> > What do you think?
>
> >
>
> > Kostas
>
> >
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Removing delete*Timer from the WindowOperator.Context

Kostas Kloudas
In reply to this post by mxm
Hi all,

This thread has been dormant for some time now.

Given that this change may affect user code, I am sending this as a
reminder that the discussion is still open and to re-invite anyone who
may be affected to participate.

I would suggest to leave it open till the end of next week and then,
if nobody objects, we can proceed to the change.

What do you think?

Kostas

> On Sep 28, 2016, at 3:21 PM, Maximilian Michels <[hidden email]> wrote:
>
> What are the use cases where you actually need to delete a timer? How
> about we only let users delete timers which they created themselves?
>
> I guessing most of these use cases will be obsolete with the new
> Trigger DSL because the trigger logic can be expressed more easily. So
> +1 for removing the delete methods from the context.
>
> On Tue, Sep 27, 2016 at 3:43 PM, Kostas Kloudas
> <[hidden email]> wrote:
>> Hi all,
>>
>> As the title of this email suggests, I am proposing to remove the  methods
>> deleteProcessingTimeTimer(long time) and deleteEventTimeTimer(long time)
>> from the WindowOperator.Context. With this change, registered timers that
>> have nothing to do (e.g. because their state has already been cleaned up)
>> will be simply ignored by the windowOperator, when their time comes.
>>
>> The reason for the change is that by allowing custom user code, e.g. a custom Trigger,
>> to delete timers we may have unpredictable behavior.
>>
>> As an example, one can imagine the case where we have allowed_lateness = 0 and the cleanup
>> timer for a window collides with the end_of_window one. In this case, by deleting the end_of_window
>> timer from the trigger (possibly a custom one), we end up also deleting the cleanup one,
>> which in turn can lead to the window state never being garbage collected.
>>
>> To see what can be the consequences apart from memory leaks, this can easily lead
>> to wrong session windows, as a session that should have been garbage collected, will
>> still be around and ready to accept new data.
>>
>> With this change, timers that should correctly be deleted will now remain in the queue of
>> pending timers, but they will do nothing, while cleanup timers will cleanup the state of their
>> corresponding window.
>>
>> Other possible solutions like keeping a separate list for cleanup timers would complicate
>> the codebase and also introduce memory overheads which can be avoided using the
>> solution above (i.e. just ignoring timers the have nothing to do anymore).
>>
>> What do you think?
>>
>> Kostas
>>

Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Removing delete*Timer from the WindowOperator.Context

Konstantin Knauf
Hi all,

thank you for looping me in. Because of the memory leak we first
experienced we have built a work-around, which did not need to delete
timers and are still using it. So for us, I think, this would currently
not be a problem. Nevertheless, I think, it is a strong limitation if
custom triggers can not delete timers. I am not familiar with the new
Trigger DSL though.

Cheers,

Konstantin

On 12.10.2016 15:38, Kostas Kloudas wrote:

> Hi all,
>
> This thread has been dormant for some time now.
>
> Given that this change may affect user code, I am sending this as a
> reminder that the discussion is still open and to re-invite anyone who
> may be affected to participate.
>
> I would suggest to leave it open till the end of next week and then,
> if nobody objects, we can proceed to the change.
>
> What do you think?
>
> Kostas
>
>> On Sep 28, 2016, at 3:21 PM, Maximilian Michels <[hidden email]> wrote:
>>
>> What are the use cases where you actually need to delete a timer? How
>> about we only let users delete timers which they created themselves?
>>
>> I guessing most of these use cases will be obsolete with the new
>> Trigger DSL because the trigger logic can be expressed more easily. So
>> +1 for removing the delete methods from the context.
>>
>> On Tue, Sep 27, 2016 at 3:43 PM, Kostas Kloudas
>> <[hidden email]> wrote:
>>> Hi all,
>>>
>>> As the title of this email suggests, I am proposing to remove the  methods
>>> deleteProcessingTimeTimer(long time) and deleteEventTimeTimer(long time)
>>> from the WindowOperator.Context. With this change, registered timers that
>>> have nothing to do (e.g. because their state has already been cleaned up)
>>> will be simply ignored by the windowOperator, when their time comes.
>>>
>>> The reason for the change is that by allowing custom user code, e.g. a custom Trigger,
>>> to delete timers we may have unpredictable behavior.
>>>
>>> As an example, one can imagine the case where we have allowed_lateness = 0 and the cleanup
>>> timer for a window collides with the end_of_window one. In this case, by deleting the end_of_window
>>> timer from the trigger (possibly a custom one), we end up also deleting the cleanup one,
>>> which in turn can lead to the window state never being garbage collected.
>>>
>>> To see what can be the consequences apart from memory leaks, this can easily lead
>>> to wrong session windows, as a session that should have been garbage collected, will
>>> still be around and ready to accept new data.
>>>
>>> With this change, timers that should correctly be deleted will now remain in the queue of
>>> pending timers, but they will do nothing, while cleanup timers will cleanup the state of their
>>> corresponding window.
>>>
>>> Other possible solutions like keeping a separate list for cleanup timers would complicate
>>> the codebase and also introduce memory overheads which can be avoided using the
>>> solution above (i.e. just ignoring timers the have nothing to do anymore).
>>>
>>> What do you think?
>>>
>>> Kostas
>>>
>
>
--
Konstantin Knauf * [hidden email] * +49-174-3413182
TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterföhring
Geschäftsführer: Henrik Klagges, Christoph Stock, Dr. Robert Dahlke
Sitz: Unterföhring * Amtsgericht München * HRB 135082


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] Removing delete*Timer from the WindowOperator.Context

Kostas Kloudas
Thanks for the feedback Konstantin!
Good to hear that.

As far as the Trigger DSL is concerned,
it is not currently in the master but it will come soon.

Kostas

> On Oct 12, 2016, at 6:05 PM, Konstantin Knauf <[hidden email]> wrote:
>
> Hi all,
>
> thank you for looping me in. Because of the memory leak we first
> experienced we have built a work-around, which did not need to delete
> timers and are still using it. So for us, I think, this would currently
> not be a problem. Nevertheless, I think, it is a strong limitation if
> custom triggers can not delete timers. I am not familiar with the new
> Trigger DSL though.
>
> Cheers,
>
> Konstantin
>
> On 12.10.2016 15:38, Kostas Kloudas wrote:
>> Hi all,
>>
>> This thread has been dormant for some time now.
>>
>> Given that this change may affect user code, I am sending this as a
>> reminder that the discussion is still open and to re-invite anyone who
>> may be affected to participate.
>>
>> I would suggest to leave it open till the end of next week and then,
>> if nobody objects, we can proceed to the change.
>>
>> What do you think?
>>
>> Kostas
>>
>>> On Sep 28, 2016, at 3:21 PM, Maximilian Michels <[hidden email]> wrote:
>>>
>>> What are the use cases where you actually need to delete a timer? How
>>> about we only let users delete timers which they created themselves?
>>>
>>> I guessing most of these use cases will be obsolete with the new
>>> Trigger DSL because the trigger logic can be expressed more easily. So
>>> +1 for removing the delete methods from the context.
>>>
>>> On Tue, Sep 27, 2016 at 3:43 PM, Kostas Kloudas
>>> <[hidden email]> wrote:
>>>> Hi all,
>>>>
>>>> As the title of this email suggests, I am proposing to remove the  methods
>>>> deleteProcessingTimeTimer(long time) and deleteEventTimeTimer(long time)
>>>> from the WindowOperator.Context. With this change, registered timers that
>>>> have nothing to do (e.g. because their state has already been cleaned up)
>>>> will be simply ignored by the windowOperator, when their time comes.
>>>>
>>>> The reason for the change is that by allowing custom user code, e.g. a custom Trigger,
>>>> to delete timers we may have unpredictable behavior.
>>>>
>>>> As an example, one can imagine the case where we have allowed_lateness = 0 and the cleanup
>>>> timer for a window collides with the end_of_window one. In this case, by deleting the end_of_window
>>>> timer from the trigger (possibly a custom one), we end up also deleting the cleanup one,
>>>> which in turn can lead to the window state never being garbage collected.
>>>>
>>>> To see what can be the consequences apart from memory leaks, this can easily lead
>>>> to wrong session windows, as a session that should have been garbage collected, will
>>>> still be around and ready to accept new data.
>>>>
>>>> With this change, timers that should correctly be deleted will now remain in the queue of
>>>> pending timers, but they will do nothing, while cleanup timers will cleanup the state of their
>>>> corresponding window.
>>>>
>>>> Other possible solutions like keeping a separate list for cleanup timers would complicate
>>>> the codebase and also introduce memory overheads which can be avoided using the
>>>> solution above (i.e. just ignoring timers the have nothing to do anymore).
>>>>
>>>> What do you think?
>>>>
>>>> Kostas
>>>>
>>
>>
>
> --
> Konstantin Knauf * [hidden email] * +49-174-3413182
> TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterföhring
> Geschäftsführer: Henrik Klagges, Christoph Stock, Dr. Robert Dahlke
> Sitz: Unterföhring * Amtsgericht München * HRB 135082
>