Hi Piotr,
I'd prefer to change the semantics of the existing `Clock`, as forcing the
relative time and absolute time to run at the same speed limits the use of
this clock. Anyway, I have no strong feelings. I understand your concern
and introducing a new interface is also fine with me.
Best,
Zakelly
On Mon, Jul 29, 2024 at 6:16 PM Piotr Nowojski <pnowoj...@apache.org> wrote:
> Hi Arvid,
>
> > Yes, I think it would be sufficient to say that the relative time doesn't
> > need to move at the same speed as wall clock without going into details
> > such as backpressure (that should be moved to the doc of the WMContext).
>
> Makes sense. I've updated the FLIP. In the final PR I will try to phrase
> that
> better with a bit more detail.
>
> > I understand your concern and we can keep the original semantics. I'd
> still
> > like to see the name of the accessor to be a bit more specific for the
> task
> > at hand. RelativeClock is a very broad name because it's supposed to be
> > generic. Then we should have a specific name for the clock in the
> context,
> > such as getInputActivityClock or so.
>
> +1
>
> I like this name and I've updated the FLIP.
>
> Best,
> Piotrek
>
> pt., 26 lip 2024 o 14:47 Arvid Heise <ahe...@confluent.io.invalid>
> napisał(a):
>
> > Hi Piotr,
> >
> > > > 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.
> > One obvious advantage is that we could use ManualClock for testing. Other
> > than that, I'd be fine to leave as-is and adjust later.
> >
> > >
> > > > 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?
> > Yes, I think it would be sufficient to say that the relative time doesn't
> > need to move at the same speed as wall clock without going into details
> > such as backpressure (that should be moved to the doc of the WMContext).
> >
> > >
> > > > 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?
> > I understand your concern and we can keep the original semantics. I'd
> still
> > like to see the name of the accessor to be a bit more specific for the
> task
> > at hand. RelativeClock is a very broad name because it's supposed to be
> > generic. Then we should have a specific name for the clock in the
> context,
> > such as getInputActivityClock or so.
> >
> > Best,
> >
> > Arvid
> >
> >
> > On Fri, Jul 26, 2024 at 12:21 PM Piotr Nowojski <pnowoj...@apache.org>
> > wrote:
> >
> > > Hi Zakelly,
> > >
> > > > Could we just use `org.apache.flink.util.clock.Clock` instead of
> > > introducing a
> > > > new one?
> > >
> > > We can not use it as is, because of this java doc in it:
> > >
> > > * <h3>Relative Time</h3>
> > > *
> > > * <p>This time advances at the same speed as the <i>absolute time</i>,
> > but
> > > the timestamps can only
> > > * be referred to relative to each other. (...). Relative time behaves
> > > similar to {@link System#nanoTime()}.
> > >
> > > Also I don't feel like squeezing some artificial clocks, that blocks
> out
> > > part of the elapsed time, should be
> > > squeezed together with the regular clock that returns the absolute
> time.
> > It
> > > feels to me like it would be
> > > confusing to potential users. From this point of view, I like adding a
> > new
> > > dedicated interface that actually
> > > highlights this possibility.
> > >
> > > > How to generate time or whether to pause is an implementation-related
> > > thing (e.g.
> > > > org.apache.flink.util.clock.ManualClock)
> > >
> > > Actually it looks like that someone has thought it through when making
> > > ManualClock `@PublicEvolving`.
> > > I guess it exists because of unit tests and when `Clock` was being
> marked
> > > as `@PublicEvolving` someone
> > > either got carried away, or wanted to expose it to Flink users also for
> > the
> > > purpose of unit testing.
> > >
> > > Either way, I don't have a strong feeling. I would prefer to keep the
> new
> > > clock under a new interface. But
> > > we can also change the Java docs of the pre-existing `Clock`.
> > >
> > > Best,
> > > Piotrek
> > >
> > > pt., 26 lip 2024 o 10:51 Zakelly Lan <zakelly....@gmail.com>
> napisał(a):
> > >
> > > > 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
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
