Re: Review Request 56048: Preemption performance improvement and new metrics release notes entry

[Aurora ReviewBot]

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



Master (a8afa59) is red with this patch.
  ./build-support/jenkins/build.sh

  Test coverage missing for org/apache/aurora/scheduler/SchedulerLifecycle$2
  Test coverage missing for org/apache/aurora/scheduler/SchedulerLifecycle$1
  Test coverage missing for org/apache/aurora/scheduler/HostOffer
  Test coverage missing for org/apache/aurora/scheduler/TaskVars
  Test coverage missing for 
org/apache/aurora/scheduler/SchedulerLifecycle$SchedulerCandidateImpl
  Test coverage missing for org/apache/aurora/scheduler/BatchWorker$Work
  Test coverage missing for org/apache/aurora/scheduler/TierInfo
  Test coverage missing for 
org/apache/aurora/scheduler/SchedulerLifecycle$DefaultDelayedActions
  Test coverage missing for 
org/apache/aurora/scheduler/TierManager$TierManagerImpl$TierConfig
  Test coverage missing for org/apache/aurora/scheduler/TaskVars$Counter
  Test coverage missing for org/apache/aurora/scheduler/TaskVars$1
  Test coverage missing for 
org/apache/aurora/scheduler/SchedulerModule$TaskEventBatchWorker
  Test coverage missing for org/apache/aurora/scheduler/BatchWorker
  Test coverage missing for org/apache/aurora/scheduler/HostOffer$1
  Test coverage missing for org/apache/aurora/scheduler/SchedulerModule
  Test coverage missing for 
org/apache/aurora/scheduler/TaskIdGenerator$TaskIdGeneratorImpl
  Test coverage missing for 
org/apache/aurora/scheduler/TierManager$TierManagerImpl
  Test coverage missing for org/apache/aurora/scheduler/SchedulerModule$1
  Test coverage missing for org/apache/aurora/scheduler/TaskStatusHandlerImpl
  Test coverage missing for org/apache/aurora/scheduler/SchedulerServicesModule
  Test coverage missing for org/apache/aurora/scheduler/TaskStatusHandlerImpl$1
  Test coverage missing for org/apache/aurora/scheduler/TierModule
  Test coverage missing for org/apache/aurora/scheduler/SchedulerLifecycle
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/ScheduleStatusTypeHandler
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/VolumeModeTypeHandler
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/MaintenanceModeTypeHandler
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/CronCollisionPolicyTypeHandler
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/AbstractTEnumTypeHandler
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler
  Test coverage missing for 
org/apache/aurora/scheduler/storage/db/typehandlers/ResourceTypeHandler

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.
==

BUILD FAILED

Total time: 3 mins 53.06 secs


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 27, 2017, 11:50 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56048/
> ---
> 
> (Updated Jan. 27, 2017, 11:50 p.m.)
> 
> 
> Review request for Aurora and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Preemption performance improvement and new metrics release notes entry
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 7d01c90610e2cddf0f0629af669fd3bdb2afa7c5 
> 
> Diff: https://reviews.apache.org/r/56048/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Review Request 56048: Preemption performance improvement and new metrics release notes entry

[Mehrdad Nurolahzade]

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

Review request for Aurora and Stephan Erb.


Repository: aurora


Description
---

Preemption performance improvement and new metrics release notes entry


Diffs
-

  RELEASE-NOTES.md 7d01c90610e2cddf0f0629af669fd3bdb2afa7c5 

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


Testing
---


Thanks,

Mehrdad Nurolahzade

Re: Review Request 55089: AURORA-1826 Expose Thrift server request workload stats

[Stephan Erb]

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


Ship it!




LGTM


src/main/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptor.java
 (line 41)


now that I see this...



src/main/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptor.java
 (line 51)


... should we use `scheduler_` as a prefix here as well?



src/main/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptor.java
 (lines 65 - 72)


Can `invocation.proceed();` ever return `null`?

If not, then we could move the new counter code from the `finally` to the 
`try` block and eliminate the `response != null` guard.


L

- Stephan Erb


On Jan. 26, 2017, 11:07 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55089/
> ---
> 
> (Updated Jan. 26, 2017, 11:07 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Stephan Erb.
> 
> 
> Bugs: AURORA-1826
> https://issues.apache.org/jira/browse/AURORA-1826
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1826   Expose Thrift server request workload stats
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  16b1b52f8691d978a9ec1bf7aa0c9716b3484cf0 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptor.java
>  d57f910d8f9bbe5c24aec960e88d03702bc353da 
>   src/main/java/org/apache/aurora/scheduler/thrift/aop/ThriftWorkload.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  b28cd2489a52041a8e7e53f298fad8d8cd29406f 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  9c40ec51c28c8c57365dc21c3cd7391a3894784c 
> 
> Diff: https://reviews.apache.org/r/55089/diff/
> 
> 
> Testing
> ---
> 
> ```
> curl 192.168.33.7:8081/vars | grep thrift_workload
>   % Total% Received % Xferd  Average Speed   TimeTime Time  
> Current
>  Dload  Upload   Total   SpentLeft  Speed
> 100 413340 413340 0  3695k  0 --:--:-- --:--:-- --:--:-- 4036k
> thrift_workload_addInstances 0
> thrift_workload_createJob 0
> thrift_workload_createOrUpdateCronTemplate 0
> thrift_workload_drainHosts 0
> thrift_workload_endMaintenance 0
> thrift_workload_getConfigSummary 0
> thrift_workload_getJobSummary 0
> thrift_workload_getJobUpdateDetails 0
> thrift_workload_getJobUpdateSummaries 0
> thrift_workload_getJobs 0
> thrift_workload_getPendingReason 0
> thrift_workload_getRoleSummary 0
> thrift_workload_getTaskStatus 0
> thrift_workload_getTasksWithoutConfigs 0
> thrift_workload_killTasks 0
> thrift_workload_maintenanceStatus 0
> thrift_workload_restartShards 0
> thrift_workload_rewriteConfigs 0
> thrift_workload_startJobUpdate 0
> thrift_workload_startMaintenance 0
> ```
> 
> ```
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> ...
> 
> *** OK (All tests passed) ***
> 
> mesos-master start/running, process 2359
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> real  28m58.389s
> user  0m1.508s
> sys   0m0.820s
> ```
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 55982: Move deprecated resource validations so they happen after the thrift backfill

[Stephan Erb]

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


Ship it!




LGTM


src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
 (line 297)


This message is now outdated and can be dropped.


- Stephan Erb


On Jan. 26, 2017, 4:16 p.m., Nicolás Donatucci wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55982/
> ---
> 
> (Updated Jan. 26, 2017, 4:16 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zameer Manji.
> 
> 
> Bugs: AURORA-1707
> https://issues.apache.org/jira/browse/AURORA-1707
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As the validations for NumCpus, RamMb and DiskMb happened before the thrift 
> backfill, those values needed to be set, even though they are deprecated. In 
> the thrift backfill, if the Resources field is set, then NumCpus, RamMb and 
> DiskMb are set accordingly. 
> 
> So by moving those validations, it is now possible to only set the Resources 
> field instead of having to set the deprecated fields. As the validations are 
> moved and not removed, the ckeck for the resource values being greater than 0 
> still happens. Furthermore, if the Resources field is set but there is no 
> Resource for Ram in the set, the thrift backfill will throw an 
> IllegalArgumentException.
> 
> Some tests were slightly modified because of this, mostly by adding an 
> unsetResources() operation. This is because as the validations now happen 
> after the thrift backfill, during the thrift backfill the values in the 
> deprecated fields are replaced by those in the Resources field (if it is 
> set). There are also some new tests.
> 
> Related Issue: AURORA-1707
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  f96cd7a8eba12c286ac4e104a22ae74d8d4108d7 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  6d0e9bc6a8040393875d4f0a88e8db9d6926a88b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  b28cd2489a52041a8e7e53f298fad8d8cd29406f 
> 
> Diff: https://reviews.apache.org/r/55982/diff/
> 
> 
> Testing
> ---
> 
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Nicolás Donatucci
> 
>

Re: Review Request 54883: Move snapshots into a separate log

[Stephan Erb]

> On Dec. 27, 2016, 11:50 p.m., Joshua Cohen wrote:
> > Overall this looks good to me. I think some work would be required to 
> > productionize it (e.g. make it optional to start). Obviously want to vet 
> > this in a test cluster and we would need some doc changes to go with this 
> > and RELEASE-NOTES.md, etc. but that's easy enough to take care of.
> > 
> > I also want to be sure I understand the behavior in the event of a problem 
> > with one, or both replicated logs. So... scenarios:
> > 
> > 1) One log or the other is missing/corrupt: our state becomes the sum of 
> > whatever's found in the log that's present/valid.
> > 2) There's no chance of a snapshot being persisted that references a log 
> > position that hasn't been persisted to the operation log, right? E.g. 
> > there's no realistic way that appending the noop transaction could take 
> > longer than creating/persisting the snapshot (in which case the snapshot 
> > could be successfully persisted referencing that position, yet that 
> > position would not exist in the operation log).
> 
> David McLaughlin wrote:
> 1) It's exactly the same as today if the log is corrupt - you need to 
> restore from a backup. 
> 2) No, that is not possible. Log appends are synchronous. 
> 
> I'm -1 to making this optional, purely because that doesn't reduce the 
> risk in any way - you'd just be putting off adding flags to your start-up 
> script. But yeah, it means I'll have to vet this change in a production-like 
> environment before this gets committed.
> 
> Joshua Cohen wrote:
> I'd like to hear what others running Aurora in production think about 
> putting this behind a flag. This feels like a big change to force on people 
> and I think if this came from someone outside of our org we'd like the 
> flexibility to enable this on our own timeframe rather than being blocked 
> from picking up all updates until we were comfortable/ready.
> 
> Stephan Erb wrote:
> Most people (us included) only deploy major versions. If we ship this in 
> 0.17 there will be many unrelated changes that might cause trouble and 
> require a rollback. So, even if this change is OK, people could be bitten by 
> other changes in the same release.
> 
> I therefore agree with Joshua here that we need a way to make this 
> tansition smoother.
> 
> David McLaughlin wrote:
> A feature flag still doesn't solve that problem. You're just punting the 
> migration to another release that will also have commits that might cause a 
> rollback.

I still believe that a feature toggle would be helpful. I don't fear that this 
particular patch is broken, but rather that any of the hundreds of other 
changes within the release may come with a regression. Decoupling the backwards 
compatible from the incompatible parts just makes sure I can test the former 
before having to deal with the latter.  

An alternative solution would be to put this patch into a release of its own.


- Stephan


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


On Dec. 22, 2016, 10:26 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54883/
> ---
> 
> (Updated Dec. 22, 2016, 10:26 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, John Sirois, Santhosh Kumar 
> Shanmugham, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> See here for more details: 
> https://docs.google.com/document/d/1QVSEfeoCyt2D6cCmTCxy8-epufcuzIfnqRUkyT1betY/edit?usp=sharing
> 
> The motivation for this patch is the realisation that if we store Snapshots 
> outside of the main transaction log, we only ever to need to hold the storage 
> lock during snapshot creation. This would reduce the time writes are blocked 
> during snapshots by 80% for a large production cluster. I'm also doing this 
> with a view to future work, which is described in the document above. 
> 
> As long as everything works fine, this change is as simple as adding two new 
> args to the scheduler JVM. But it's worth noting that once this change is 
> deployed and the first snapshot is written, you will no longer be able to 
> rollback without first restoring from a backup.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   examples/vagrant/aurorabuild.sh dbec54d3e677db8cb0f02849e5373e619629fc7c 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> e68a801017ae02e0ed581129e12a645ccc1e35d4 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 43cc5b4645bc26b0fc6b23726ad3292699048ded 
>   src/main/java/org/apache/aurora/scheduler/log/Log.ja

Re: Review Request 55982: Move deprecated resource validations so they happen after the thrift backfill

[Aurora ReviewBot]

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


Ship it!




Master (a8afa59) 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 Jan. 26, 2017, 3:16 p.m., Nicolás Donatucci wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55982/
> ---
> 
> (Updated Jan. 26, 2017, 3:16 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zameer Manji.
> 
> 
> Bugs: AURORA-1707
> https://issues.apache.org/jira/browse/AURORA-1707
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As the validations for NumCpus, RamMb and DiskMb happened before the thrift 
> backfill, those values needed to be set, even though they are deprecated. In 
> the thrift backfill, if the Resources field is set, then NumCpus, RamMb and 
> DiskMb are set accordingly. 
> 
> So by moving those validations, it is now possible to only set the Resources 
> field instead of having to set the deprecated fields. As the validations are 
> moved and not removed, the ckeck for the resource values being greater than 0 
> still happens. Furthermore, if the Resources field is set but there is no 
> Resource for Ram in the set, the thrift backfill will throw an 
> IllegalArgumentException.
> 
> Some tests were slightly modified because of this, mostly by adding an 
> unsetResources() operation. This is because as the validations now happen 
> after the thrift backfill, during the thrift backfill the values in the 
> deprecated fields are replaced by those in the Resources field (if it is 
> set). There are also some new tests.
> 
> Related Issue: AURORA-1707
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  f96cd7a8eba12c286ac4e104a22ae74d8d4108d7 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  6d0e9bc6a8040393875d4f0a88e8db9d6926a88b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  b28cd2489a52041a8e7e53f298fad8d8cd29406f 
> 
> Diff: https://reviews.apache.org/r/55982/diff/
> 
> 
> Testing
> ---
> 
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Nicolás Donatucci
> 
>

Re: Review Request 55982: Move deprecated resource validations so they happen after the thrift backfill

[Stephan Erb]

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



@ReviewBot retry

- Stephan Erb


On Jan. 26, 2017, 4:16 p.m., Nicolás Donatucci wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55982/
> ---
> 
> (Updated Jan. 26, 2017, 4:16 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zameer Manji.
> 
> 
> Bugs: AURORA-1707
> https://issues.apache.org/jira/browse/AURORA-1707
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As the validations for NumCpus, RamMb and DiskMb happened before the thrift 
> backfill, those values needed to be set, even though they are deprecated. In 
> the thrift backfill, if the Resources field is set, then NumCpus, RamMb and 
> DiskMb are set accordingly. 
> 
> So by moving those validations, it is now possible to only set the Resources 
> field instead of having to set the deprecated fields. As the validations are 
> moved and not removed, the ckeck for the resource values being greater than 0 
> still happens. Furthermore, if the Resources field is set but there is no 
> Resource for Ram in the set, the thrift backfill will throw an 
> IllegalArgumentException.
> 
> Some tests were slightly modified because of this, mostly by adding an 
> unsetResources() operation. This is because as the validations now happen 
> after the thrift backfill, during the thrift backfill the values in the 
> deprecated fields are replaced by those in the Resources field (if it is 
> set). There are also some new tests.
> 
> Related Issue: AURORA-1707
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  f96cd7a8eba12c286ac4e104a22ae74d8d4108d7 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  6d0e9bc6a8040393875d4f0a88e8db9d6926a88b 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  b28cd2489a52041a8e7e53f298fad8d8cd29406f 
> 
> Diff: https://reviews.apache.org/r/55982/diff/
> 
> 
> Testing
> ---
> 
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Nicolás Donatucci
> 
>

Re: Review Request 54754: Fixed starting cron jobs when using default_docker_parameters

[Steve Niemitz]

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



Wow I forgot I even submitted this!

- Steve Niemitz


On Dec. 15, 2016, 5:48 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54754/
> ---
> 
> (Updated Dec. 15, 2016, 5:48 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1684
> https://issues.apache.org/jira/browse/AURORA-1684
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fixed starting cron jobs when using default_docker_parameters
> 
> The code was previously attempting to re-sanitize the configuration read from 
> storage rather than just using it as is.  This causes issues if after 
> sanitization the job no longer passes sanitization (which is the case here w/ 
> default_docker_parameters).
> 
> We've been running this in our branch forever.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 
> 5873983479c39a011eb363f7fd442867f0794b17 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java 
> 650facecc2e02be7bb3cd5ea9ff0f094e006bcb3 
>   
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java 
> fb06c282c4e94639c521658682c5da3e4dd4baf7 
> 
> Diff: https://reviews.apache.org/r/54754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 54754: Fixed starting cron jobs when using default_docker_parameters

[Joshua Cohen]

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


Ship it!




Ship It!

- Joshua Cohen


On Dec. 15, 2016, 5:48 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54754/
> ---
> 
> (Updated Dec. 15, 2016, 5:48 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1684
> https://issues.apache.org/jira/browse/AURORA-1684
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fixed starting cron jobs when using default_docker_parameters
> 
> The code was previously attempting to re-sanitize the configuration read from 
> storage rather than just using it as is.  This causes issues if after 
> sanitization the job no longer passes sanitization (which is the case here w/ 
> default_docker_parameters).
> 
> We've been running this in our branch forever.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 
> 5873983479c39a011eb363f7fd442867f0794b17 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java 
> 650facecc2e02be7bb3cd5ea9ff0f094e006bcb3 
>   
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java 
> fb06c282c4e94639c521658682c5da3e4dd4baf7 
> 
> Diff: https://reviews.apache.org/r/54754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 54754: Fixed starting cron jobs when using default_docker_parameters

[Stephan Erb]

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


Ship it!




Ship It!

- Stephan Erb


On Dec. 15, 2016, 6:48 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54754/
> ---
> 
> (Updated Dec. 15, 2016, 6:48 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1684
> https://issues.apache.org/jira/browse/AURORA-1684
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fixed starting cron jobs when using default_docker_parameters
> 
> The code was previously attempting to re-sanitize the configuration read from 
> storage rather than just using it as is.  This causes issues if after 
> sanitization the job no longer passes sanitization (which is the case here w/ 
> default_docker_parameters).
> 
> We've been running this in our branch forever.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 
> 5873983479c39a011eb363f7fd442867f0794b17 
>   src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java 
> 650facecc2e02be7bb3cd5ea9ff0f094e006bcb3 
>   
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java 
> fb06c282c4e94639c521658682c5da3e4dd4baf7 
> 
> Diff: https://reviews.apache.org/r/54754/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>