Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 23, 2015, 10:52 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 255 https://reviews.apache.org/r/30891/diff/7/?file=873086#file873086line255 s/may/will/? Changed. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73703 --- On Feb. 23, 2015, 8:36 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 23, 2015, 8:36 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote:
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 301
https://reviews.apache.org/r/30891/diff/7/?file=873086#file873086line301
No need to hold the intrinsic lock while logging here
Having an explicit lock will be inconsistent with the rest of this class and
will add up to clutter. Given the on-demand logging here, I don't see much
value in over-optimizing this logic.
On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote:
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 317
https://reviews.apache.org/r/30891/diff/7/?file=873086#file873086line317
Same as above - no need to hold the intrinsic lock while logging here.
Same here.
On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote:
src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java,
line 78
https://reviews.apache.org/r/30891/diff/7/?file=873089#file873089line78
It would be good form to reset the level in the tearDown here.
Sure, done.
On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote:
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 119
https://reviews.apache.org/r/30891/diff/3/?file=862800#file862800line119
as written, a caller needs to know FAILURE and FAILURE_STATIC_MISMATCH
both mean failure, but it would be easy to just check for `== FAILURE` and
miss that in code reviews. Would it make sense to add `isFailure()` and
`isStaticMismatch()` here?
Also, consider fetching the VetoGroup in the constructor so that
precondition checks will fail faster
as written, a caller needs to know FAILURE and FAILURE_STATIC_MISMATCH both
mean failure, but it would be easy to just check for == FAILURE and miss that
in code reviews. Would it make sense to add isFailure() and isStaticMismatch()
here?
This would shift failure detection responsibility upstream and will open up for
invalid combinations like isFailure=False isStaticMismatch=True. I believe
enum-based approach is the more accurate and easier to reason solution in this
case.
Also, consider fetching the VetoGroup in the constructor so that precondition
checks will fail faster
`Veto.identifyGroup` does not provide any additional error handling. It's
already failing fast as the only way to create an instance with vetoes is
through this:
```
public static Assignment failure(SetVeto vetoes) {
return new Assignment(NO_TASK_INFO, MorePreconditions.checkNotBlank(vetoes));
}
```
- Maxim
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30891/#review72448
---
On Feb. 23, 2015, 8:36 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30891/
---
(Updated Feb. 23, 2015, 8:36 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-909
https://issues.apache.org/jira/browse/AURORA-909
Repository: aurora
Description
---
Offer filtering for static vetoes. Part 3 of 4: Filtering out statically
banned offers.
Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a
parent.
Original RB: https://reviews.apache.org/r/28617/
Diffs
-
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
b241d7b22c3d1ceca127b2671eb608ae41283bf3
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java
21ea7d2b9d2f8c76367d7ae985270402bb51ea26
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java
5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917
src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java
7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java
9eef52a333e09454e8fd0026371c7e64472a883d
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java
b6d4d8e771c7d16a46e43c7d5c427b911f8b661d
Diff: https://reviews.apache.org/r/30891/diff/
Testing
---
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72448 --- Ship it! src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java https://reviews.apache.org/r/30891/#comment118503 as written, a caller needs to know FAILURE and FAILURE_STATIC_MISMATCH both mean failure, but it would be easy to just check for `== FAILURE` and miss that in code reviews. Would it make sense to add `isFailure()` and `isStaticMismatch()` here? Also, consider fetching the VetoGroup in the constructor so that precondition checks will fail faster src/main/java/org/apache/aurora/scheduler/async/OfferManager.java https://reviews.apache.org/r/30891/#comment120285 No need to hold the intrinsic lock while logging here src/main/java/org/apache/aurora/scheduler/async/OfferManager.java https://reviews.apache.org/r/30891/#comment120286 Same as above - no need to hold the intrinsic lock while logging here. src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java https://reviews.apache.org/r/30891/#comment120290 It would be good form to reset the level in the tearDown here. - Kevin Sweeney On Feb. 23, 2015, 12:36 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 23, 2015, 12:36 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 21, 2015, 12:43 a.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java, line 80 https://reviews.apache.org/r/30891/diff/5-6/?file=867194#file867194line80 This is kinda weird, what's the motivation here? Maxim Khutornenko wrote: Test coverage for the fine-logging statements. Maxim Khutornenko wrote: Just noticed you are referring to the `setUp()`. This is making sure anything with INFO is still logged and setting to FINE does not mess up with it depending on the test execution sequence. Bill Farner wrote: How about an injected `Logger` instead? Guice actually injects this by default, and there's no good reason we aren't using it. https://github.com/google/guice/wiki/BuiltInBindings It would require modifying HostOffers to accept a Logger instance and adjusting both OfferManagerImplTest and TaskSchedulerTest to expect info(), fine() and isLoggable(). I rejected that idea to reduce the diff size. Since info level folds into fine, set the default level to Fine in setUp() instead. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73355 --- On Feb. 20, 2015, 11:58 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 20, 2015, 11:58 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 23, 2015, 8:36 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Bill's comments. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs (updated) - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73683 --- Master (277ac43) is red with this patch. ./build-support/jenkins/build.sh src.test.python.apache.aurora.client.cli.sla . SUCCESS src.test.python.apache.aurora.client.cli.supdate . SUCCESS src.test.python.apache.aurora.client.cli.task . SUCCESS src.test.python.apache.aurora.client.cli.update . SUCCESS src.test.python.apache.aurora.client.cli.version . SUCCESS src.test.python.apache.aurora.client.config . SUCCESS src.test.python.apache.aurora.client.factory . SUCCESS src.test.python.apache.aurora.client.hooks.hooked_api . SUCCESS src.test.python.apache.aurora.client.hooks.non_hooked_api . SUCCESS src.test.python.apache.aurora.common.test_aurora_job_key . SUCCESS src.test.python.apache.aurora.common.test_cluster . SUCCESS src.test.python.apache.aurora.common.test_cluster_option . SUCCESS src.test.python.apache.aurora.common.test_clusters . SUCCESS src.test.python.apache.aurora.common.test_http_signaler . SUCCESS src.test.python.apache.aurora.common.test_pex_version . SUCCESS src.test.python.apache.aurora.common.test_shellify . SUCCESS src.test.python.apache.aurora.common.test_transport . SUCCESS src.test.python.apache.aurora.config.test_base . SUCCESS src.test.python.apache.aurora.config.test_constraint_parsing . SUCCESS src.test.python.apache.aurora.config.test_loader . SUCCESS src.test.python.apache.aurora.config.test_thrift . SUCCESS src.test.python.apache.aurora.executor.common.announcer . SUCCESS src.test.python.apache.aurora.executor.common.directory_sandbox . SUCCESS src.test.python.apache.aurora.executor.common.executor_detector . SUCCESS src.test.python.apache.aurora.executor.common.executor_timeout . SUCCESS src.test.python.apache.aurora.executor.common.health_checker . FAILURE src.test.python.apache.aurora.executor.common.path_detector . SUCCESS src.test.python.apache.aurora.executor.common.task_info . SUCCESS src.test.python.apache.aurora.executor.executor_base . SUCCESS src.test.python.apache.aurora.executor.executor_vars . SUCCESS src.test.python.apache.aurora.executor.status_manager . SUCCESS src.test.python.apache.aurora.executor.thermos_task_runner . SUCCESS src.test.python.apache.thermos.common.test_pathspec . SUCCESS src.test.python.apache.thermos.core.test_runner_integration . SUCCESS src.test.python.apache.thermos.monitoring.test_disk . SUCCESS FAILURE [31m FAILURE[0m I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 23, 2015, 8:36 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 23, 2015, 8:36 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner.
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73703 --- Ship it! src/main/java/org/apache/aurora/scheduler/async/OfferManager.java https://reviews.apache.org/r/30891/#comment120087 s/may/will/? - Bill Farner On Feb. 23, 2015, 8:36 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 23, 2015, 8:36 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 21, 2015, 12:43 a.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java, line 80 https://reviews.apache.org/r/30891/diff/5-6/?file=867194#file867194line80 This is kinda weird, what's the motivation here? Maxim Khutornenko wrote: Test coverage for the fine-logging statements. Just noticed you are referring to the `setUp()`. This is making sure anything with INFO is still logged and setting to FINE does not mess up with it depending on the test execution sequence. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73355 --- On Feb. 20, 2015, 11:58 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 20, 2015, 11:58 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73376 --- Ship it! Master (e5de618) 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 Feb. 20, 2015, 11:58 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 20, 2015, 11:58 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 20, 2015, 11:58 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Bill's comments. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs (updated) - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 21, 2015, 12:43 a.m., Bill Farner wrote: src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java, line 80 https://reviews.apache.org/r/30891/diff/5-6/?file=867194#file867194line80 This is kinda weird, what's the motivation here? Test coverage for the fine-logging statements. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73355 --- On Feb. 20, 2015, 11:58 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 20, 2015, 11:58 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 295
https://reviews.apache.org/r/30891/diff/5/?file=867191#file867191line295
Some debug visibility could be _really_ helpful here.
Consider a `LOG.isLoggable(Level.FINE)` guard + `LOG.fine()` here and
in `addStaticGroupBan`.
Done.
On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 299
https://reviews.apache.org/r/30891/diff/5/?file=867191#file867191line299
It wouldn't hurt to ensure consistency between `staticallyBannedOffers`
and `offersById` by making sure that the offer ID exists in `offersById`
before adding to this map. It's plausible for `launchFirst` to race
removal of an offer, resulting in a memory leak in `staticallyBannedOffers`.
Done.
On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 63
https://reviews.apache.org/r/30891/diff/5/?file=867193#file867193line63
More context would be helpful here, specifically about what a static
mismatch is. (Feel free to link to `VetoGroup#STATIC`.
Linked to VetoGroup.
On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 117
https://reviews.apache.org/r/30891/diff/5/?file=867193#file867193line117
I find this more readable:
if (Veto.identifyGroup(vetoes) == VetoGroup.STATIC) {
return Result.FAILURE_STATIC_MISMATCH;
} else {
return Result.FAILURE;
}
Or, if you prefer ternary:
return (Veto.identifyGroup(vetoes) == VetoGroup.STATIC)
? Result.FAILURE_STATIC_MISMATCH
: Result.FAILURE;
Done.
On Feb. 19, 2015, 10:05 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 251
https://reviews.apache.org/r/30891/diff/5/?file=867191#file867191line251
It would be helpful to include a comment about why things land in this
map, if only to save the reader from tracing what goes in.
Also, please TODO+ticket exposing this map via a debug endpoint.
Done and done.
- Maxim
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30891/#review73208
---
On Feb. 20, 2015, 11:58 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30891/
---
(Updated Feb. 20, 2015, 11:58 p.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-909
https://issues.apache.org/jira/browse/AURORA-909
Repository: aurora
Description
---
Offer filtering for static vetoes. Part 3 of 4: Filtering out statically
banned offers.
Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a
parent.
Original RB: https://reviews.apache.org/r/28617/
Diffs
-
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
b241d7b22c3d1ceca127b2671eb608ae41283bf3
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java
21ea7d2b9d2f8c76367d7ae985270402bb51ea26
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java
5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917
src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java
7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java
9eef52a333e09454e8fd0026371c7e64472a883d
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java
b6d4d8e771c7d16a46e43c7d5c427b911f8b661d
Diff: https://reviews.apache.org/r/30891/diff/
Testing
---
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73355 --- src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java https://reviews.apache.org/r/30891/#comment119653 This is kinda weird, what's the motivation here? - Bill Farner On Feb. 20, 2015, 11:58 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 20, 2015, 11:58 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30891/#review73208
---
Looks good! Only a few minor suggestions.
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
https://reviews.apache.org/r/30891/#comment119445
It would be helpful to include a comment about why things land in this map,
if only to save the reader from tracing what goes in.
Also, please TODO+ticket exposing this map via a debug endpoint.
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
https://reviews.apache.org/r/30891/#comment119450
Some debug visibility could be _really_ helpful here.
Consider a `LOG.isLoggable(Level.FINE)` guard + `LOG.fine()` here and in
`addStaticGroupBan`.
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
https://reviews.apache.org/r/30891/#comment119454
It wouldn't hurt to ensure consistency between `staticallyBannedOffers` and
`offersById` by making sure that the offer ID exists in `offersById` before
adding to this map. It's plausible for `launchFirst` to race removal of an
offer, resulting in a memory leak in `staticallyBannedOffers`.
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java
https://reviews.apache.org/r/30891/#comment119447
More context would be helpful here, specifically about what a static
mismatch is. (Feel free to link to `VetoGroup#STATIC`.
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java
https://reviews.apache.org/r/30891/#comment119449
I find this more readable:
if (Veto.identifyGroup(vetoes) == VetoGroup.STATIC) {
return Result.FAILURE_STATIC_MISMATCH;
} else {
return Result.FAILURE;
}
Or, if you prefer ternary:
return (Veto.identifyGroup(vetoes) == VetoGroup.STATIC)
? Result.FAILURE_STATIC_MISMATCH
: Result.FAILURE;
- Bill Farner
On Feb. 18, 2015, 2:40 a.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30891/
---
(Updated Feb. 18, 2015, 2:40 a.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-909
https://issues.apache.org/jira/browse/AURORA-909
Repository: aurora
Description
---
Offer filtering for static vetoes. Part 3 of 4: Filtering out statically
banned offers.
Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a
parent.
Original RB: https://reviews.apache.org/r/28617/
Diffs
-
src/main/java/org/apache/aurora/scheduler/async/OfferManager.java
b241d7b22c3d1ceca127b2671eb608ae41283bf3
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java
21ea7d2b9d2f8c76367d7ae985270402bb51ea26
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java
5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917
src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java
7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java
9eef52a333e09454e8fd0026371c7e64472a883d
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java
b6d4d8e771c7d16a46e43c7d5c427b911f8b661d
Diff: https://reviews.apache.org/r/30891/diff/
Testing
---
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72876 --- This patch does not apply cleanly on master (ec66a5e), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 18, 2015, 2:01 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 18, 2015, 2:01 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 17, 2015, 4:59 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java, line 316 https://reviews.apache.org/r/30891/diff/3/?file=862798#file862798line316 It seems like an abstraction violation for a thing named Queue to make logic decisions about its contents. Have you considered having the `acceptor` own this behavior? Maxim Khutornenko wrote: Well, I don't really see why it has Queue in its name in the first place :) It does not really act like one. It accepts offers by iterating over the entire collection of available offers and sends Mesos launchTask requests. I'd rather rename it to OfferCache or OfferHandler if it helps. Regarding the abstraction violation, I actually see the static ban details belonging more to the OfferQueue than the acceptor as its more about offer state wrt a given task group than a specific assignment request. Bill Farner wrote: I actually see the static ban details belonging more to the OfferQueue than the acceptor as its more about offer state wrt a given task group than a specific assignment request That makes sense, otherwise you need to figure out a way to plumb invalidation to the third party. +1 to the rename +1 to the rename. I vote for OfferManager to keep parity with other naming conventions. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72852 --- On Feb. 12, 2015, 6:27 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 12, 2015, 6:27 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 18, 2015, 2:01 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Bill's and Kevin's comments. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs (updated) - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72885 --- Ship it! Master (e0e3f2e) 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 Feb. 18, 2015, 2:40 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 18, 2015, 2:40 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 18, 2015, 2:40 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Rebased. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs (updated) - src/main/java/org/apache/aurora/scheduler/async/OfferManager.java b241d7b22c3d1ceca127b2671eb608ae41283bf3 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 21ea7d2b9d2f8c76367d7ae985270402bb51ea26 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferManagerImplTest.java 7ee2bb9bec6c59ba67b65d5b1229c64aca1277ff src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 9eef52a333e09454e8fd0026371c7e64472a883d src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b6d4d8e771c7d16a46e43c7d5c427b911f8b661d Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 18, 2015, 12:59 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java, line 316 https://reviews.apache.org/r/30891/diff/3/?file=862798#file862798line316 It seems like an abstraction violation for a thing named Queue to make logic decisions about its contents. Have you considered having the `acceptor` own this behavior? Maxim Khutornenko wrote: Well, I don't really see why it has Queue in its name in the first place :) It does not really act like one. It accepts offers by iterating over the entire collection of available offers and sends Mesos launchTask requests. I'd rather rename it to OfferCache or OfferHandler if it helps. Regarding the abstraction violation, I actually see the static ban details belonging more to the OfferQueue than the acceptor as its more about offer state wrt a given task group than a specific assignment request. Bill Farner wrote: I actually see the static ban details belonging more to the OfferQueue than the acceptor as its more about offer state wrt a given task group than a specific assignment request That makes sense, otherwise you need to figure out a way to plumb invalidation to the third party. +1 to the rename Kevin Sweeney wrote: +1 to the rename. I vote for OfferManager to keep parity with other naming conventions. I am fine with OfferManager. I will add a TODO and address it in a separate diff (incoming). - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72852 --- On Feb. 13, 2015, 2:27 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 13, 2015, 2:27 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72429 --- To support this diff, can you include the jmh results before and after? - Bill Farner On Feb. 13, 2015, 2:27 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 13, 2015, 2:27 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 13, 2015, 8:35 p.m., Bill Farner wrote: To support this diff, can you include the jmh results before and after? They are in the mentioned above original diff description: https://reviews.apache.org/r/28617/ - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72429 --- On Feb. 13, 2015, 2:27 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 13, 2015, 2:27 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30891/#review72308
---
Master (f3473a3) is red with this patch.
./build-support/jenkins/build.sh
import org.apache.aurora.scheduler.filter.SchedulingFilter.VetoGroup;
^
symbol: class VetoGroup
location: interface SchedulingFilter
Note: Writing
file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java:33:
error: cannot find symbol
import org.apache.aurora.scheduler.filter.SchedulingFilter.VetoGroup;
^
symbol: class VetoGroup
location: interface SchedulingFilter
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java:117:
error: cannot find symbol
VetoGroup vetoGroup = Veto.identifyGroup(vetoes);
^
symbol: class VetoGroup
location: class Assignment
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java:117:
error: cannot find symbol
VetoGroup vetoGroup = Veto.identifyGroup(vetoes);
^
symbol: method identifyGroup(SetVeto)
location: class Veto
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java:118:
error: cannot find symbol
if (vetoGroup == VetoGroup.STATIC) {
^
symbol: variable VetoGroup
location: class Assignment
4 errors
FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':compileJava'.
Compilation failed; see the compiler error output for details.
* 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: 1 mins 17.762 secs
I will refresh this build result if you post a review containing @ReviewBot
retry
- Aurora ReviewBot
On Feb. 13, 2015, 1:04 a.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30891/
---
(Updated Feb. 13, 2015, 1:04 a.m.)
Review request for Aurora, Kevin Sweeney and Bill Farner.
Bugs: AURORA-909
https://issues.apache.org/jira/browse/AURORA-909
Repository: aurora
Description
---
Offer filtering for static vetoes. Part 3 of 4: Filtering out statically
banned offers.
Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a
parent.
Original RB: https://reviews.apache.org/r/28617/
Diffs
-
src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java
332338b7bef7d622333f8ea6508c4f5970b8e7c4
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java
ced3bdef1731354aedc82bb12f45ba6f040e1ab7
src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java
5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917
src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java
2b5dc4902f57e508d76f5e16997ae09d04464220
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java
d0e11932e8b5ba1393279137c8465a308e1d6bf5
src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java
74e31334bc139c47eb8b0beee46ee7bad62a2f80
Diff: https://reviews.apache.org/r/30891/diff/
Testing
---
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 13, 2015, 1:04 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Rebased. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs (updated) - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 13, 2015, 2:27 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Rebased. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs (updated) - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72321 --- Ship it! Master (7637200) 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 Feb. 13, 2015, 2:27 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 13, 2015, 2:27 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 332338b7bef7d622333f8ea6508c4f5970b8e7c4 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ced3bdef1731354aedc82bb12f45ba6f040e1ab7 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 2b5dc4902f57e508d76f5e16997ae09d04464220 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java d0e11932e8b5ba1393279137c8465a308e1d6bf5 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 74e31334bc139c47eb8b0beee46ee7bad62a2f80 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72030 --- src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java https://reviews.apache.org/r/30891/#comment117964 The cohesion of the HostOffer class seems to be rather low. The new field `staticallyBannedOffers` has not much in common with the other members. We might improve this situation by performing the `isStaticallyBanned` check within an overload of the `getWeaklyConsistentOffers` method. - Stephan Erb On Feb. 11, 2015, 9:38 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 11, 2015, 9:38 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java f66383830140e5eaba436f35ebb5192eee65947a src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ce47ff152e303fd2116bc3b9e91c0c1a8f76f258 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java e1c29747c9854cf75bf63f6f085cf40ca68989af src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 4cf602ad32b972c18eb5a81e9b2f59c67859bdb2 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 5647349854a5e04de749c4d809684a0066d4da06 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 6cc13231560996b144101eba36577f49017aba06 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72036 --- This patch does not apply cleanly on master (7b531e9), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 11, 2015, 9:38 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 11, 2015, 9:38 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java f66383830140e5eaba436f35ebb5192eee65947a src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ce47ff152e303fd2116bc3b9e91c0c1a8f76f258 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java e1c29747c9854cf75bf63f6f085cf40ca68989af src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 4cf602ad32b972c18eb5a81e9b2f59c67859bdb2 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 5647349854a5e04de749c4d809684a0066d4da06 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 6cc13231560996b144101eba36577f49017aba06 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.
On Feb. 11, 2015, 10:22 p.m., Stephan Erb wrote: src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java, line 316 https://reviews.apache.org/r/30891/diff/1/?file=861043#file861043line316 The cohesion of the HostOffer class seems to be rather low. The new field `staticallyBannedOffers` has not much in common with the other members. We might improve this situation by performing the `isStaticallyBanned` check within an overload of the `getWeaklyConsistentOffers` method. Yeah, this was my original approach but it turned out to be hurting performance too much in benchmark testing (due to excessive filtering and dual map lookup). This is much cleaner and faster, so I'd rather stick with it as I am more concerned about performance than private class cohesiveness in this case. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72030 --- On Feb. 11, 2015, 9:38 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 11, 2015, 9:38 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-909 https://issues.apache.org/jira/browse/AURORA-909 Repository: aurora Description --- Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned offers. Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a parent. Original RB: https://reviews.apache.org/r/28617/ Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java f66383830140e5eaba436f35ebb5192eee65947a src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java ce47ff152e303fd2116bc3b9e91c0c1a8f76f258 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java e1c29747c9854cf75bf63f6f085cf40ca68989af src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 4cf602ad32b972c18eb5a81e9b2f59c67859bdb2 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 5647349854a5e04de749c4d809684a0066d4da06 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 6cc13231560996b144101eba36577f49017aba06 Diff: https://reviews.apache.org/r/30891/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
