Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-16 Thread Stephan Erb 


> On Dec. 16, 2016, 2:13 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java,
> >  lines 69-72
> > 
> >
> > I feel like we should change the default to an empty set. The largest 
> > number of Aurora users will not need those fields and there is therefore no 
> > need to populate those by default.
> 
> Joshua Cohen wrote:
> I think at least for one release it should match the current behavior. 
> After 0.18.0 we can default it to empty?
> 
> David McLaughlin wrote:
> +1. Let's follow the deprecation policy for backwards incompatible 
> changes here.

We should probably file a ticket then, so that we don't forget this.


- Stephan


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


On Dec. 15, 2016, 9:04 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 9:04 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> Applied this to an internal test cluster with the update hydration disabled 
> and verified snapshots and backups and failover.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-16 Thread David McLaughlin 


> On Dec. 16, 2016, 1:13 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java,
> >  lines 69-72
> > 
> >
> > I feel like we should change the default to an empty set. The largest 
> > number of Aurora users will not need those fields and there is therefore no 
> > need to populate those by default.
> 
> Joshua Cohen wrote:
> I think at least for one release it should match the current behavior. 
> After 0.18.0 we can default it to empty?

+1. Let's follow the deprecation policy for backwards incompatible changes here.


- David


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


On Dec. 15, 2016, 8:04 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 8:04 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> Applied this to an internal test cluster with the update hydration disabled 
> and verified snapshots and backups and failover.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-16 Thread Joshua Cohen 


> On Dec. 16, 2016, 1:13 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java,
> >  lines 69-72
> > 
> >
> > I feel like we should change the default to an empty set. The largest 
> > number of Aurora users will not need those fields and there is therefore no 
> > need to populate those by default.

I think at least for one release it should match the current behavior. After 
0.18.0 we can default it to empty?


- Joshua


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


On Dec. 15, 2016, 8:04 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 8:04 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> Applied this to an internal test cluster with the update hydration disabled 
> and verified snapshots and backups and failover.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-16 Thread Stephan Erb 

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



Sorry, I am kind of late to the party:


src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
(lines 69 - 72)


I feel like we should change the default to an empty set. The largest 
number of Aurora users will not need those fields and there is therefore no 
need to populate those by default.


- Stephan Erb


On Dec. 15, 2016, 9:04 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 9:04 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> Applied this to an internal test cluster with the update hydration disabled 
> and verified snapshots and backups and failover.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread Aurora ReviewBot 

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


Ship it!




Master (8e37d0f) 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 Dec. 15, 2016, 8:04 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 8:04 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> Applied this to an internal test cluster with the update hydration disabled 
> and verified snapshots and backups and failover.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 

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

(Updated Dec. 15, 2016, 8:04 p.m.)


Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and Zameer 
Manji.


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


Repository: aurora


Description
---

Motivation: Thanks to the mybatis query metrics we added, we found that double 
writing Snapshot fields for H2 stores adds considerable overhead to our 
snapshot creation time. 

Snapshots are also written as backups, and many operators choose to process 
backups offline for analytics, rather than query the live scheduler (due to not 
being able to scale reads horizontally). So this allows operators to 
enable/disable the hydrated fields as needed.


Diffs
-

  RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
  
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 
3fa408e283f91b313633959ea6d2e730d4dc0771 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
  src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
853780bc68400489578ed3692920aafcec42c999 
  src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
7a11850e217dcb0148e4a4d33542c95b2e53a726 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 
cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 

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


Testing (updated)
---

./gradlew test

Applied this to an internal test cluster with the update hydration disabled and 
verified snapshots and backups and failover.


Thanks,

David McLaughlin

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread Santhosh Kumar Shanmugham 

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


Ship it!




Ship It!

- Santhosh Kumar Shanmugham


On Dec. 15, 2016, 11:24 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 11:24 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> I'll apply this to one of our test clusters before merging to master too.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 

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



@ReviewBot retry

- David McLaughlin


On Dec. 15, 2016, 7:24 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 7:24 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> I'll apply this to one of our test clusters before merging to master too.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread Aurora ReviewBot 

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



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

  Test coverage missing for 
org/apache/aurora/scheduler/reconciliation/TaskTimeout$TimedOutTaskHandler
  Test coverage missing for 
org/apache/aurora/scheduler/cron/quartz/AuroraCronJob$CronBatchWorker
  Test coverage missing for 
org/apache/aurora/scheduler/cron/quartz/AuroraCronJob
  Test coverage missing for 
org/apache/aurora/scheduler/cron/quartz/AuroraCronJob$Config
  Test coverage missing for 
org/apache/aurora/scheduler/scheduling/TaskGroups$TaskGroupBatchWorker
  Test coverage missing for org/apache/aurora/scheduler/scheduling/TaskGroups$1
  Test coverage missing for org/apache/aurora/scheduler/scheduling/TaskGroups
  Test coverage missing for 
org/apache/aurora/scheduler/storage/mem/MemTaskStore$SecondaryIndex$2
  Test coverage missing for 
org/apache/aurora/scheduler/storage/mem/MemTaskStore$SecondaryIndex$1
  Test coverage missing for org/apache/aurora/scheduler/mesos/TaskStatusStats$3
  Test coverage missing for org/apache/aurora/scheduler/mesos/TaskStatusStats$2
  Test coverage missing for org/apache/aurora/scheduler/mesos/TaskStatusStats$1
  Test coverage missing for org/apache/aurora/scheduler/mesos/TaskStatusStats
  Test coverage missing for org/apache/aurora/scheduler/thrift/AuditMessages
  Test coverage missing for 
org/apache/aurora/scheduler/preemptor/PendingTaskProcessor
  Test coverage missing for 
org/apache/aurora/scheduler/preemptor/PendingTaskProcessor$1
  Test coverage missing for 
org/apache/aurora/scheduler/preemptor/PendingTaskProcessor$2
  Test coverage missing for 
org/apache/aurora/scheduler/preemptor/PendingTaskProcessor$3
  Test coverage missing for 
org/apache/aurora/scheduler/preemptor/PreemptorModule$PreemptorService
  Test coverage missing for org/apache/aurora/scheduler/events/WebhookModule
  Test coverage missing for org/apache/aurora/scheduler/events/Webhook
  Test coverage missing for org/apache/aurora/scheduler/events/WebhookInfo
  Test coverage missing for 
org/apache/aurora/scheduler/storage/log/EntrySerializer$EntrySerializerImpl$1
  Test coverage missing for 
org/apache/aurora/scheduler/storage/log/LogStorage$Settings
  Test coverage missing for 
org/apache/aurora/scheduler/storage/log/LogStorage$ScheduledExecutorSchedulingService
  Test coverage missing for 
org/apache/aurora/scheduler/storage/log/LogStorageModule
  Test coverage missing for 
org/apache/aurora/scheduler/storage/backup/BackupModule
  Test coverage missing for org/apache/aurora/scheduler/TaskVars
  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

* 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: 5 mins 24.358 secs


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

- Aurora ReviewBot


On Dec. 15, 2016, 7:24 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 7:24 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 

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

(Updated Dec. 15, 2016, 7:24 p.m.)


Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and Zameer 
Manji.


Changes
---

Fix timing merge.


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


Repository: aurora


Description
---

Motivation: Thanks to the mybatis query metrics we added, we found that double 
writing Snapshot fields for H2 stores adds considerable overhead to our 
snapshot creation time. 

Snapshots are also written as backups, and many operators choose to process 
backups offline for analytics, rather than query the live scheduler (due to not 
being able to scale reads horizontally). So this allows operators to 
enable/disable the hydrated fields as needed.


Diffs (updated)
-

  RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
  
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 
3fa408e283f91b313633959ea6d2e730d4dc0771 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
  src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
853780bc68400489578ed3692920aafcec42c999 
  src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
7a11850e217dcb0148e4a4d33542c95b2e53a726 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 
cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 

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


Testing
---

./gradlew test

I'll apply this to one of our test clusters before merging to master too.


Thanks,

David McLaughlin

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 


> On Dec. 15, 2016, 7:21 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java,
> >  line 128
> > 
> >
> > This `@Timed` annotation is on the wrong method now.

Derp. Fixed.


- David


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


On Dec. 15, 2016, 7:22 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 7:22 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> I'll apply this to one of our test clusters before merging to master too.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 

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

(Updated Dec. 15, 2016, 7:22 p.m.)


Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and Zameer 
Manji.


Changes
---

Fix the update logic. Add release notes.


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


Repository: aurora


Description
---

Motivation: Thanks to the mybatis query metrics we added, we found that double 
writing Snapshot fields for H2 stores adds considerable overhead to our 
snapshot creation time. 

Snapshots are also written as backups, and many operators choose to process 
backups offline for analytics, rather than query the live scheduler (due to not 
being able to scale reads horizontally). So this allows operators to 
enable/disable the hydrated fields as needed.


Diffs (updated)
-

  RELEASE-NOTES.md 90c4793b218f54ad92ab4da5049d5e92c3a104b3 
  
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 
3fa408e283f91b313633959ea6d2e730d4dc0771 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
  src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
853780bc68400489578ed3692920aafcec42c999 
  src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
7a11850e217dcb0148e4a4d33542c95b2e53a726 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 
cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 

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


Testing
---

./gradlew test

I'll apply this to one of our test clusters before merging to master too.


Thanks,

David McLaughlin

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread Joshua Cohen 

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


Fix it, then Ship it!




lgtm modulo the below.


src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
(line 128)


This `@Timed` annotation is on the wrong method now.


- Joshua Cohen


On Dec. 15, 2016, 6:16 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 6:16 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 853780bc68400489578ed3692920aafcec42c999 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> I'll apply this to one of our test clusters before merging to master too.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread Aurora ReviewBot 

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



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

at 
org.gradle.launcher.daemon.server.exec.RequestStopIfSingleUsedDaemon.execute(RequestStopIfSingleUsedDaemon.java:34)
at 
org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
at 
org.gradle.launcher.daemon.server.exec.ForwardClientInput$2.call(ForwardClientInput.java:74)
at 
org.gradle.launcher.daemon.server.exec.ForwardClientInput$2.call(ForwardClientInput.java:72)
at org.gradle.util.Swapper.swap(Swapper.java:38)
at 
org.gradle.launcher.daemon.server.exec.ForwardClientInput.execute(ForwardClientInput.java:72)
at 
org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
at 
org.gradle.launcher.daemon.server.exec.LogAndCheckHealth.execute(LogAndCheckHealth.java:55)
at 
org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
at 
org.gradle.launcher.daemon.server.exec.LogToClient.doBuild(LogToClient.java:60)
at 
org.gradle.launcher.daemon.server.exec.BuildCommandOnly.execute(BuildCommandOnly.java:36)
at 
org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
at 
org.gradle.launcher.daemon.server.exec.EstablishBuildEnvironment.doBuild(EstablishBuildEnvironment.java:72)
at 
org.gradle.launcher.daemon.server.exec.BuildCommandOnly.execute(BuildCommandOnly.java:36)
at 
org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
at 
org.gradle.launcher.daemon.server.exec.HintGCAfterBuild.execute(HintGCAfterBuild.java:44)
at 
org.gradle.launcher.daemon.server.api.DaemonCommandExecution.proceed(DaemonCommandExecution.java:120)
at 
org.gradle.launcher.daemon.server.exec.StartBuildOrRespondWithBusy$1.run(StartBuildOrRespondWithBusy.java:50)
at 
org.gradle.launcher.daemon.server.DaemonStateCoordinator$1.run(DaemonStateCoordinator.java:293)
at 
org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:54)
at 
org.gradle.internal.concurrent.StoppableExecutorImpl$1.run(StoppableExecutorImpl.java:40)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java:392:
   These nested if statements could be combined
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java:392:
   These nested if statements could be combined
:pmdMain FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':pmdMain'.
> 2 PMD rule violations were found. See the report at: 
> file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/pmd/main.html

* 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: 2 mins 28.393 secs


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

- Aurora ReviewBot


On Dec. 15, 2016, 6:16 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 6:16 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 

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

(Updated Dec. 15, 2016, 6:16 p.m.)


Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and Zameer 
Manji.


Changes
---

Rebase.


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


Repository: aurora


Description
---

Motivation: Thanks to the mybatis query metrics we added, we found that double 
writing Snapshot fields for H2 stores adds considerable overhead to our 
snapshot creation time. 

Snapshots are also written as backups, and many operators choose to process 
backups offline for analytics, rather than query the live scheduler (due to not 
being able to scale reads horizontally). So this allows operators to 
enable/disable the hydrated fields as needed.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 
3fa408e283f91b313633959ea6d2e730d4dc0771 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
  src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
853780bc68400489578ed3692920aafcec42c999 
  src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
7a11850e217dcb0148e4a4d33542c95b2e53a726 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 
cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 

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


Testing
---

./gradlew test

I'll apply this to one of our test clusters before merging to master too.


Thanks,

David McLaughlin

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread Aurora ReviewBot 

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



This patch does not apply cleanly against master (8e37d0f), do you need to 
rebase?

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

- Aurora ReviewBot


On Dec. 15, 2016, 6:07 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 6:07 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> d2c859055f905ac76ee8eb387dca103b9857ddbe 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> I'll apply this to one of our test clusters before merging to master too.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 

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

(Updated Dec. 15, 2016, 6:07 p.m.)


Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and Zameer 
Manji.


Changes
---

Checkstyle.


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


Repository: aurora


Description
---

Motivation: Thanks to the mybatis query metrics we added, we found that double 
writing Snapshot fields for H2 stores adds considerable overhead to our 
snapshot creation time. 

Snapshots are also written as backups, and many operators choose to process 
backups offline for analytics, rather than query the live scheduler (due to not 
being able to scale reads horizontally). So this allows operators to 
enable/disable the hydrated fields as needed.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 
3fa408e283f91b313633959ea6d2e730d4dc0771 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
  src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
d2c859055f905ac76ee8eb387dca103b9857ddbe 
  src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
7a11850e217dcb0148e4a4d33542c95b2e53a726 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 
cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 

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


Testing
---

./gradlew test

I'll apply this to one of our test clusters before merging to master too.


Thanks,

David McLaughlin

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 


> On Dec. 15, 2016, 3:39 p.m., Joshua Cohen wrote:
> > I think there's some relevant context in the original review for this 
> > feature: https://reviews.apache.org/r/44471/. There was concern in that 
> > review with the removal of the native fields from snapshots for the reasons 
> > you mentioned in the associated ticket. I think that's still a concern, and 
> > there's the possibility that tooling exists outside of Aurora that depends 
> > on these fields existing in the snapshot.
> > 
> > I'm generally in favor of the changes you've outlined, as they should 
> > dramatically improve snapshot performance, however perhaps it makes sense 
> > to put this behind a command line flag? Maybe a single flag that allows for 
> > control over which snapshot fields are emitted in addition to the dbScript 
> > (this would require adding something like `getName` to `SnapshotField`). 
> > I'd also be fine with providing some tooling that converts the dbScript in 
> > a snapshot to a fully hydrated thrift snapshot. Either way, I don't think 
> > we should ship this without some accommodation for that use case. Maybe 
> > start with the flag (as it's much easier to add), and follow up with the 
> > additional tooling at a later date (at which point we could kill the flag 
> > and all of the `SnapshotField`s that are already encompassed by the 
> > `dbScript`.)

Moved behind a Scheduler flag.


> On Dec. 15, 2016, 3:39 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java,
> >  lines 315-317
> > 
> >
> > Calling `hasDbSnapshot` has nothing to do with whether or not there's a 
> > memory store. It has to do with whether or not there's a `dbScript` in the 
> > snapshot. That's a relatively recent addition to snapshots. Prior to that 
> > we fetched everything from H2 and dumped it to the snapshot as individual 
> > thrift entries.

Good to know, thanks.


- David


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


On Dec. 15, 2016, 6:01 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 6:01 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> d2c859055f905ac76ee8eb387dca103b9857ddbe 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> I'll apply this to one of our test clusters before merging to master too.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 


> On Dec. 15, 2016, 2:15 p.m., Stephan Erb wrote:
> > I would like to understand your thinking here: Why add so many todos if the 
> > necessary code change itself is rather small. Do you expect there is a 
> > certain risk here?

Updated description to match information I added to the JIRA. The TODOs are 
also now gone. Please let me know if it's still not clear why we can't just 
remove these duplicate fields.


- David


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


On Dec. 15, 2016, 6:01 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54774/
> ---
> 
> (Updated Dec. 15, 2016, 6:01 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1861
> https://issues.apache.org/jira/browse/AURORA-1861
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Motivation: Thanks to the mybatis query metrics we added, we found that 
> double writing Snapshot fields for H2 stores adds considerable overhead to 
> our snapshot creation time. 
> 
> Snapshots are also written as backups, and many operators choose to process 
> backups offline for analytics, rather than query the live scheduler (due to 
> not being able to scale reads horizontally). So this allows operators to 
> enable/disable the hydrated fields as needed.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  3fa408e283f91b313633959ea6d2e730d4dc0771 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> d2c859055f905ac76ee8eb387dca103b9857ddbe 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> 7a11850e217dcb0148e4a4d33542c95b2e53a726 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 
> 
> Diff: https://reviews.apache.org/r/54774/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew test
> 
> I'll apply this to one of our test clusters before merging to master too.
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 

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

(Updated Dec. 15, 2016, 6:01 p.m.)


Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and Zameer 
Manji.


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


Repository: aurora


Description (updated)
---

Motivation: Thanks to the mybatis query metrics we added, we found that double 
writing Snapshot fields for H2 stores adds considerable overhead to our 
snapshot creation time. 

Snapshots are also written as backups, and many operators choose to process 
backups offline for analytics, rather than query the live scheduler (due to not 
being able to scale reads horizontally). So this allows operators to 
enable/disable the hydrated fields as needed.


Diffs
-

  
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 
3fa408e283f91b313633959ea6d2e730d4dc0771 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
  src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
d2c859055f905ac76ee8eb387dca103b9857ddbe 
  src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
7a11850e217dcb0148e4a4d33542c95b2e53a726 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 
cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 

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


Testing
---

./gradlew test

I'll apply this to one of our test clusters before merging to master too.


Thanks,

David McLaughlin

Re: Review Request 54774: Avoid double writing job updates to the Scheduler Snapshot

2016-12-15 Thread David McLaughlin 

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

(Updated Dec. 15, 2016, 5:59 p.m.)


Review request for Aurora, Joshua Cohen, Santhosh Kumar Shanmugham, and Zameer 
Manji.


Changes
---

Put this feature behind a flag.


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


Repository: aurora


Description
---

Motivation: Thanks to the mybatis query metrics we added, we found that the 
redundant query I'm removing here currently adds 20s to the snapshot creation 
time. This is an extra 20s unavailability every time we snapshot on our prod 
clusters. 

===

Avoid double writing job updates to the Scheduler Snapshot. This should be 
low-risk because:

1) Job Updates have *never* had an in-memory store implementation. 
2) They are also relatively new, so the chances of some old code processing job 
updates from backups is fairly low?


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 
3fa408e283f91b313633959ea6d2e730d4dc0771 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
7dcd1bfcf303cf374e9a6627cb6c632ccea098f2 
  src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
d2c859055f905ac76ee8eb387dca103b9857ddbe 
  src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
7a11850e217dcb0148e4a4d33542c95b2e53a726 
  
src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 
cf0a8f3ea11e9c48d1f16441af54dc781b33bdfc 

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


Testing
---

./gradlew test

I'll apply this to one of our test clusters before merging to master too.


Thanks,

David McLaughlin