+1 to remove this, I have not encountered such a strong case. best, JingsongLee
------------------------------------------------------------------ From:Kenneth Knowles <k...@google.com.INVALID> Time:2017 May 9 (Tue) 05:45 To:dev <dev@beam.apache.org> Subject:Re: [DISCUSS] Remove TimerInternals.deleteTimer(*) and Timer.cancel() Interesting! I believe the only thing we need to change to remove it for FSR is https://github.com/apache/beam/blob/master/sdks/java/core /src/main/java/org/apache/beam/sdk/state/Timer.java#L60 Here is how I might summarize the possibilities, at the risk of having something quite wrong: - Cancel as-is: some runners must manage a tombstone and/or timestamp in state or may choose to do so for performance. - Cancel requires a timestamp on the backend: runners must track the timestamp in state in order to cancel. Some runners may already need to track this for other reasons. For special cases like the end of the window or GC time it can be guessed, but those aren't user timers. - Cancel requires a timestamp from the user: Really weird. IMO implies a timer can be set multiple times and each one is independent. This is actually an increase in capability in perhaps an interesting way, but I thought it was too confusing. Also might have a wacky spec around the same timer set multiple times for the same timestamp (probably fine/idempotent) Technically, timers are marked `@Experimental`. But, given the interest in state and timers, making changes here will be very hard on users. Unless someone objects with a strong case, I am comfortable removing this from userland and potentially adding it later if there is demand. Kenn On Mon, May 8, 2017 at 3:26 AM, Aljoscha Krettek <aljos...@apache.org> wrote: > I wanted to bring this up before the First Stable release and see what > other people think. The methods I’m talking about are: > > void deleteTimer(StateNamespace namespace, String timerId, TimeDomain > timeDomain); > > @Deprecated > void deleteTimer(StateNamespace namespace, String timerId); > > @Deprecated > void deleteTimer(TimerData timerKey); > > The last two are slated for deletion. Notice that the first one doesn’t > take the timestamp of the timer to delete, which implies that there is only > one active timer per namespace/timerId/timeDomain. > > These are my motivations for removal: > - Timer removal is difficult and has varying levels of support on > different Runners and varying levels of cost. > - Removing the interface now and adding it back later is easy. Having it > in the FSR and later removing it is quite hard. > > I can only speak about the internal Flink implementation where Timers are > on a Heap (Java Priority Queue). Here removal is quite expensive, see, for > example this ML thread: https://lists.apache.org/thread.html/ > ac4d8e36360779a9b5047cf21303222980015720aac478e8cf632216@% > 3Cuser.flink.apache.org%3E. Especially this part: > > I thought I would drop my opinion here maybe it is relevant. > > We have used the Flink internal timer implementation in many of our > production applications, this supports the Timer deletion but the deletion > actually turned out to be a huge performance bottleneck because of the bad > deletion performance of the Priority queue. > > In many of our cases deletion could have been avoided by some more clever > registration/firing logic and we also ended up completely avoiding deletion > and instead using "tombstone" markers by setting a flag in the state which > timers not to fire when they actually trigger. > > Gyula > > Note that in Flink it’s not possible to delete a timer if you don’t know > the timestamp. Other systems might store timers in more elaborate data > structures (hierarchical timer wheels come to mind) where it’s also hard to > delete a timer without knowing the timestamp. > > If we decide to keep timer removal in the user API there’s the possibility > to simulate timer removal by keeping the timestamp of the currently active > timer for a given timerID and checking a firing timer against that. > > Best, > Aljoscha > > > > > >
