Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Aurora ReviewBot

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


Ship it!




Master (dde2c92) 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 July 28, 2016, 10:54 p.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 28, 2016, 10:54 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   RELEASE-NOTES.md 29d224d5596eb060356f1343cf347db79b1ad687 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/update.py 
> bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e157c0dfde5efc418448e138aa008ade742fe816 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afbd385b7eda64cb1f7d118b695e65e4045eac6c 
> 
> Diff: https://reviews.apache.org/r/50168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Igor Morozov
> 
>



Re: Review Request 50584: Upgrade to Mesos 1.0.0

2016-07-28 Thread Aurora ReviewBot

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


Ship it!




Master (dde2c92) 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 July 28, 2016, 9:18 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50584/
> ---
> 
> (Updated July 28, 2016, 9:18 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Steve Niemitz.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> I also took this as an opportunity to switch us from mesos.native to 
> mesos.executor on the Python side, meaning Docker containers will no longer 
> require all mesos deps.
> 
> Release notes here: http://mesos.apache.org/blog/mesos-1-0-0-released/
> Upgrade notes here: http://mesos.apache.org/documentation/latest/upgrades/
> 
> 
> Diffs
> -
> 
>   3rdparty/python/BUILD a44624c6cf3dcbdbfcc0504dd5e3dbcbfe6812de 
>   RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 
>   Vagrantfile 5aa2128b8645402b9892ee8dc17439bf58ea19ab 
>   build-support/packer/build.sh 08938ec13f3054c0c5ec6a75c1515f195533bc6a 
>   build-support/python/make-mesos-native-egg 
> 736685f9fc57b2d758edc752dc15bffb426a1879 
>   build.gradle 95401953c51dfabb9ce388d66b5aac109507f74c 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> e5bafa441cc848eb7eabdfe8c9e22dea3fac2ab7 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 8fe2b7997c0dfcbf9abdb467eea0fc768be6745c 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  e137daa2221c49e6fcd2b62b8ad0ccff1b8cb759 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 
>   src/main/python/apache/aurora/executor/BUILD 
> 0be65fc7c180d15f5a94bef268652ef7b868dcf7 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 0ef3856abc0df5403e3443ac35ba8d6940de8938 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  448d57a06b80fe1222abbc5149acf4e752d839a5 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 500fd435b4c72b25abd8df7eea6b3850edc96e99 
>   src/test/sh/org/apache/aurora/e2e/Dockerfile 
> 6fdea3d28760f59235c51c5b6913d2ee0172ef1a 
> 
> Diff: https://reviews.apache.org/r/50584/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> ran e2e tests.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread David McLaughlin


> On July 28, 2016, 8:44 p.m., David McLaughlin wrote:
> > For me ROLLED_BACK has a clear meaning - a job that was rolled back due to 
> > a failed health check in the update process. Overloading like this is going 
> > to lead to ambiguity and I wouldn't be comfortable exposing this 
> > functionality to users. 
> > 
> > Further to that, having to reinsert the code lock is basically a code smell 
> > that this approach is not what the current code was designed for. In fact 
> > this approach should have failed the unit tests because ROLLED_FORWARD (or 
> > any other terminal state) to ROLLING_BACK isn't an allowed state transition 
> > into the JobUpdateStateMachine: 
> > (https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java#L56).
> > 
> > It looks like a bug that only one of the changeUpdateStatus methods 
> > contains the transition check:
> > 
> > https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L404
> > 
> > I'm concerned that moving a previously terminal state into ROLLING_BACK 
> > creates a bunch of conceptual issues and complexity that will make the 
> > codebase (even) harder to understand. For example, throughout the code 
> > (also the UI code) we designate some states as terminal 
> > (https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L416)
> >  - but in the future we'll have to keep in mind that those states are 
> > TERMINAL_BUT_NOT_REALLY. ROLLED_BACK becomes the only true terminal state 
> > in this model. 
> > 
> > I'm not against reusing the rollback functionality built into the scheduler 
> > in the way you're doing, but maybe we should introduce a new STATE to 
> > reflect that it was user-initiated and add it to ALLOWED_TRANSITIONS? I'm 
> > not really familiar with this code, so maybe Bill or Maxim could speak to 
> > an approach (or dismiss my concerns ;)).
> 
> Igor Morozov wrote:
> Would not creating a new state imply the same level of conceptual 
> complexity? You would need to keep in mind not only to properly handle 
> terminal states but also a new rollbackable one? May be terminal states are 
> not being useful as a conocept as they are being used only here 
> https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L451
>  where update job locks being cleand up and it sounds like it should be 
> refactored away (see Maxim's comment). So what I'm trying to say it would not 
> be too much of a stratch to extend the semantic of rollbacks and terminal 
> states for update jobs.

They are only used there in the Scheduler, yes. But for things like hooks and 
other processes around update lifecycle, the idea that an update can "come back 
to life" breaks a lot of assumptions. The use of pulse in your diff is a great 
example. How is the service that was sending pulses for the original update 
supposed to know the update was revived? The service we use to send pulses 
needs some signal for when it can stop otherwise it would need to spam forever 
- it relies on the terminal states to do that. I see similar problems with the 
hooks in the CLI too. 

Again, maybe this is too specific to our tooling and others might disagree with 
my concerns. We can always just remove this feature from our CLI.


- David


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


On July 28, 2016, 10:54 p.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 28, 2016, 10:54 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   RELEASE-NOTES.md 29d224d5596eb060356f1343cf347db79b1ad687 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   

Re: Review Request 50480: Multiple executor support in Scheduler

2016-07-28 Thread Aurora ReviewBot

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


Ship it!




Master (dde2c92) 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 July 28, 2016, 10:35 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50480/
> ---
> 
> (Updated July 28, 2016, 10:35 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adds support for using multiple executors in a single scheduler.
> 
> Added warnings for pre-emption performance degradation when increasing 
> executor overhead.
> 
> Made executor config file require a JSON array.
> 
> Modified the way in which Thermos and any custom executors are loaded at 
> startup.
> 
> Thermos now always exists regardless of any other executors being used.
> 
> Jobs sent where no executor configuration is found for them are rejected.
> 
> Task prefix is now set from json file.
> 
> Fixed non revokable resources not being filtered out from overhead.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 
>   docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 
>   src/main/java/org/apache/aurora/GuavaUtils.java 
> 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> c43e04aee0df8988ed3af9d463dd6747d6631e3b 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  93ed3600268f231b0e53ceb6b3674ff742d94407 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorConfig.java
>  24329a49155b9e8f6d11dd1d09f6f5188d1e2ad9 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  8b7f8dcf4d8e0372208e636b3f57fc159272ecc7 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java
>  e919d3f2e2f86c26c0448029b06277a3a41a6941 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
>  b74edf46d35ac99164ec1bcf33edf36556baf9ed 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 
>   src/main/java/org/apache/aurora/scheduler/mesos/TestExecutorSettings.java 
> 1dfa97e659c2fca8308cb592f37d14328e4b42bc 
>   
> src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java
>  ee6fe95eaa41c7952a974ebea069b3de6ed82994 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 
> 3b84dbcfde9ae686110409173d742b3fa86ac764 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  147327036a872c9ccd03e17daaaf8cb1df489843 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoaderTest.java
>  7de7c46e6e1f478e25f7362d1d060b7f765dcb36 
>   
> src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
>  d34e12f6e0837fa43ea9b4e2a17db579e0ee10a2 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 500fd435b4c72b25abd8df7eea6b3850edc96e99 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java
>  7eb1714d14581a6ab25e85d36a1f3e973380c536 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java
>  fba427bd327e7f63b640c8b8753bfdeec3ee31e7 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> a79b0f413e603374347f8ce5765fb6e71bc9a5b5 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  c0fe84371ff2f9d47721126a9c356180f7c166de 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-missing-field.json
>  464617028b1e765a563a3e11f70d66089ede6866 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-multiple-executor.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-single-executor.json
>  PRE-CREATION 
>   
> src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-thermos-executor.json
>  114eb4f6c3eaeca3ad976e4907a3ec32c2339a09 
> 
> Diff: https://reviews.apache.org/r/50480/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> Created and killed jobs running a custom executor using a custom client.
> 
> Created and killed thermos jobs with the Aurora Client.
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Igor Morozov

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

(Updated July 28, 2016, 10:54 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
Zameer Manji.


Bugs: AURORA-1721
https://issues.apache.org/jira/browse/AURORA-1721


Repository: aurora


Description
---

Add rollback functionality to the scheduler

Thic change also includes an addition to storage replicated log. I needed a way 
to re-insert lock token for an existing job update.


Diffs (updated)
-

  CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
  RELEASE-NOTES.md 29d224d5596eb060356f1343cf347db79b1ad687 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
1d66208490aff6ea8af4c737845fa2cf13617529 
  api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
9e4213f13255a182df938bea44ca87fa03a25318 
  src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
52c3c6618a3cf1009435ca8a9cece36365913e55 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
39924c62108f6a68f545f90d77ceab4265d41fad 
  src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
d0de063fd78e6c4f62fae4a598d1d22f9775772d 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
b534abf95bab6e1657e3ef993cf34c0d6ec460be 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java 
9243c92b11040b68ed6014b3991db69fc08bcddf 
  src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
  
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
  src/main/python/apache/aurora/client/api/__init__.py 
68baf8fdb90cd26100159401c46c9963c24332b3 
  src/main/python/apache/aurora/client/cli/update.py 
bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
  src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
e157c0dfde5efc418448e138aa008ade742fe816 
  src/test/python/apache/aurora/client/api/test_scheduler_client.py 
afbd385b7eda64cb1f7d118b695e65e4045eac6c 

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


Testing
---


Thanks,

Igor Morozov



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Igor Morozov


> On July 28, 2016, 8:44 p.m., David McLaughlin wrote:
> > For me ROLLED_BACK has a clear meaning - a job that was rolled back due to 
> > a failed health check in the update process. Overloading like this is going 
> > to lead to ambiguity and I wouldn't be comfortable exposing this 
> > functionality to users. 
> > 
> > Further to that, having to reinsert the code lock is basically a code smell 
> > that this approach is not what the current code was designed for. In fact 
> > this approach should have failed the unit tests because ROLLED_FORWARD (or 
> > any other terminal state) to ROLLING_BACK isn't an allowed state transition 
> > into the JobUpdateStateMachine: 
> > (https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java#L56).
> > 
> > It looks like a bug that only one of the changeUpdateStatus methods 
> > contains the transition check:
> > 
> > https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L404
> > 
> > I'm concerned that moving a previously terminal state into ROLLING_BACK 
> > creates a bunch of conceptual issues and complexity that will make the 
> > codebase (even) harder to understand. For example, throughout the code 
> > (also the UI code) we designate some states as terminal 
> > (https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L416)
> >  - but in the future we'll have to keep in mind that those states are 
> > TERMINAL_BUT_NOT_REALLY. ROLLED_BACK becomes the only true terminal state 
> > in this model. 
> > 
> > I'm not against reusing the rollback functionality built into the scheduler 
> > in the way you're doing, but maybe we should introduce a new STATE to 
> > reflect that it was user-initiated and add it to ALLOWED_TRANSITIONS? I'm 
> > not really familiar with this code, so maybe Bill or Maxim could speak to 
> > an approach (or dismiss my concerns ;)).

Would not creating a new state imply the same level of conceptual complexity? 
You would need to keep in mind not only to properly handle terminal states but 
also a new rollbackable one? May be terminal states are not being useful as a 
conocept as they are being used only here 
https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L451
 where update job locks being cleand up and it sounds like it should be 
refactored away (see Maxim's comment). So what I'm trying to say it would not 
be too much of a stratch to extend the semantic of rollbacks and terminal 
states for update jobs.


- Igor


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


On July 28, 2016, 7:06 p.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 28, 2016, 7:06 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   

Re: Review Request 50480: Multiple executor support in Scheduler

2016-07-28 Thread Renan DelValle

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

(Updated July 28, 2016, 3:35 p.m.)


Review request for Aurora, Joshua Cohen and Maxim Khutornenko.


Changes
---

Fixed code formatting issues. Implemented feedback.

Task prefix is can now be set from the json config file.

Fixed non revokable resources not being filtered out from overhead.

@ReviewBot retry


Repository: aurora


Description (updated)
---

Adds support for using multiple executors in a single scheduler.

Added warnings for pre-emption performance degradation when increasing executor 
overhead.

Made executor config file require a JSON array.

Modified the way in which Thermos and any custom executors are loaded at 
startup.

Thermos now always exists regardless of any other executors being used.

Jobs sent where no executor configuration is found for them are rejected.

Task prefix is now set from json file.

Fixed non revokable resources not being filtered out from overhead.


Diffs (updated)
-

  RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 
  docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 
  src/main/java/org/apache/aurora/GuavaUtils.java 
8c2ab5720b808e5eae4c6b93fea8b787839b8dbe 
  src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
c43e04aee0df8988ed3af9d463dd6747d6631e3b 
  
src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
 93ed3600268f231b0e53ceb6b3674ff742d94407 
  
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorConfig.java
 24329a49155b9e8f6d11dd1d09f6f5188d1e2ad9 
  
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
 8b7f8dcf4d8e0372208e636b3f57fc159272ecc7 
  
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java
 e919d3f2e2f86c26c0448029b06277a3a41a6941 
  
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
 b74edf46d35ac99164ec1bcf33edf36556baf9ed 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 
  src/main/java/org/apache/aurora/scheduler/mesos/TestExecutorSettings.java 
1dfa97e659c2fca8308cb592f37d14328e4b42bc 
  
src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java 
ee6fe95eaa41c7952a974ebea069b3de6ed82994 
  src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 
3b84dbcfde9ae686110409173d742b3fa86ac764 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 147327036a872c9ccd03e17daaaf8cb1df489843 
  
src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoaderTest.java
 7de7c46e6e1f478e25f7362d1d060b7f765dcb36 
  
src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java 
d34e12f6e0837fa43ea9b4e2a17db579e0ee10a2 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
500fd435b4c72b25abd8df7eea6b3850edc96e99 
  
src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java
 7eb1714d14581a6ab25e85d36a1f3e973380c536 
  
src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java 
fba427bd327e7f63b640c8b8753bfdeec3ee31e7 
  src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
a79b0f413e603374347f8ce5765fb6e71bc9a5b5 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 c0fe84371ff2f9d47721126a9c356180f7c166de 
  
src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-missing-field.json
 464617028b1e765a563a3e11f70d66089ede6866 
  
src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-multiple-executor.json
 PRE-CREATION 
  
src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-single-executor.json
 PRE-CREATION 
  
src/test/resources/org/apache/aurora/scheduler/configuration/executor/test-thermos-executor.json
 114eb4f6c3eaeca3ad976e4907a3ec32c2339a09 

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


Testing (updated)
---

./build-support/jenkins/build.sh

./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh

Created and killed jobs running a custom executor using a custom client.

Created and killed thermos jobs with the Aurora Client.


Thanks,

Renan DelValle



Re: Review Request 50480: Multiple executor support in Scheduler

2016-07-28 Thread Renan DelValle


> On July 26, 2016, 8:25 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java, 
> > line 158
> > 
> >
> > Are we guaranteed that the executor config exists here? The previous 
> > branch is checking if executorConfig is set and that the config exists, but 
> > won't we fall into this branch if executorConfig is not set?
> 
> Renan DelValle wrote:
> Great catch! I forgot to put in some logic to handle the case where it's 
> a Docker task with no executor config.
> Iffy on how the code on this should look like without making it 
> convoluted because there are a couple of scenarios that need to be handled:
> 
> 1. Task with Docker container and with executor config -> must have a 
> valid config
> 2. Task without docker container and with executor config -> must have a 
> valid config
> 3. Task with Docker container and without executor config -> Accept
> 4. Task without docker container and without executor config -> Reject
> 
> Another question that comes to mind here is, if there is if there's a 
> task with a docker container but no executor config set, should the task be 
> allocated thermo's overhead or not.

Combined with one of Maxim's suggestions, came up with a way to do this. Fixed.


- Renan


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


On July 26, 2016, 7:04 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50480/
> ---
> 
> (Updated July 26, 2016, 7:04 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adds support for using multiple executors in a single scheduler.
> 
> Added warnings for pre-emption performance degradation when increasing 
> executor overhead.
> 
> Made executor config file require a JSON array.
> 
> Modified the way in which Thermos and any custom executors are loaded at 
> startup.
> 
> Thermos now always exists regardless of any other executors being used.
> 
> Jobs sent where no executor configuration is found for them are rejected.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 
>   docs/operations/configuration.md 0615c545f560597683f727c2425de4cc4e82c364 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> c43e04aee0df8988ed3af9d463dd6747d6631e3b 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  93ed3600268f231b0e53ceb6b3674ff742d94407 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
>  8b7f8dcf4d8e0372208e636b3f57fc159272ecc7 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java
>  e919d3f2e2f86c26c0448029b06277a3a41a6941 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoader.java
>  b74edf46d35ac99164ec1bcf33edf36556baf9ed 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 
>   src/main/java/org/apache/aurora/scheduler/mesos/TestExecutorSettings.java 
> 1dfa97e659c2fca8308cb592f37d14328e4b42bc 
>   
> src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java
>  ee6fe95eaa41c7952a974ebea069b3de6ed82994 
>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 
> 3b84dbcfde9ae686110409173d742b3fa86ac764 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  147327036a872c9ccd03e17daaaf8cb1df489843 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettingsLoaderTest.java
>  7de7c46e6e1f478e25f7362d1d060b7f765dcb36 
>   
> src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
>  d34e12f6e0837fa43ea9b4e2a17db579e0ee10a2 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> 500fd435b4c72b25abd8df7eea6b3850edc96e99 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java
>  7eb1714d14581a6ab25e85d36a1f3e973380c536 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java
>  fba427bd327e7f63b640c8b8753bfdeec3ee31e7 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> a79b0f413e603374347f8ce5765fb6e71bc9a5b5 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  c0fe84371ff2f9d47721126a9c356180f7c166de 
>   
> 

Re: Review Request 50480: Multiple executor support in Scheduler

2016-07-28 Thread Renan DelValle


> On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java,
> >  lines 205-208
> > 
> >
> > I'd drop this in favor of EMPTY overhead already returned by the method 
> > call below. It would leave ConfigurationManager and TaskScheduler to assert 
> > both new and existing task routes.

Sounds good to me, applied the change.


> On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorSettings.java,
> >  lines 33-34
> > 
> >
> > We usually avoid concrete collection types in publicly accessible 
> > methods/constructors. Accepting Map should be enough. 
> > 
> > If you worry about modifications (which does not seem to be the case 
> > here) you can do ImmutableMap.copyOf() in the assignment.

Makes sense, fixed.


> On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, 
> > lines 113-120
> > 
> >
> > How about moving EXECUTOR_PREFIX into ExecutorSettings and make it 
> > fully configurable instead? The default would be the current 'thermos-' 
> > value overriden by custom executor definition.

Great suggestion, implemented.


> On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, 
> > lines 162-164
> > 
> >
> > Is this reachable? Looks like you return Optional.fromNullable() from 
> > that method instead of throwing. 
> > 
> > How about 
> > `executorSettings.getExecutorOverhead(...).orElse(ResourceBag.EMPTY)` 
> > instead? Throwing exception this late in the scheduling cycle does not seem 
> > right anyway.

That sounds like a good idea. I agree that by this point in the pipeline, we 
should have validated everything.


> On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java,
> >  line 122
> > 
> >
> > This is actually a great finding! Should not be hard to fix it in this 
> > diff and avoid the TODO, e.g.:
> > ```
> > ResourceBag overhead = 
> > executorSettings.getExecutorOverhead(...).orElse(EMPTY);
> > if (tierManager.getTier(victim.getConfig()).isRevocable()) {
> >   bag = 
> > bag.filter(IS_MESOS_REVOCABLE.negate()).add(overhead.filter(IS_MESOS_REVOCABLE.negate()));
> > } else {
> >   bag.add(overhead);
> > }
> > ```

Meant to ask about this but forgot to. Glad I left a todo in there as a 
reminder. Fixed in this diff.


> On July 27, 2016, 1:47 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java, 
> > lines 139-148
> > 
> >
> > I suggest convert this block into an assert statement and move into 
> > current else block:
> > 
> > ```
> > if (!executorSettings.getExecutorConfig(...).isPresent()) {
> >   throw new IllegalStateException("Cannot find executor 
> > configuration...");
> > }
> > ```

Makes sense, changed, thanks for the suggestion. Wasn't sure if it was OK to 
throw an exception from here.


- Renan


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


On July 26, 2016, 7:04 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50480/
> ---
> 
> (Updated July 26, 2016, 7:04 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adds support for using multiple executors in a single scheduler.
> 
> Added warnings for pre-emption performance degradation when increasing 
> executor overhead.
> 
> Made executor config file require a JSON array.
> 
> Modified the way in which Thermos and any custom executors are loaded at 
> startup.
> 
> Thermos now always exists regardless of any other executors being used.
> 
> Jobs sent where no executor configuration is found for them are rejected.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 
>   docs/operations/configuration.md 

Review Request 50584: Upgrade to Mesos 1.0.0

2016-07-28 Thread Joshua Cohen

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

Review request for Aurora, Maxim Khutornenko and Steve Niemitz.


Repository: aurora


Description
---

I also took this as an opportunity to switch us from mesos.native to 
mesos.executor on the Python side, meaning Docker containers will no longer 
require all mesos deps.

Release notes here: http://mesos.apache.org/blog/mesos-1-0-0-released/
Upgrade notes here: http://mesos.apache.org/documentation/latest/upgrades/


Diffs
-

  3rdparty/python/BUILD a44624c6cf3dcbdbfcc0504dd5e3dbcbfe6812de 
  RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 
  Vagrantfile 5aa2128b8645402b9892ee8dc17439bf58ea19ab 
  build-support/packer/build.sh 08938ec13f3054c0c5ec6a75c1515f195533bc6a 
  build-support/python/make-mesos-native-egg 
736685f9fc57b2d758edc752dc15bffb426a1879 
  build.gradle 95401953c51dfabb9ce388d66b5aac109507f74c 
  src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
e5bafa441cc848eb7eabdfe8c9e22dea3fac2ab7 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
8fe2b7997c0dfcbf9abdb467eea0fc768be6745c 
  
src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 e137daa2221c49e6fcd2b62b8ad0ccff1b8cb759 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
cbbd6be94aa857b02cd7b45bfb2f0216d9a1cec3 
  src/main/python/apache/aurora/executor/BUILD 
0be65fc7c180d15f5a94bef268652ef7b868dcf7 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
0ef3856abc0df5403e3443ac35ba8d6940de8938 
  
src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
 448d57a06b80fe1222abbc5149acf4e752d839a5 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
500fd435b4c72b25abd8df7eea6b3850edc96e99 
  src/test/sh/org/apache/aurora/e2e/Dockerfile 
6fdea3d28760f59235c51c5b6913d2ee0172ef1a 

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


Testing
---

./build-support/jenkins/build.sh

ran e2e tests.


Thanks,

Joshua Cohen



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Zameer Manji

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



David makes a good comment. Maxim/Bill: What do you guys think about this 
change? I agree it's a bit of a code smell to do this without introducing a new 
state, but I don't think it's a serious problem.


CHANGELOG (line 7)


The CHANGELOG is machine generated.

Please undo the changes here and include a brief statement in 
`RELEASE-NOTES.md`.


- Zameer Manji


On July 28, 2016, 12:06 p.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 28, 2016, 12:06 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/update.py 
> bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e157c0dfde5efc418448e138aa008ade742fe816 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afbd385b7eda64cb1f7d118b695e65e4045eac6c 
> 
> Diff: https://reviews.apache.org/r/50168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Igor Morozov
> 
>



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread David McLaughlin

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



For me ROLLED_BACK has a clear meaning - a job that was rolled back due to a 
failed health check in the update process. Overloading like this is going to 
lead to ambiguity and I wouldn't be comfortable exposing this functionality to 
users. 

Further to that, having to reinsert the code lock is basically a code smell 
that this approach is not what the current code was designed for. In fact this 
approach should have failed the unit tests because ROLLED_FORWARD (or any other 
terminal state) to ROLLING_BACK isn't an allowed state transition into the 
JobUpdateStateMachine: 
(https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java#L56).

It looks like a bug that only one of the changeUpdateStatus methods contains 
the transition check:

https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L404

I'm concerned that moving a previously terminal state into ROLLING_BACK creates 
a bunch of conceptual issues and complexity that will make the codebase (even) 
harder to understand. For example, throughout the code (also the UI code) we 
designate some states as terminal 
(https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L416)
 - but in the future we'll have to keep in mind that those states are 
TERMINAL_BUT_NOT_REALLY. ROLLED_BACK becomes the only true terminal state in 
this model. 

I'm not against reusing the rollback functionality built into the scheduler in 
the way you're doing, but maybe we should introduce a new STATE to reflect that 
it was user-initiated and add it to ALLOWED_TRANSITIONS? I'm not really 
familiar with this code, so maybe Bill or Maxim could speak to an approach (or 
dismiss my concerns ;)).

- David McLaughlin


On July 28, 2016, 7:06 p.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 28, 2016, 7:06 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/update.py 
> bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e157c0dfde5efc418448e138aa008ade742fe816 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afbd385b7eda64cb1f7d118b695e65e4045eac6c 
> 
> Diff: https://reviews.apache.org/r/50168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Igor Morozov
> 
>



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Aurora ReviewBot

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


Ship it!




Master (dde2c92) 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 July 28, 2016, 7:06 p.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 28, 2016, 7:06 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/update.py 
> bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e157c0dfde5efc418448e138aa008ade742fe816 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afbd385b7eda64cb1f7d118b695e65e4045eac6c 
> 
> Diff: https://reviews.apache.org/r/50168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Igor Morozov
> 
>



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Igor Morozov


> On July 25, 2016, 7:57 p.m., Maxim Khutornenko wrote:
> > api/src/main/thrift/org/apache/aurora/gen/storage.thrift, lines 95-99
> > 
> >
> > The idea of update locks is entirely obsolete now that we don't have 
> > client updater. I'd favor a pre-requisite patch to remove lock support from 
> > job updates entirely and rely on the "one active update per job" invariant 
> > instead. 
> > 
> > Happy to shepherd that change if necessary.

I certainly see the point for removing lock tokens but it would make sense to 
remove them in a separate review. Hoping I'm not creating to much of a 
technical debt here.


- Igor


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


On July 28, 2016, 7:06 p.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 28, 2016, 7:06 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/update.py 
> bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e157c0dfde5efc418448e138aa008ade742fe816 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afbd385b7eda64cb1f7d118b695e65e4045eac6c 
> 
> Diff: https://reviews.apache.org/r/50168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Igor Morozov
> 
>



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Igor Morozov


> On July 21, 2016, 9:12 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java, 
> > lines 129-131
> > 
> >
> > +1
> > 
> > I'm fine with having this "undo" any of the existing updates.
> > 
> > I can imagine scenarios where a user of a cluster just wants to revert 
> > to a "last known good state" (ie they deploy bad code or configuration 
> > twice).

For us it is also the case when we cannot guarantee that the upgrade that is 
being rolled back is the latest one. I also updated docstring


> On July 21, 2016, 9:12 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java,
> >  lines 279-280
> > 
> >
> > +1
> > 
> > I think this can be removed, it seems to be a left over of a previous 
> > approach.

see the comment above. I'm fine with removing it as soon as we don't need it as 
additional verification step.


- Igor


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


On July 28, 2016, 7:06 p.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 28, 2016, 7:06 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/update.py 
> bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e157c0dfde5efc418448e138aa008ade742fe816 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afbd385b7eda64cb1f7d118b695e65e4045eac6c 
> 
> Diff: https://reviews.apache.org/r/50168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Igor Morozov
> 
>



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Igor Morozov

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

(Updated July 28, 2016, 7:06 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
Zameer Manji.


Bugs: AURORA-1721
https://issues.apache.org/jira/browse/AURORA-1721


Repository: aurora


Description
---

Add rollback functionality to the scheduler

Thic change also includes an addition to storage replicated log. I needed a way 
to re-insert lock token for an existing job update.


Diffs (updated)
-

  CHANGELOG fc6a46d77ebf889c8f60402c95e8a7472980ba1c 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
1d66208490aff6ea8af4c737845fa2cf13617529 
  api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
9e4213f13255a182df938bea44ca87fa03a25318 
  src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
52c3c6618a3cf1009435ca8a9cece36365913e55 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
39924c62108f6a68f545f90d77ceab4265d41fad 
  src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
d0de063fd78e6c4f62fae4a598d1d22f9775772d 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
b534abf95bab6e1657e3ef993cf34c0d6ec460be 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java 
9243c92b11040b68ed6014b3991db69fc08bcddf 
  src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
  
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
  src/main/python/apache/aurora/client/api/__init__.py 
68baf8fdb90cd26100159401c46c9963c24332b3 
  src/main/python/apache/aurora/client/cli/update.py 
bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
  src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
e157c0dfde5efc418448e138aa008ade742fe816 
  src/test/python/apache/aurora/client/api/test_scheduler_client.py 
afbd385b7eda64cb1f7d118b695e65e4045eac6c 

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


Testing
---


Thanks,

Igor Morozov



Re: Review Request 50168: Add rollback functionality to the scheduler

2016-07-28 Thread Igor Morozov


> On July 21, 2016, 4:05 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java,
> >  lines 279-280
> > 
> >
> > What is the purpose of this call if it's not using the return value? As 
> > far as I can tell there are no side effects from the call.
> 
> Igor Morozov wrote:
> This is additional verification step similar to start() function. I think 
> this step is not nessesary if 
> updateFactory.newUpdate(update.getInstructions(), true) == 
> updateFactory.newUpdate(update.getInstructions(), false). It was not clear 
> from a code that this is the case.
> 
> https://github.com/apache/aurora/blob/master/src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java#L162

I verified it is not needed, dropping...


- Igor


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


On July 21, 2016, 3:37 a.m., Igor Morozov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> ---
> 
> (Updated July 21, 2016, 3:37 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
> https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/update.py 
> bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e157c0dfde5efc418448e138aa008ade742fe816 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afbd385b7eda64cb1f7d118b695e65e4045eac6c 
> 
> Diff: https://reviews.apache.org/r/50168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Igor Morozov
> 
>



Re: Review Request 50530: AURORA-1656 Document tier concept

2016-07-28 Thread Stephan Erb

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


Ship it!




Ship It!

- Stephan Erb


On Juli 28, 2016, 12:41 vorm., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50530/
> ---
> 
> (Updated Juli 28, 2016, 12:41 vorm.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Stephan Erb.
> 
> 
> Bugs: AURORA-1656
> https://issues.apache.org/jira/browse/AURORA-1656
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1656 Document tier concept
> 
> 
> Diffs
> -
> 
>   docs/features/multitenancy.md 62bcd535d3bf39c9ea7e6d5958f6ae8ba0867c0d 
>   docs/reference/configuration.md 64c076d862453545652fbaa2d3e29f284ddd164d 
> 
> Diff: https://reviews.apache.org/r/50530/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>