Re: Review Request 33243: Use explicit status update acknowledgements.

2015-04-27 Thread Bill Farner

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

2015-04-27 Thread Ben Mahler

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

2015-04-27 Thread Bill Farner

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

2015-04-27 Thread Aurora ReviewBot

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

2015-04-27 Thread Ben Mahler

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

2015-04-27 Thread Ben Mahler


 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.

2015-04-27 Thread Aurora ReviewBot

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

2015-04-15 Thread Kevin Sweeney

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




Re: Review Request 33243: Use explicit status update acknowledgements.

2015-04-15 Thread Aurora ReviewBot

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

2015-04-15 Thread Ben Mahler


 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