You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Luke Cwik <lc...@google.com> on 2020/09/18 22:14:26 UTC
[DISCUSS] Clearing timers (https://github.com/apache/beam/pull/12836)
PR 12836[1] is adding support for clearing timers and there is a discussion
about what the semantics for a cleared timer should be.
So far we have:
1) Clearing an unset timer is a no-op
2) If the last action on the timer was to clear it, then a future bundle
should not see it fire
Ambiguity occurs if the last action on a timer was to clear it within the
same bundle then should the current bundle not see it fire if it has yet to
become visible to the user? Since element processing and timer firings are
"unordered", this can happen.
Having the clear prevent the timer from firing within the same bundle if it
has yet to fire could make sense and simplifies clearing timer loops. For
example:
@ProcessElement
process(ProcessContext c) {
if (initialCondition) {
setTimer();
} else {
clearTimer();
}
}
@OnTimer
onTimer(...) {
do some side effect
set timer to fire again in the future
}
would require logic within the onTimer() method to check to see if we
should stop instead of relying on the fact that the clear will prevent the
timer loop.
On the other hand, we currently don't prevent timers from firing that are
eligible within the same bundle if their firing time is changed within the
bundle to some future time. Clearing timers could be treated conceptually
like setting them to "infinity" and hence the current set logic would
suggest that we shouldn't prevent timer firings that are part of the same
bundle.
Are there additional use cases that we should consider that suggest one
approach over the other?
What do people think?
1: https://github.com/apache/beam/pull/12836
Re: [DISCUSS] Clearing timers
(https://github.com/apache/beam/pull/12836)
Posted by Jan Lukavský <je...@seznam.cz>.
Big +1 to fixing this issue. Clearing & setting timer should IMHO follow
the same "happens-before" semantics. If timer is cleared before being
actually fired, then it should definitely not fire.
Jan
On 9/19/20 2:26 AM, Reuven Lax wrote:
> It appears that this PR tried to fix things for the Dataflow runner -
> https://github.com/apache/beam/pull/11924. It also ensure that if a
> timer fired in a bundle but was reset mid bundle to a later time mid
> bundle, then we will skip that timer firing.
>
> I believe this bug was also fixed in the direct runner previously. We
> probably still need to fix it for other runners and portability.
>
> Reuven
>
> On Fri, Sep 18, 2020 at 4:48 PM Boyuan Zhang <boyuanz@google.com
> <ma...@google.com>> wrote:
>
> Hi Reuven,
>
> Would you like to share the links to potential fixes? We can
> figure out what we can do there.
>
> On Fri, Sep 18, 2020 at 4:21 PM Reuven Lax <relax@google.com
> <ma...@google.com>> wrote:
>
>
>
> On Fri, Sep 18, 2020 at 3:14 PM Luke Cwik <lcwik@google.com
> <ma...@google.com>> wrote:
>
> PR 12836[1] is adding support for clearing timers and
> there is a discussion about what the semantics for a
> cleared timer should be.
>
> So far we have:
> 1) Clearing an unset timer is a no-op
> 2) If the last action on the timer was to clear it, then a
> future bundle should not see it fire
>
> Ambiguity occurs if the last action on a timer was to
> clear it within the same bundle then should the current
> bundle not see it fire if it has yet to become visible to
> the user? Since element processing and timer firings are
> "unordered", this can happen.
>
> Having the clear prevent the timer from firing within the
> same bundle if it has yet to fire could make sense and
> simplifies clearing timer loops. For example:
>
> |@ProcessElement process(ProcessContext c) { if
> (initialCondition) { setTimer(); } else { clearTimer(); }
> } @OnTimer onTimer(...) { do some side effect set timer to
> fire again in the future }|
>
> would require logic within the onTimer() method to check
> to see if we should stop instead of relying on the fact
> that the clear will prevent the timer loop.
>
> On the other hand, we currently don't prevent timers from
> firing that are eligible within the same bundle if their
> firing time is changed within the bundle to some future
> time. Clearing timers could be treated conceptually like
> setting them to "infinity" and hence the current set logic
> would suggest that we shouldn't prevent timer firings
> that are part of the same bundle.
>
>
> This "current" behavior is a bug, and one that has led to some
> weird effects. There have been some PRs attempting to fix it,
> and I think we should prioritize fixing this bug.
>
>
> Are there additional use cases that we should consider
> that suggest one approach over the other?
> What do people think?
>
> 1: https://github.com/apache/beam/pull/12836
>
Re: [DISCUSS] Clearing timers (https://github.com/apache/beam/pull/12836)
Posted by Reuven Lax <re...@google.com>.
It appears that this PR tried to fix things for the Dataflow runner -
https://github.com/apache/beam/pull/11924. It also ensure that if a timer
fired in a bundle but was reset mid bundle to a later time mid bundle, then
we will skip that timer firing.
I believe this bug was also fixed in the direct runner previously. We
probably still need to fix it for other runners and portability.
Reuven
On Fri, Sep 18, 2020 at 4:48 PM Boyuan Zhang <bo...@google.com> wrote:
> Hi Reuven,
>
> Would you like to share the links to potential fixes? We can figure out
> what we can do there.
>
> On Fri, Sep 18, 2020 at 4:21 PM Reuven Lax <re...@google.com> wrote:
>
>>
>>
>> On Fri, Sep 18, 2020 at 3:14 PM Luke Cwik <lc...@google.com> wrote:
>>
>>> PR 12836[1] is adding support for clearing timers and there is a
>>> discussion about what the semantics for a cleared timer should be.
>>>
>>> So far we have:
>>> 1) Clearing an unset timer is a no-op
>>> 2) If the last action on the timer was to clear it, then a future bundle
>>> should not see it fire
>>>
>>> Ambiguity occurs if the last action on a timer was to clear it within
>>> the same bundle then should the current bundle not see it fire if it has
>>> yet to become visible to the user? Since element processing and timer
>>> firings are "unordered", this can happen.
>>>
>>> Having the clear prevent the timer from firing within the same bundle if
>>> it has yet to fire could make sense and simplifies clearing timer loops.
>>> For example:
>>>
>>> @ProcessElement
>>> process(ProcessContext c) {
>>> if (initialCondition) {
>>> setTimer();
>>> } else {
>>> clearTimer();
>>> }
>>> }
>>>
>>> @OnTimer
>>> onTimer(...) {
>>> do some side effect
>>> set timer to fire again in the future
>>> }
>>>
>>> would require logic within the onTimer() method to check to see if we
>>> should stop instead of relying on the fact that the clear will prevent the
>>> timer loop.
>>>
>>> On the other hand, we currently don't prevent timers from firing that
>>> are eligible within the same bundle if their firing time is changed within
>>> the bundle to some future time. Clearing timers could be treated
>>> conceptually like setting them to "infinity" and hence the current set
>>> logic would suggest that we shouldn't prevent timer firings that are part
>>> of the same bundle.
>>>
>>
>> This "current" behavior is a bug, and one that has led to some weird
>> effects. There have been some PRs attempting to fix it, and I think we
>> should prioritize fixing this bug.
>>
>>
>>> Are there additional use cases that we should consider that suggest one
>>> approach over the other?
>>> What do people think?
>>>
>>> 1: https://github.com/apache/beam/pull/12836
>>>
>>
Re: [DISCUSS] Clearing timers (https://github.com/apache/beam/pull/12836)
Posted by Boyuan Zhang <bo...@google.com>.
Hi Reuven,
Would you like to share the links to potential fixes? We can figure out
what we can do there.
On Fri, Sep 18, 2020 at 4:21 PM Reuven Lax <re...@google.com> wrote:
>
>
> On Fri, Sep 18, 2020 at 3:14 PM Luke Cwik <lc...@google.com> wrote:
>
>> PR 12836[1] is adding support for clearing timers and there is a
>> discussion about what the semantics for a cleared timer should be.
>>
>> So far we have:
>> 1) Clearing an unset timer is a no-op
>> 2) If the last action on the timer was to clear it, then a future bundle
>> should not see it fire
>>
>> Ambiguity occurs if the last action on a timer was to clear it within the
>> same bundle then should the current bundle not see it fire if it has yet to
>> become visible to the user? Since element processing and timer firings are
>> "unordered", this can happen.
>>
>> Having the clear prevent the timer from firing within the same bundle if
>> it has yet to fire could make sense and simplifies clearing timer loops.
>> For example:
>>
>> @ProcessElement
>> process(ProcessContext c) {
>> if (initialCondition) {
>> setTimer();
>> } else {
>> clearTimer();
>> }
>> }
>>
>> @OnTimer
>> onTimer(...) {
>> do some side effect
>> set timer to fire again in the future
>> }
>>
>> would require logic within the onTimer() method to check to see if we
>> should stop instead of relying on the fact that the clear will prevent the
>> timer loop.
>>
>> On the other hand, we currently don't prevent timers from firing that are
>> eligible within the same bundle if their firing time is changed within the
>> bundle to some future time. Clearing timers could be treated conceptually
>> like setting them to "infinity" and hence the current set logic would
>> suggest that we shouldn't prevent timer firings that are part of the same
>> bundle.
>>
>
> This "current" behavior is a bug, and one that has led to some weird
> effects. There have been some PRs attempting to fix it, and I think we
> should prioritize fixing this bug.
>
>
>> Are there additional use cases that we should consider that suggest one
>> approach over the other?
>> What do people think?
>>
>> 1: https://github.com/apache/beam/pull/12836
>>
>
Re: [DISCUSS] Clearing timers (https://github.com/apache/beam/pull/12836)
Posted by Reuven Lax <re...@google.com>.
On Fri, Sep 18, 2020 at 3:14 PM Luke Cwik <lc...@google.com> wrote:
> PR 12836[1] is adding support for clearing timers and there is a
> discussion about what the semantics for a cleared timer should be.
>
> So far we have:
> 1) Clearing an unset timer is a no-op
> 2) If the last action on the timer was to clear it, then a future bundle
> should not see it fire
>
> Ambiguity occurs if the last action on a timer was to clear it within the
> same bundle then should the current bundle not see it fire if it has yet to
> become visible to the user? Since element processing and timer firings are
> "unordered", this can happen.
>
> Having the clear prevent the timer from firing within the same bundle if
> it has yet to fire could make sense and simplifies clearing timer loops.
> For example:
>
> @ProcessElement
> process(ProcessContext c) {
> if (initialCondition) {
> setTimer();
> } else {
> clearTimer();
> }
> }
>
> @OnTimer
> onTimer(...) {
> do some side effect
> set timer to fire again in the future
> }
>
> would require logic within the onTimer() method to check to see if we
> should stop instead of relying on the fact that the clear will prevent the
> timer loop.
>
> On the other hand, we currently don't prevent timers from firing that are
> eligible within the same bundle if their firing time is changed within the
> bundle to some future time. Clearing timers could be treated conceptually
> like setting them to "infinity" and hence the current set logic would
> suggest that we shouldn't prevent timer firings that are part of the same
> bundle.
>
This "current" behavior is a bug, and one that has led to some weird
effects. There have been some PRs attempting to fix it, and I think we
should prioritize fixing this bug.
> Are there additional use cases that we should consider that suggest one
> approach over the other?
> What do people think?
>
> 1: https://github.com/apache/beam/pull/12836
>
Re: [DISCUSS] Clearing timers (https://github.com/apache/beam/pull/12836)
Posted by Robert Bradshaw <ro...@google.com>.
Personally, I think that it makes sense to not see an old timer after one
has cleared (or moved) it. This is consistent with the idea that the
behavior should be the same if every element was processed in its own
bundle. The fact that a user can (sometimes, but not always) see an old
timer after they set a new one is leaking implementation details.
On Fri, Sep 18, 2020 at 3:14 PM Luke Cwik <lc...@google.com> wrote:
> PR 12836[1] is adding support for clearing timers and there is a
> discussion about what the semantics for a cleared timer should be.
>
> So far we have:
> 1) Clearing an unset timer is a no-op
> 2) If the last action on the timer was to clear it, then a future bundle
> should not see it fire
>
> Ambiguity occurs if the last action on a timer was to clear it within the
> same bundle then should the current bundle not see it fire if it has yet to
> become visible to the user? Since element processing and timer firings are
> "unordered", this can happen.
>
> Having the clear prevent the timer from firing within the same bundle if
> it has yet to fire could make sense and simplifies clearing timer loops.
> For example:
>
> @ProcessElement
> process(ProcessContext c) {
> if (initialCondition) {
> setTimer();
> } else {
> clearTimer();
> }
> }
>
> @OnTimer
> onTimer(...) {
> do some side effect
> set timer to fire again in the future
> }
>
> would require logic within the onTimer() method to check to see if we
> should stop instead of relying on the fact that the clear will prevent the
> timer loop.
>
> On the other hand, we currently don't prevent timers from firing that are
> eligible within the same bundle if their firing time is changed within the
> bundle to some future time. Clearing timers could be treated conceptually
> like setting them to "infinity" and hence the current set logic would
> suggest that we shouldn't prevent timer firings that are part of the same
> bundle.
>
> Are there additional use cases that we should consider that suggest one
> approach over the other?
> What do people think?
>
> 1: https://github.com/apache/beam/pull/12836
>