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 Sat, 13 Aug 2016 02:47:49 GMT
OK folks, here's a pull request to fix the issue + cleanup the code a bit:
https://github.com/apache/reef/pull/1096

Now after a fix and cleanup I feel like I want to write more unit tests for
RuntimeClock and related code. More Jiras and PRs will follow next week.

Cheers and have a great weekend,
Sergiy.

On Fri, Aug 12, 2016 at 12:46 PM, Sergiy Matusevych <
sergiy.matusevych@gmail.com> wrote:

> I've created a JIRA entry for the issue: https://issues.apache.
> org/jira/browse/REEF-1527
>
> Will send a pool request soon.
>
> Cheers,
> Sergiy.
>
> On Fri, Aug 12, 2016 at 11:22 AM, Sergiy Matusevych <
> sergiy.matusevych@gmail.com> wrote:
>
>> 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-w
>>> ake/wake/src/main/java/org/apache/reef/wake/time/Clock.java#L54 <
>>> https://github.com/apache/reef/blob/master/lang/java/reef-w
>>> ake/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-wa
>>> ke/wake/src/main/java/org/apache/reef/wake/time/runtime/Runt
>>> imeClock.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