> 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 > >
