Hi Piotr, I'd +1 for this idea. I partially share the same concern with Arvid. Could we just use `org.apache.flink.util.clock.Clock` instead of introducing a new one? Thus only a new implementation is required IIUC. My thinking is that clock is a general need for timing. How to generate time or whether to pause is an implementation-related thing (e.g. org.apache.flink.util.clock.ManualClock). WDYT?
Best, Zakelly On Thu, Jul 25, 2024 at 9:02 PM Piotr Nowojski <piotr.nowoj...@gmail.com> wrote: > Hi Arvid, > > > 1. `RelativeClock` looks like a super-interface of > > `org.apache.flink.util.clock.Clock`. Should we also reflect that > > accordingly by extending? This should not break anything. > > It should be fine to do so, but to me the question is if we should do so? > It doesn't give any benefit right now, > if necessary either we or someone else can do that in the future. While in > the meantime it makes the APIs > somehow a bit more strict/more defined, so a bit higher chance of causing > some problems if we decide to > change something. > > > 2. Since `RelativeClock` is rather general, I'd also propose to change > the > > javadoc to not include the pausing part. > > My intention was not to enforce in the contract that progress of the timer > must be paused, but the interface > doesn't guarantee that the returned relativeTimeNanos value will follow the > wall clock 1 to 1. That concrete > classes can pause progress of that time if needed. I wanted to state that > clearly, to better highlight the > difference against the `Clock`. > > Maybe the java doc should be clarified? > > > 4. I now realized that I changed semantics compared to the proposal: this > > idle clock would already calculate the time difference (now - last > event). > > That may narrow down the usefulness but makes the only known use case > more > > explicit. > > > > Do we have other (future) use cases that could profit from having access > to > > the relative time of the last event occurring? Basically would we ever > need > > (now - some event) or (now - some periodic time)? > > I'm not sure. Idle clock seems a bit too specific to me. Also, at least > currently, idle time at the watermark > generator level is not calculated accurately. Exposing an approximate value > via API might make some > false impression that it's accurate? > > Best, > Piotrek > > śr., 24 lip 2024 o 15:56 Arvid Heise <ar...@apache.org> napisał(a): > > > Hi Piotr, > > > > thank you very much for addressing this issue. I'm convinced that the > > approach is the right solution also in contrast to the alternatives. > > Ultimately, only WatermarkGenratorWithIdleness needs to be adjusted with > > this change. > > > > My only concerns are regarding the actual code. > > 1. `RelativeClock` looks like a super-interface of > > `org.apache.flink.util.clock.Clock`. Should we also reflect that > > accordingly by extending? This should not break anything. > > 2. Since `RelativeClock` is rather general, I'd also propose to change > the > > javadoc to not include the pausing part. > > 3. Instead I'd reflect the pausing part in the accessor similarly to your > > proposal. Note the naming that is more specific > > > > /** > > * Returns a RelativeClock that represents the time that the > > respective input of this > > * WatermarkGenerator has been idle. This clock does not advance if > > the input is blocked by the > > * runtime (e.g., backpressure or watermark alignment cause the whole > > subtask to be blocked). > > * > > * @see RelativeClock > > */ > > RelativeClock getIdleClock(); > > > > 4. I now realized that I changed semantics compared to the proposal: this > > idle clock would already calculate the time difference (now - last > event). > > That may narrow down the usefulness but makes the only known use case > more > > explicit. > > > > Do we have other (future) use cases that could profit from having access > to > > the relative time of the last event occurring? Basically would we ever > need > > (now - some event) or (now - some periodic time)? > > > > Best, > > > > Arvid > > > > On Wed, Jul 24, 2024 at 3:01 PM Martijn Visser <martijnvis...@apache.org > > > > wrote: > > > > > Hi Piotr, > > > > > > We've talked offline about this proposal and I think it would be > > beneficial > > > for users to get this fixed. +1 overall, and thanks for writing it > down. > > > > > > Best regards, > > > > > > Martijn > > > > > > On Tue, Jul 23, 2024 at 5:45 PM Piotr Nowojski <pnowoj...@apache.org> > > > wrote: > > > > > > > Hi All, > > > > > > > > A bit unusual FLIP [1], as this is a bug fix for a problem that I > have > > > > recently discovered [2]. However I think FLIP is required, as > properly > > > > fixing the issue requires changes to the public API. > > > > > > > > As this is a bug fix, I would propose to back-port this change to > > > previous > > > > releases (1.19 and 1.20), despite this changing Public API. > > > > > > > > Best, > > > > Piotrek > > > > > > > > [1] https://cwiki.apache.org/confluence/x/oQvOEg > > > > [2] https://issues.apache.org/jira/browse/FLINK-35886 > > > > > > > > > >
