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 > > > > > >
