You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@reef.apache.org by Sergiy Matusevych <se...@gmail.com> on 2016/08/12 01:29:47 UTC
A bug in Wake RuntimeClock.close()?
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.
Re: A bug in Wake RuntimeClock.close()?
Posted by Sergiy Matusevych <se...@gmail.com>.
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 <ta...@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.
>>>
>>>
>>
>
Re: A bug in Wake RuntimeClock.close()?
Posted by Sergiy Matusevych <se...@gmail.com>.
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 <ta...@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.
>>
>>
>
Re: A bug in Wake RuntimeClock.close()?
Posted by Sergiy Matusevych <se...@gmail.com>.
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 <ta...@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.
>
>
Re: A bug in Wake RuntimeClock.close()?
Posted by Tae-Geon Um <ta...@gmail.com>.
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 <se...@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.