reef-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sergiy Matusevych <sergiy.matusev...@gmail.com>
Subject Re: A bug in Wake RuntimeClock.close()?
Date Fri, 12 Aug 2016 18:22:47 GMT
Hi Taegeon,

Thanks for confirming that! I ran unit tests against the fixed version of
the clock last night, and they all pass. I'll do more testing today and
send a pull request.

Cheers,
Sergiy.


On Thu, Aug 11, 2016 at 10:04 PM, Tae-Geon Um <taegeonum@gmail.com> wrote:

> Hi Sergiy,
>
> For the Clock.close() method, the comment says :
> “This will stop the clock after all client alarms finish executing”
> (https://github.com/apache/reef/blob/master/lang/java/
> reef-wake/wake/src/main/java/org/apache/reef/wake/time/Clock.java#L54 <
> https://github.com/apache/reef/blob/master/lang/java/
> reef-wake/wake/src/main/java/org/apache/reef/wake/time/Clock.java#L54>)
>
> So, I think yes you’re right.
> We should not clear the client alarms before finishing them when the
> .close() is called.
>
> IMHO, it needs to be fixed :)
> Does anyone else have a different thought?
>
> Thanks,
> Taegeon
>
> > On Aug 12, 2016, at 10:29 AM, Sergiy Matusevych <
> sergiy.matusevych@gmail.com> wrote:
> >
> > Hi guys,
> >
> > I am looking at the Wake RuntimeClock implementation, and there is a
> piece
> > of code that puzzles me. In .close() method, i.e. when we are trying to
> > gracefully shutdown the clock, we search for the last scheduled client
> > alarm, and schedule StopTime event immediately after it:
> >
> > https://github.com/apache/reef/blob/master/lang/java/
> reef-wake/wake/src/main/java/org/apache/reef/wake/time/
> runtime/RuntimeClock.java#L140
> >
> > The problem is, in the line just before it, we *clear* the schedule, so
> > there won't be any client alarms when findAcceptableStopTime() tries to
> > find them, and the method would always return current time + 1.
> >
> > I don't think that this is the intended behavior, because then the whole
> > findAcceptableStopTime() method does not have much sense.
> >
> > What do you guys think? Do you want me to fix and test it? I am already
> in
> > the process of some cosmetic cleanups in that part of the codebase.
> >
> > Cheers,
> > Sergiy.
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message