Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread Bill Farner 

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

Ship it!


This is now on master, thanks!

commit 4f6093b
Author: David McLaughlin 
Date:   Mon Aug 18 11:27:16 2014 -0700

Expand actions in JobUpdateAction

Reviewed at https://reviews.apache.org/r/24720/

- Bill Farner


On Aug. 18, 2014, 5:59 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 18, 2014, 5:59 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f669dbe472ea2171b0ac898b71626f0b25c0a9ec 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 78798f29bfbcf04b48953d050e498b3cad2248fb 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
>   src/test/resources/org/apache/aurora/gen/storage.thrift.md5 
> 45762990d33969bedde7340887cde16a535e99fe 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread David McLaughlin 

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

(Updated Aug. 18, 2014, 5:59 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Changes
---

rebase. (also, a reminder that I need someone to commit this for me.)


Repository: aurora


Description
---

Added some more items to the JobUpdateAction enum. Not married to the labels 
I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
finer-grained actions. 


Expected actions:

1) instance update: INSTANCE_UPDATED
2) instance remove: INSTANCE_REMOVED
3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
4) noop: INSTANCE_SKIPPED
5) instance rolled back: INSTANCE_ROLLED_BACK


Diffs (updated)
-

  src/main/thrift/org/apache/aurora/gen/api.thrift 
af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
  
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 
f669dbe472ea2171b0ac898b71626f0b25c0a9ec 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
78798f29bfbcf04b48953d050e498b3cad2248fb 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
  src/test/resources/org/apache/aurora/gen/storage.thrift.md5 
45762990d33969bedde7340887cde16a535e99fe 

Diff: https://reviews.apache.org/r/24720/diff/


Testing
---

./gradlew -Pq build


Thanks,

David McLaughlin

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread Bill Farner 

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

Ship it!


Ship It!

- Bill Farner


On Aug. 18, 2014, 5:49 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 18, 2014, 5:49 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread Maxim Khutornenko 

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

Ship it!


Ship It!

- Maxim Khutornenko


On Aug. 18, 2014, 5:49 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 18, 2014, 5:49 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread David McLaughlin 

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

(Updated Aug. 18, 2014, 5:49 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Changes
---

Failure states.


Repository: aurora


Description
---

Added some more items to the JobUpdateAction enum. Not married to the labels 
I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
finer-grained actions. 


Expected actions:

1) instance update: INSTANCE_UPDATED
2) instance remove: INSTANCE_REMOVED
3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
4) noop: INSTANCE_SKIPPED
5) instance rolled back: INSTANCE_ROLLED_BACK


Diffs (updated)
-

  src/main/thrift/org/apache/aurora/gen/api.thrift 
af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
  
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 
f695b85514bcc5cedb16e962124af3db052cb17a 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
21a05f6939da1dd7fc15cf6336bc3fee283f16ab 

Diff: https://reviews.apache.org/r/24720/diff/


Testing
---

./gradlew -Pq build


Thanks,

David McLaughlin

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread Bill Farner 


> On Aug. 16, 2014, 12:23 a.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 538
> > 
> >
> > What action is going to be used for an instance that failed to update 
> > and the job rollback is disabled?
> 
> David McLaughlin wrote:
> Sounds like we need another state for this. Do you have a suggestion? 
> INSTANCE_UPDATE_FAILED ?
> 
> Bill Farner wrote:
> INSTANCE_UPDATE_FAILED SGTM
> 
> Bill Farner wrote:
> And FWIW disabled rollback isn't the only case for this.  It can happen 
> any time max_failed_instances > 0.
> 
> Maxim Khutornenko wrote:
> Do we need a separate INSTANCE_ROLLBACK_FAILED state to set apart a 
> rollback failure? Otherwise, we may see two actions in the log: one from a 
> forward failure and another one from a rollback failure.

I'm ambivalent, and leave it up to what we want to do display-side.  I don't 
plan to have these entries influence the updater.


- Bill


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


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread Maxim Khutornenko 


> On Aug. 16, 2014, 12:23 a.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 538
> > 
> >
> > What action is going to be used for an instance that failed to update 
> > and the job rollback is disabled?
> 
> David McLaughlin wrote:
> Sounds like we need another state for this. Do you have a suggestion? 
> INSTANCE_UPDATE_FAILED ?
> 
> Bill Farner wrote:
> INSTANCE_UPDATE_FAILED SGTM
> 
> Bill Farner wrote:
> And FWIW disabled rollback isn't the only case for this.  It can happen 
> any time max_failed_instances > 0.

Do we need a separate INSTANCE_ROLLBACK_FAILED state to set apart a rollback 
failure? Otherwise, we may see two actions in the log: one from a forward 
failure and another one from a rollback failure.


- Maxim


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


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread Bill Farner 


> On Aug. 16, 2014, 12:23 a.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 538
> > 
> >
> > What action is going to be used for an instance that failed to update 
> > and the job rollback is disabled?
> 
> David McLaughlin wrote:
> Sounds like we need another state for this. Do you have a suggestion? 
> INSTANCE_UPDATE_FAILED ?
> 
> Bill Farner wrote:
> INSTANCE_UPDATE_FAILED SGTM

And FWIW disabled rollback isn't the only case for this.  It can happen any 
time max_failed_instances > 0.


- Bill


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


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread Bill Farner 


> On Aug. 16, 2014, 12:23 a.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 538
> > 
> >
> > What action is going to be used for an instance that failed to update 
> > and the job rollback is disabled?
> 
> David McLaughlin wrote:
> Sounds like we need another state for this. Do you have a suggestion? 
> INSTANCE_UPDATE_FAILED ?

INSTANCE_UPDATE_FAILED SGTM


- Bill


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


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread David McLaughlin 


> On Aug. 16, 2014, 12:23 a.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 538
> > 
> >
> > What action is going to be used for an instance that failed to update 
> > and the job rollback is disabled?

Sounds like we need another state for this. Do you have a suggestion? 
INSTANCE_UPDATE_FAILED ?


- David


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


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-18 Thread David McLaughlin 


> On Aug. 15, 2014, 6:02 p.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 558
> > 
> >
> > Rolling back assumes removing new (INSTANCE_REMOVED) and adding old 
> > (INSTANCE_ADDED). Do we want to distinct those actions from the forward 
> > roll?
> 
> David McLaughlin wrote:
> INSTANCE_REMOVED is only used when you remove an instance because the new 
> instanceCount is less than previous one, it's not used if you're removing an 
> instance as part of updating it.
> 
> Maxim Khutornenko wrote:
> I guess I did not see the JobUpdateAction as a post factum update story 
> teller. I thought it would be used more as a facilitator for internal actions 
> to be done (i.e. add_instance, kill_instance, and etc.) 
> 
> I don't have a problem with this high level mapping as long as updater 
> would not rely on it to drive the state machine progress.

We should be careful either way not to leak internal state machines into the 
public API. But yes, I think definitely the purpose of instance events is to 
retell a story of what happened during this update.


- David


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


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-15 Thread Maxim Khutornenko 

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



src/main/thrift/org/apache/aurora/gen/api.thrift


What action is going to be used for an instance that failed to update and 
the job rollback is disabled?


- Maxim Khutornenko


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-15 Thread Maxim Khutornenko 


> On Aug. 15, 2014, 6:02 p.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 558
> > 
> >
> > Rolling back assumes removing new (INSTANCE_REMOVED) and adding old 
> > (INSTANCE_ADDED). Do we want to distinct those actions from the forward 
> > roll?
> 
> David McLaughlin wrote:
> INSTANCE_REMOVED is only used when you remove an instance because the new 
> instanceCount is less than previous one, it's not used if you're removing an 
> instance as part of updating it.

I guess I did not see the JobUpdateAction as a post factum update story teller. 
I thought it would be used more as a facilitator for internal actions to be 
done (i.e. add_instance, kill_instance, and etc.) 

I don't have a problem with this high level mapping as long as updater would 
not rely on it to drive the state machine progress.


- Maxim


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


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-15 Thread David McLaughlin 


> On Aug. 15, 2014, 6:02 p.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 558
> > 
> >
> > Rolling back assumes removing new (INSTANCE_REMOVED) and adding old 
> > (INSTANCE_ADDED). Do we want to distinct those actions from the forward 
> > roll?

INSTANCE_REMOVED is only used when you remove an instance because the new 
instanceCount is less than previous one, it's not used if you're removing an 
instance as part of updating it.


- David


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


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-15 Thread Maxim Khutornenko 

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



src/main/thrift/org/apache/aurora/gen/api.thrift


Rolling back assumes removing new (INSTANCE_REMOVED) and adding old 
(INSTANCE_ADDED). Do we want to distinct those actions from the forward roll?


- Maxim Khutornenko


On Aug. 15, 2014, 5:52 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 15, 2014, 5:52 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Expected actions:
> 
> 1) instance update: INSTANCE_UPDATED
> 2) instance remove: INSTANCE_REMOVED
> 3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
> 4) noop: INSTANCE_SKIPPED
> 5) instance rolled back: INSTANCE_ROLLED_BACK
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  f695b85514bcc5cedb16e962124af3db052cb17a 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 21a05f6939da1dd7fc15cf6336bc3fee283f16ab 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-15 Thread David McLaughlin 

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

(Updated Aug. 15, 2014, 5:52 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Repository: aurora


Description (updated)
---

Added some more items to the JobUpdateAction enum. Not married to the labels 
I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
finer-grained actions. 


Expected actions:

1) instance update: INSTANCE_UPDATED
2) instance remove: INSTANCE_REMOVED
3) instance add: INSTANCE_ADDED -> INSTANCE_UPDATING -> INSTANCE_UPDATED
4) noop: INSTANCE_SKIPPED
5) instance rolled back: INSTANCE_ROLLED_BACK


Diffs (updated)
-

  src/main/thrift/org/apache/aurora/gen/api.thrift 
af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
  
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 
f695b85514bcc5cedb16e962124af3db052cb17a 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
ebcb9103d75909080f5b6a69db3a1bf46cfd9780 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
21a05f6939da1dd7fc15cf6336bc3fee283f16ab 

Diff: https://reviews.apache.org/r/24720/diff/


Testing (updated)
---

./gradlew -Pq build


Thanks,

David McLaughlin

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-14 Thread Bill Farner 

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


Also, while it might seem tedious, can you include in the description the 
events you would expect for the success and failure (revert) cases of:

1. instance update
2. instance remove
3. instance add
4. noop

This will help prove the enum is complete, and help me follow the same policy 
when implementing the backend.

- Bill Farner


On Aug. 14, 2014, 11:21 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 14, 2014, 11:21 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-14 Thread Maxim Khutornenko 

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


The ADD_INSTANCES is being used in quite a few places in unit tests. Please, 
update those and the thrift checksum.

- Maxim Khutornenko


On Aug. 14, 2014, 11:21 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 14, 2014, 11:21 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 24720: Expand actions in JobUpdateAction

2014-08-14 Thread Bill Farner 

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

Ship it!



src/main/thrift/org/apache/aurora/gen/api.thrift


Your call, but you might benefit from an INSTANCE_ROLLING_BACK value as 
well.


- Bill Farner


On Aug. 14, 2014, 11:21 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24720/
> ---
> 
> (Updated Aug. 14, 2014, 11:21 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added some more items to the JobUpdateAction enum. Not married to the labels 
> I've chosen, and I wasn't sure if we want to break out INSTANCE_UPDATING into 
> finer-grained actions. 
> 
> 
> Diffs
> -
> 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> af9f02ed1de487bc5cc2967d2edcece5b21e0be5 
> 
> Diff: https://reviews.apache.org/r/24720/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>