aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 16054: Rewrite SchedulerLifecycle, employing a state machine.
Date Mon, 16 Dec 2013 22:30:29 GMT


> On Dec. 16, 2013, 5:22 a.m., Kevin Sweeney wrote:
> > src/main/java/com/twitter/aurora/scheduler/SchedulerLifecycle.java, line 328
> > <https://reviews.apache.org/r/16054/diff/4/?file=397801#file397801line328>
> >
> >     Is this true? Not clear to me how we get that from InterruptedException (and
if it is true why isn't it the same phrasing as above?)

At this point, all we know is that we don't know whether we advertised, so we behave pessimistically
and fail over.  The message is indeed unclear, changed.


> On Dec. 16, 2013, 5:22 a.m., Kevin Sweeney wrote:
> > src/main/java/com/twitter/aurora/scheduler/SchedulerLifecycle.java, lines 352-354
> > <https://reviews.apache.org/r/16054/diff/4/?file=397801#file397801line352>
> >
> >     You add e.toString() in one message, but not the other. Mind standardizing here?

Done.


> On Dec. 16, 2013, 5:22 a.m., Kevin Sweeney wrote:
> > src/main/java/com/twitter/aurora/scheduler/SchedulerLifecycle.java, line 359
> > <https://reviews.apache.org/r/16054/diff/4/?file=397801#file397801line359>
> >
> >     Should storage teardown be moved into the finally block? I don't see an obvious
reason to not attempt storage teardown when driver teardown fails.

This relates to the TODO.  Ideally, control.leave(), driver.stop(), storage.stop(), and lifecycle.shutdown()
would all be in a logical 'finally'.  However, i see lifecycle.shutdown() as the critical
one to call, and opted for this arrangement rather than a nesting of try/finally.


> On Dec. 16, 2013, 5:22 a.m., Kevin Sweeney wrote:
> > src/main/java/com/twitter/aurora/scheduler/SchedulerLifecycle.java, line 363
> > <https://reviews.apache.org/r/16054/diff/4/?file=397801#file397801line363>
> >
> >     Should storage teardown happen after this? AFAICT the thrift and UI components
can handle the case of the driver being shutdown gracefully (since they have to handle a disconnected
master anyway) and doing it in this order just generates ugly user-facing error messages.
But I might be missing something, and as this is the current behavior I'd be okay with a TODO
punting this to another review.

Leaving a TODO, as it's not obvious to me that one way is better than another.


> On Dec. 16, 2013, 5:22 a.m., Kevin Sweeney wrote:
> > src/test/java/com/twitter/aurora/scheduler/SchedulerLifecycleTest.java, lines 2-3
> > <https://reviews.apache.org/r/16054/diff/4/?file=397806#file397806line2>
> >
> >     Drop Twitter copyright

This matches all current copyright headers.  Punting for a separate effort for mass migration.


> On Dec. 16, 2013, 5:22 a.m., Kevin Sweeney wrote:
> > src/test/java/com/twitter/aurora/scheduler/SchedulerLifecycleTest.java, line 84
> > <https://reviews.apache.org/r/16054/diff/4/?file=397806#file397806line84>
> >
> >     Mind adding a brief prose description of what's expected here?

Done.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16054/#review30433
-----------------------------------------------------------


On Dec. 13, 2013, 8:09 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16054/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2013, 8:09 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This change addresses two issues:
>   - Ensure leadership is canceled whenever onDefeated is called
>   - Scheduler should wait for registered to be called before attempting to invoke driver
> 
> Some additional structural changes were made:
>   - Driver.run() is no longer used.  Instead, we invoke Driver.start() (non-blocking),
and the lifecycle uses Driver.join() to await exit.
>     This allows us to avoid jumping through thread-safety hoops in unit tests.
>   - A shim interface (DelayedActions) was added to SchedulerLifecycle to make testing
easier when capturing delayed closures.
> 
> 
> Diffs
> -----
> 
>   src/main/java/com/twitter/aurora/scheduler/Driver.java e8fe170b2d9e1a752b152cedc0e006f10eaa5a11

>   src/main/java/com/twitter/aurora/scheduler/SchedulerLifecycle.java 346d52acc5fc9e4841e2dc8b424fc6d46d2cdc8c

>   src/main/java/com/twitter/aurora/scheduler/SchedulerModule.java bd7929d631cf45b4c2c7f39177bbafbd8f659071

>   src/main/java/com/twitter/aurora/scheduler/app/SchedulerMain.java ec99e01dd4f982ed5b794ff86b65e6f58d2a0e27

>   src/main/java/com/twitter/aurora/scheduler/storage/testing/StorageTestUtil.java ceef9d3bd4b43d2dcac0ab9129ae2b624ab654cf

>   src/test/java/com/twitter/aurora/scheduler/DriverTest.java 5609b0b8b9a26a737558316e8e4fab0235704cb8

>   src/test/java/com/twitter/aurora/scheduler/SchedulerLifecycleTest.java PRE-CREATION

>   src/test/java/com/twitter/aurora/scheduler/app/SchedulerIT.java 4c381b946c8a3c7bbe1757d384d5d43dd74bb4d0

> 
> Diff: https://reviews.apache.org/r/16054/diff/
> 
> 
> Testing
> -------
> 
> gradle clean build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>


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