aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Farner <wfar...@apache.org>
Subject Re: Review Request 63973: Enable custom offer scoring modules for task assignment and injecting of custom OfferManagers
Date Tue, 21 Nov 2017 21:27:40 GMT

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



This looks great to me overall.  The issue of custom modules is the only thing that causes
me to hold back a 'Ship It'.


src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java
Lines 58 (patched)
<https://reviews.apache.org/r/63973/#comment269492>

    Pre-dates this patch, but stands out now since it's green - it's odd to see a concurrent
structure willfully used alongside `HashMap`.
    
    For consistency, consider using `Sets.newHashSet()` here and adding `synchronized` to
`isGloballyBanned()`.



src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java
Lines 190 (patched)
<https://reviews.apache.org/r/63973/#comment269493>

    Must be `synchronized` since it's lazily accessed via `getAllMatching`.



src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java
Lines 199 (patched)
<https://reviews.apache.org/r/63973/#comment269494>

    Consider making this `static` and passing `SchedulingFilter`.  This makes it clear that
it is not accessing member fields that require synchronization.



src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java
Lines 46 (patched)
<https://reviews.apache.org/r/63973/#comment269495>

    This doesn't seem like a reasonable customization to provide or implement; it is complex
and has strong ties to the overall behavior of the scheduler (e.g. interacting with `Driver`).
    
    What are you hoping to accomplish with this?



src/main/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImpl.java
Lines 140 (patched)
<https://reviews.apache.org/r/63973/#comment269499>

    Consider moving this up to :122 and replacing `ImmutableSet<IAssignedTask> assignedtasks`.



src/main/java/org/apache/aurora/scheduler/state/StateModule.java
Line 45 (original), 46 (patched)
<https://reviews.apache.org/r/63973/#comment269498>

    I suggest removing this customization.  `OfferRanker` seems like a much more reasonable
level of abstraction.


- Bill Farner


On Nov. 20, 2017, 7:40 p.m., Jordan Ly wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63973/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2017, 7:40 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, Stephan Erb,
and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This is a rather large diff. Posting without tests first to get initial impressions/make
the review more digestible, but tests are soon to come.
> 
> Major portions of the refactor:
> 
> * Allows injection of custom `OfferManager` via `OfferManagerModule`
> * Refactor `OfferManager` to do filtering of offers (added `getMatching` and `getAllMatching`
methods) as opposed to TaskAssigner
> * Refactor `TaskAssigner`, allow for injection of custom "scoring" class through `OfferRanker`
interface
> 
> And some minor things as well:
> 
> * Moved `TaskAssignerImpl`, `TaskSchedulerImpl`, and `HostOffers` into their own upper-level
classes
> * Moved `TaskAssigner` to the `scheduling` package and out of the `state` package
> * Renamed some methods in `OfferManager` to avoid code stutter
> * Renaming of some classes (e.g. `FirstFitTaskAssigner` -> `TaskAssignerImpl`)
> * And a slew of others
> 
> Overall, the goal of this refactor is to allow for the easy injection of custom offer
"scoring" modules via the `OfferRanker` interface. Callers will not have to replace the entire
`TaskAssigner` module, but instead just provide a function that chooses from a list of offers
that should already fit the task.
> 
> Going through this review, I would start with the `maybeAssign`/`launchUsingOffer` methods
in `TaskAssigner`. Then, I would look at `getMatching`/`getAllMatching` in `HostOffers`.
> 
> 
> Diffs
> -----
> 
>   docs/reference/scheduler-configuration.md d17541f9458650240983276b69f749a854057aa8

>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 3204cca5446d0f798ace622d334473bf6db13733

>   src/main/java/org/apache/aurora/scheduler/config/CliOptions.java d4537e386af53ec9bff661ed23d61a5bd4893edf

>   src/main/java/org/apache/aurora/scheduler/filter/AttributeAggregate.java 60f141d34d862f200ae1346a38a71bba57551059

>   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 36608a9f027c95723c31f9915852112beb367223

>   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java df51d4cf4893899613683603ab4aa9aefa88faa6

>   src/main/java/org/apache/aurora/scheduler/http/Offers.java f22ca6e5402d8ec7241376de14a9051ab0cfbfc6

>   src/main/java/org/apache/aurora/scheduler/mesos/MesosCallbackHandler.java fd5874d51cfbca1b98d15623a9c845d208d43b1f

>   src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 96b0f46e69255944fa0f00621db315e22d68515c

>   src/main/java/org/apache/aurora/scheduler/offers/OfferManagerImpl.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java 57fc1a18f1b82decc780ba0069ab4dd0c125f77d

>   src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 4a6ea8dac97ed2f5df47e4d16fa65a3b516988bd

>   src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java 766d3b24fd64de96b82d3dd2a17f7a80331889d8

>   src/main/java/org/apache/aurora/scheduler/preemptor/Preemptor.java 82a0ff67e9f70fed305f8f1bf38e3cf7955b9ce7

>   src/main/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferRanker.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/scheduling/OfferRanker.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java 0796712a00e3cba595ceb049833933b9a70c2f9a

>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssigner.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 0002b0ceb429a404ac9dd0ae7e5ea9f6362fa100

>   src/main/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImpl.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/state/FirstFitTaskAssignerModule.java dc244eeaa0d292a06d8750cb340fb379701ad49f

>   src/main/java/org/apache/aurora/scheduler/state/StateModule.java c03fff11ea3a4086f9daaa8b07315006c1b481e4

>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java cdd0d15872248b19d5eb52d25537f2f863fd5c77

>   src/main/java/org/apache/aurora/scheduler/stats/AsyncStatsModule.java 6033c01477bec7a491128e3089ffc1b1c3f1c150

> 
> 
> Diff: https://reviews.apache.org/r/63973/diff/1/
> 
> 
> Testing
> -------
> 
> Tests coming soon, but they should be relatively close to what we have now on master
since we don't really add/remove features but just move them around.
> 
> Ran some benchmarks as well to ensure performance parity:
> 
> ```
> MASTER
> Benchmark                                                                      Mode 
Cnt       Score       Error  Units
> SchedulingBenchmarks.ClusterFullUtilizationBenchmark.runBenchmark             thrpt 
 10  113910.922 ± 26263.903  ops/s
> SchedulingBenchmarks.FillClusterBenchmark.runBenchmark                        thrpt 
 10     263.043 ±   299.686  ops/s
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark    thrpt 
 10   12353.669 ±   433.182  ops/s
> SchedulingBenchmarks.LimitConstraintMismatchSchedulingBenchmark.runBenchmark  thrpt 
 10     893.263 ±    54.882  ops/s
> SchedulingBenchmarks.ValueConstraintMismatchSchedulingBenchmark.runBenchmark  thrpt 
 10   11478.420 ±  1068.819  ops/s
> ```
> and
> ```
> MY PATCH
> Benchmark                                                                      Mode 
Cnt       Score       Error  Units
> SchedulingBenchmarks.ClusterFullUtilizationBenchmark.runBenchmark             thrpt 
 10  124041.966 ± 33331.424  ops/s
> SchedulingBenchmarks.FillClusterBenchmark.runBenchmark                        thrpt 
 10    1030.434 ±  1223.539  ops/s
> SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.runBenchmark    thrpt 
 10   12305.154 ±   375.814  ops/s
> SchedulingBenchmarks.LimitConstraintMismatchSchedulingBenchmark.runBenchmark  thrpt 
 10    1080.267 ±    74.635  ops/s
> SchedulingBenchmarks.ValueConstraintMismatchSchedulingBenchmark.runBenchmark  thrpt 
 10   11604.642 ±   586.248  ops/s
> ```
> 
> Not exactly sure why `FillClusterBenchmark` increased so dramatically -- I will look
into that while testing as well.
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message