You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/10/18 01:01:36 UTC
Review Request 26899: Require StateManager callers to open their own
transactions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Repository: aurora
Description
-------
Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
[1] https://issues.apache.org/jira/browse/AURORA-871
Diffs
-----
src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
Diff: https://reviews.apache.org/r/26899/diff/
Testing
-------
./gradlew build -Pq
Thanks,
Bill Farner
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/#review57397
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On Oct. 17, 2014, 4:01 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26899/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 4:01 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
>
> This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
>
> [1] https://issues.apache.org/jira/browse/AURORA-871
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
> src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
> src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
> src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
> src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
> src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
> src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
> src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
> src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
> src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
> src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
> src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
> src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
>
> Diff: https://reviews.apache.org/r/26899/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/#review61324
-----------------------------------------------------------
Ship it!
Master (f8040b9) 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 Nov. 13, 2014, 10:19 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26899/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2014, 10:19 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
>
> This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
>
> [1] https://issues.apache.org/jira/browse/AURORA-871
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java e1b7d05b32bc5f6061e89fe2af901e34650f97bc
> src/main/java/org/apache/aurora/scheduler/async/Preemptor.java 1d337f6245530f0e22f75d5f1d301462d53dd8bb
> src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
> src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java e2ba8b8fe978a58d1edcd01963ea020e54529353
> src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
> src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 8217c512f31d4222e855218bee8d3172767d4ba4
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
> src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 86440ebd30bfe96262f30769d74e50c21d86f1c2
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6663555ef58541afb9b6634196f1c40aff94274c
> src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 77db411f338416787011868fad58986b3d614a2d
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b2b66acee9c0789f3660469d6d504b4510af5e79
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java f918d150fd14b71db5c1d0f501423a8f05e87f71
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 4673e8097dade38449278424d055ef92d0d1b275
> src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 59bfbcb9912a10c0e7fb28b6993cae7de8004ffe
> src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java 9682c89ee1b8f10a93d5e536359df4f034cc9eee
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 0e8a98c60293b8b5c23039b3c480670ed54faeb1
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 41709023fa95e29e45143f52ead68900c5ca6642
> src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
> src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 17295acc1e818ec59253dac93915efbe5c6f1d06
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
> src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4739909d1315e39b8225e4a718820f2f5965e3cc
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java c602cbdfc09bbc7179cd4a10602f11a4c89ef605
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 8d69ae97e77e271e01935722a9a4b2b2d1099273
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f36e34cc56e56c8268cafa0a2af0b15feb3187e8
>
> Diff: https://reviews.apache.org/r/26899/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/
-----------------------------------------------------------
(Updated Nov. 13, 2014, 10:19 p.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Changes
-------
Other priorities caused this review to go stale, so the inter-diff is probably not useful any more :-/
Repository: aurora
Description
-------
Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
[1] https://issues.apache.org/jira/browse/AURORA-871
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java e1b7d05b32bc5f6061e89fe2af901e34650f97bc
src/main/java/org/apache/aurora/scheduler/async/Preemptor.java 1d337f6245530f0e22f75d5f1d301462d53dd8bb
src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java e2ba8b8fe978a58d1edcd01963ea020e54529353
src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 8217c512f31d4222e855218bee8d3172767d4ba4
src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 86440ebd30bfe96262f30769d74e50c21d86f1c2
src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6663555ef58541afb9b6634196f1c40aff94274c
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 77db411f338416787011868fad58986b3d614a2d
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b2b66acee9c0789f3660469d6d504b4510af5e79
src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java f918d150fd14b71db5c1d0f501423a8f05e87f71
src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 4673e8097dade38449278424d055ef92d0d1b275
src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 59bfbcb9912a10c0e7fb28b6993cae7de8004ffe
src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java 9682c89ee1b8f10a93d5e536359df4f034cc9eee
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 0e8a98c60293b8b5c23039b3c480670ed54faeb1
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 41709023fa95e29e45143f52ead68900c5ca6642
src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 17295acc1e818ec59253dac93915efbe5c6f1d06
src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4739909d1315e39b8225e4a718820f2f5965e3cc
src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java c602cbdfc09bbc7179cd4a10602f11a4c89ef605
src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 8d69ae97e77e271e01935722a9a4b2b2d1099273
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f36e34cc56e56c8268cafa0a2af0b15feb3187e8
Diff: https://reviews.apache.org/r/26899/diff/
Testing
-------
./gradlew build -Pq
Thanks,
Bill Farner
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/#review58216
-----------------------------------------------------------
This patch does not apply cleanly on master (53f4e73), do you need to rebase?
- Aurora ReviewBot
On Oct. 21, 2014, 1:09 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26899/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 1:09 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
>
> This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
>
> [1] https://issues.apache.org/jira/browse/AURORA-871
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
> src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
> src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
> src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
> src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
> src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
> src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
> src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
> src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
> src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
> src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
> src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
> src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
>
> Diff: https://reviews.apache.org/r/26899/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 21, 2014, 1:25 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, lines 817-819
> > <https://reviews.apache.org/r/26899/diff/1-2/?file=724971#file724971line817>
> >
> > FWIW "|=" is a standard way to record boolean True in a result. This feels lees readable in my opinion.
Sure thing, reverted.
> On Oct. 21, 2014, 1:25 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 825
> > <https://reviews.apache.org/r/26899/diff/1-2/?file=724971#file724971line825>
> >
> > Would be great to update unit tests to validate this change.
Which change are you thinking of? There's `testKillCronJob` to cover the cron case, if that's what you're referring to. Jacoco shows no branches missed here.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/#review57512
-----------------------------------------------------------
On Oct. 21, 2014, 1:09 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26899/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 1:09 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
>
> This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
>
> [1] https://issues.apache.org/jira/browse/AURORA-871
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
> src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
> src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
> src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
> src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
> src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
> src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
> src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
> src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
> src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
> src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
> src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
> src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
>
> Diff: https://reviews.apache.org/r/26899/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/#review57512
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/26899/#comment98233>
FWIW "|=" is a standard way to record boolean True in a result. This feels lees readable in my opinion.
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/26899/#comment98235>
Would be great to update unit tests to validate this change.
- Maxim Khutornenko
On Oct. 21, 2014, 1:09 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26899/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 1:09 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
>
> This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
>
> [1] https://issues.apache.org/jira/browse/AURORA-871
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
> src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
> src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
> src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
> src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
> src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
> src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
> src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
> src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
> src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
> src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
> src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
> src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
>
> Diff: https://reviews.apache.org/r/26899/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/
-----------------------------------------------------------
(Updated Oct. 21, 2014, 1:09 a.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Repository: aurora
Description
-------
Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
[1] https://issues.apache.org/jira/browse/AURORA-871
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
Diff: https://reviews.apache.org/r/26899/diff/
Testing
-------
./gradlew build -Pq
Thanks,
Bill Farner
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 21, 2014, 12:30 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java, line 215
> > <https://reviews.apache.org/r/26899/diff/1/?file=724963#file724963line215>
> >
> > Mind renaming store to storeProvider?
I don't see any potential risk of misusing the variable if it is named `store`, and it results in more concise code.
> On Oct. 21, 2014, 12:30 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 804
> > <https://reviews.apache.org/r/26899/diff/1/?file=724971#file724971line804>
> >
> > This must be | or the write operation will never get executed in case tasksKilled is already true.
Oh wow, that's a manjor gotcha. I've reworked the code to avoid the trap.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/#review57487
-----------------------------------------------------------
On Oct. 17, 2014, 11:01 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26899/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:01 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
>
> This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
>
> [1] https://issues.apache.org/jira/browse/AURORA-871
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
> src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
> src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
> src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
> src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
> src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
> src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
> src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
> src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
> src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
> src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
> src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
> src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
>
> Diff: https://reviews.apache.org/r/26899/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 21, 2014, 12:30 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java, line 215
> > <https://reviews.apache.org/r/26899/diff/1/?file=724963#file724963line215>
> >
> > Mind renaming store to storeProvider?
>
> Bill Farner wrote:
> I don't see any potential risk of misusing the variable if it is named `store`, and it results in more concise code.
Up to you. Does not look consistent though as only these two places use store, everything else uses storeProvider.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/#review57487
-----------------------------------------------------------
On Oct. 21, 2014, 1:09 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26899/
> -----------------------------------------------------------
>
> (Updated Oct. 21, 2014, 1:09 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
>
> This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
>
> [1] https://issues.apache.org/jira/browse/AURORA-871
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
> src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
> src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
> src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
> src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
> src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
> src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
> src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
> src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
> src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
> src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
> src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
> src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
>
> Diff: https://reviews.apache.org/r/26899/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 26899: Require StateManager callers to open their
own transactions.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26899/#review57487
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java
<https://reviews.apache.org/r/26899/#comment98201>
Mind renaming store to storeProvider?
src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java
<https://reviews.apache.org/r/26899/#comment98203>
same here s/store/storeProvider
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/26899/#comment98210>
This must be | or the write operation will never get executed in case tasksKilled is already true.
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/26899/#comment98211>
Same here | instead of ||.
- Maxim Khutornenko
On Oct. 17, 2014, 11:01 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26899/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:01 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack.
>
> This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read->write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes.
>
> [1] https://issues.apache.org/jira/browse/AURORA-871
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7
> src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8
> src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44
> src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397
> src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234
> src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e
> src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5
> src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe
> src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40
> src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc
> src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d
> src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63
> src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32
> src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6
> src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142
> src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87
> src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b
> src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69
> src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d
> src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
> src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java cf4a015a040338a642fb07eec1fb7b5c11058fe5
> src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java c48cbae4864127e7799917182439f7670285b0d3
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java f36a88fda3539553800bd727c3d2a77a54f1e71c
> src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f739e6d1b1af6eea4875e03d32bfe88cef87b3ff
>
> Diff: https://reviews.apache.org/r/26899/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>