You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Ben Mahler <be...@gmail.com> on 2015/04/16 00:47:48 UTC
Review Request 33243: Use explicit status update acknowledgements.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/
-----------------------------------------------------------
Review request for Aurora and Bill Farner.
Bugs: AURORA-1228
https://issues.apache.org/jira/browse/AURORA-1228
Repository: aurora
Description
-------
The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
Diff: https://reviews.apache.org/r/33243/diff/
Testing
-------
Ran the unit tests and the end-to-end test.
Thanks,
Ben Mahler
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Ben Mahler <be...@gmail.com>.
> On April 15, 2015, 10:49 p.m., Aurora ReviewBot wrote:
> > Master (b18dc44) is red with this patch.
> > ./build-support/jenkins/build.sh
> >
> > :compileJavaNote: Writing file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java:231: error: cannot find symbol
> > driver.acknowledgeStatusUpdate(status);
> > ^
> > symbol: method acknowledgeStatusUpdate(TaskStatus)
> > location: variable driver of type SchedulerDriver
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java:250: error: cannot find symbol
> > driver.acknowledgeStatusUpdate(status);
> > ^
> > symbol: method acknowledgeStatusUpdate(TaskStatus)
> > location: variable driver of type SchedulerDriver
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java:39: error: no suitable constructor found for MesosSchedulerDriver(Scheduler,FrameworkInfo,String,boolean,Credential)
> > return new MesosSchedulerDriver(scheduler, frameworkInfo, master, false, credentials.get());
> > ^
> > constructor MesosSchedulerDriver.MesosSchedulerDriver(Scheduler,FrameworkInfo,String,Credential) is not applicable
> > (actual and formal argument lists differ in length)
> > constructor MesosSchedulerDriver.MesosSchedulerDriver(Scheduler,FrameworkInfo,String) is not applicable
> > (actual and formal argument lists differ in length)
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java:41: error: no suitable constructor found for MesosSchedulerDriver(Scheduler,FrameworkInfo,String,boolean)
> > return new MesosSchedulerDriver(scheduler, frameworkInfo, master, false);
> > ^
> > constructor MesosSchedulerDriver.MesosSchedulerDriver(Scheduler,FrameworkInfo,String,Credential) is not applicable
> > (actual argument boolean cannot be converted to Credential by method invocation conversion)
> > constructor MesosSchedulerDriver.MesosSchedulerDriver(Scheduler,FrameworkInfo,String) is not applicable
> > (actual and formal argument lists differ in length)
> > 4 errors
> > FAILED
> >
> > FAILURE: Build failed with an exception.
> >
> > * What went wrong:
> > Execution failed for task ':compileJava'.
> > > Compilation failed; see the compiler error output for details.
> >
> > * Try:
> > Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
> >
> > BUILD FAILED
> >
> > Total time: 1 mins 21.608 secs
> >
> >
> > I will refresh this build result if you post a review containing "@ReviewBot retry"
Ah, I see reviewbot doesn't apply dependent patches.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review80276
-----------------------------------------------------------
On April 15, 2015, 10:47 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 10:47 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review80276
-----------------------------------------------------------
Master (b18dc44) is red with this patch.
./build-support/jenkins/build.sh
:compileJavaNote: Writing file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java:231: error: cannot find symbol
driver.acknowledgeStatusUpdate(status);
^
symbol: method acknowledgeStatusUpdate(TaskStatus)
location: variable driver of type SchedulerDriver
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java:250: error: cannot find symbol
driver.acknowledgeStatusUpdate(status);
^
symbol: method acknowledgeStatusUpdate(TaskStatus)
location: variable driver of type SchedulerDriver
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java:39: error: no suitable constructor found for MesosSchedulerDriver(Scheduler,FrameworkInfo,String,boolean,Credential)
return new MesosSchedulerDriver(scheduler, frameworkInfo, master, false, credentials.get());
^
constructor MesosSchedulerDriver.MesosSchedulerDriver(Scheduler,FrameworkInfo,String,Credential) is not applicable
(actual and formal argument lists differ in length)
constructor MesosSchedulerDriver.MesosSchedulerDriver(Scheduler,FrameworkInfo,String) is not applicable
(actual and formal argument lists differ in length)
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java:41: error: no suitable constructor found for MesosSchedulerDriver(Scheduler,FrameworkInfo,String,boolean)
return new MesosSchedulerDriver(scheduler, frameworkInfo, master, false);
^
constructor MesosSchedulerDriver.MesosSchedulerDriver(Scheduler,FrameworkInfo,String,Credential) is not applicable
(actual argument boolean cannot be converted to Credential by method invocation conversion)
constructor MesosSchedulerDriver.MesosSchedulerDriver(Scheduler,FrameworkInfo,String) is not applicable
(actual and formal argument lists differ in length)
4 errors
FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':compileJava'.
> Compilation failed; see the compiler error output for details.
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
BUILD FAILED
Total time: 1 mins 21.608 secs
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On April 15, 2015, 10:47 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 10:47 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review81747
-----------------------------------------------------------
Ship it!
Master (94fe6c9) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On April 27, 2015, 10:44 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 10:44 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Ben Mahler <be...@gmail.com>.
> On April 27, 2015, 11:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java, line 51
> > <https://reviews.apache.org/r/33243/diff/2/?file=943401#file943401line51>
> >
> > Remove `credentials.get()`, it will throw a NullPointerException in this branch.
Terrible, sorry for the oversight!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review81745
-----------------------------------------------------------
On April 27, 2015, 10:44 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 10:44 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review81745
-----------------------------------------------------------
LGTM, only comment is a ship blocker though.
src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java
<https://reviews.apache.org/r/33243/#comment132177>
Remove `credentials.get()`, it will throw a NullPointerException in this branch.
- Bill Farner
On April 27, 2015, 10:44 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 10:44 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review81753
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On April 27, 2015, 11:14 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 11:14 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review81750
-----------------------------------------------------------
Ship it!
Master (297c0eb) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On April 27, 2015, 11:14 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 11:14 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review81752
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java
<https://reviews.apache.org/r/33243/#comment132198>
One more comment for upstream here - using the Builder pattern to construct a MesosSchedulerDriver instead of overloaded constructors would allow all framework writers to avoid this.
- Kevin Sweeney
On April 27, 2015, 4:14 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 27, 2015, 4:14 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/
-----------------------------------------------------------
(Updated April 27, 2015, 11:14 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
Fixed a regresssion introduced in the latest diff.
Bugs: AURORA-1228
https://issues.apache.org/jira/browse/AURORA-1228
Repository: aurora
Description
-------
The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
Diff: https://reviews.apache.org/r/33243/diff/
Testing
-------
Ran the unit tests and the end-to-end test.
Thanks,
Ben Mahler
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/
-----------------------------------------------------------
(Updated April 27, 2015, 10:44 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Changes
-------
Kevin's comments.
Bugs: AURORA-1228
https://issues.apache.org/jira/browse/AURORA-1228
Repository: aurora
Description
-------
The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
Diff: https://reviews.apache.org/r/33243/diff/
Testing
-------
Ran the unit tests and the end-to-end test.
Thanks,
Ben Mahler
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Ben Mahler <be...@gmail.com>.
> On April 15, 2015, 11:06 p.m., Kevin Sweeney wrote:
> > Looks like you forgot to git add build.gradle?
This was just reviewbot not applying the dependent review, which has since been committed.
> On April 15, 2015, 11:06 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java, line 37
> > <https://reviews.apache.org/r/33243/diff/1/?file=931107#file931107line37>
> >
> > we typically will place a comment next to the parameter to indicate its name, but don't duplicate javadoc of APIs we're calling. In this case the comment is vague - consider annotating the parameter directly:
> >
> > ```java
> > return new MesosSchedulerDriver(
> > scheduler,
> > frameworkInfo,
> > master,
> > false /* disable implicit acknowledgements */);
> > ```
> >
> > instead. Also this probably escaped code review upstream but this would be a good place to use an enum instead of a boolean - consider filing an upstream ticket.
Sounds good. Enum would have been more explicit, given the effort needed for API / JNI changes, that ship has sailed. :)
> On April 15, 2015, 11:06 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 237
> > <https://reviews.apache.org/r/33243/diff/1/?file=931108#file931108line237>
> >
> > This comment is now a lie - we are sure we've NACKed the status update.
My bad, thanks for catching this! I've left it saying that we re-throw to abort the driver, since SchedulerException looks to be due to only unexpected errors?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review80278
-----------------------------------------------------------
On April 15, 2015, 10:47 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 10:47 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 33243: Use explicit status update acknowledgements.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33243/#review80278
-----------------------------------------------------------
Looks like you forgot to git add build.gradle?
src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java
<https://reviews.apache.org/r/33243/#comment130089>
we typically will place a comment next to the parameter to indicate its name, but don't duplicate javadoc of APIs we're calling. In this case the comment is vague - consider annotating the parameter directly:
```java
return new MesosSchedulerDriver(
scheduler,
frameworkInfo,
master,
false /* disable implicit acknowledgements */);
```
instead. Also this probably escaped code review upstream but this would be a good place to use an enum instead of a boolean - consider filing an upstream ticket.
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java
<https://reviews.apache.org/r/33243/#comment130090>
This comment is now a lie - we are sure we've NACKed the status update.
- Kevin Sweeney
On April 15, 2015, 3:47 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33243/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 3:47 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Bugs: AURORA-1228
> https://issues.apache.org/jira/browse/AURORA-1228
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The scheduler now explicitly acknowledges updates. I left the structure as is, per Bill's suggestion.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/DriverFactoryImpl.java db7aa74c2dfca3d520d978d7837d1f2639e6c05c
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f40746b5712f21e551c06eeb1aa379ebdcdbc693
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 4ef23ea6ebd2c9587a8356507fcb49a36b9de219
>
> Diff: https://reviews.apache.org/r/33243/diff/
>
>
> Testing
> -------
>
> Ran the unit tests and the end-to-end test.
>
>
> Thanks,
>
> Ben Mahler
>
>