----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57717/#review169264 -----------------------------------------------------------
Looks good to me. Couple of small remarks below. src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java Lines 135 (patched) <https://reviews.apache.org/r/57717/#comment241605> General question: Is this value only used internally or also shown somewhere? src/main/java/org/apache/aurora/scheduler/mesos/MesosCallbackHandler.java Lines 326-330 (patched) <https://reviews.apache.org/r/57717/#comment241607> In our clusters we set the offer filter to 0, so we have to be thoughtful with how we use the option here. If we ever decide to use this filter for declining an offer, we would build an andless loop where an offer will bounce between Mesos and Aurora constantly. It is OK now, as the filter does not seem to do anything if we accept in offer (?). However, maybe we should just use the default filter for now to prevent potential accidents in the future. src/main/java/org/apache/aurora/scheduler/mesos/MesosCallbackHandler.java Lines 333-334 (patched) <https://reviews.apache.org/r/57717/#comment241606> We run the `handleOffers` code in a callback. Should we move the portion of the `handleInverseOffer` code that relies on the storage lock (i.e. `drainForInverseOffer`) to a callback as well? src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java Lines 140-142 (patched) <https://reviews.apache.org/r/57717/#comment241608> I suppose we never call this method on the (legacy) SchedulerDriver? src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java Lines 281-291 (patched) <https://reviews.apache.org/r/57717/#comment241633> I am not sure if I get this one. My first assumption was we could write something like: ``` Ordering. .natural() .reverse() .onResultOf((Function<HostOffer, Instant>) o -> o.getUnavailabilityStart().or(Instant.MAX))); ``` Or would this miss some cases covered by the code in the patch? src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java Lines 79-80 (patched) <https://reviews.apache.org/r/57717/#comment241635> Why do we need the offer filter duration here? In any case, we also need `offer_hold_jitter_window` to be included. src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java Lines 83 (patched) <https://reviews.apache.org/r/57717/#comment241634> an -> and src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Lines 227-236 (patched) <https://reviews.apache.org/r/57717/#comment241638> The test using this helper will be waiting for over 2.5min. Would be great if you replace the sleeps with proper "wait until loops" in `assert_task_status` to gain some performance and robustness. For an example, see `test_observer_ui`. - Stephan Erb On March 17, 2017, 2:25 a.m., Zameer Manji wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57717/ > ----------------------------------------------------------- > > (Updated March 17, 2017, 2:25 a.m.) > > > Review request for Aurora, David McLaughlin and Stephan Erb. > > > Bugs: AURORA-1904 > https://issues.apache.org/jira/browse/AURORA-1904 > > > Repository: aurora > > > Description > ------- > > This adds support for Mesos Maintenance per the design doc[1]. > > Per the design the scheduler gains another parameter, > `unavailability_threshold`. With this threshold the scheduler does the > following: > > 1. Accept all inverse offers from Mesos. > 2. Drain when accepting an inverse offer if the unavailability starts within > the thereshold. > 3. Veto any offers with unavailability starting within the threshold. > 4. Penalize offers that have unavailablity information > > For readability and safety the time based code uses the new `java.time` > package in Java 8, primarily relying on the `Instant` class. > > [1]: > https://docs.google.com/document/d/1Z7dFAm6I1nrBE9S5WHw0D0LApBumkIbHrk0-ceoD2YI/edit#heading=h.n5tvzjaj9llx > > > Diffs > ----- > > commons/src/main/java/org/apache/aurora/common/util/Clock.java > 5c4ced1ffe7827c0e529d17cb51db42fd1b762ff > commons/src/main/java/org/apache/aurora/common/util/testing/FakeClock.java > 104f2c64196da16d68a85e365f1dc762547e1e36 > examples/vagrant/upstart/aurora-scheduler.conf > 31fa0368435a179698d1a745331a85430049762e > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java > 45f59c0bd09f81916c95345233e6642b4cf81830 > src/main/java/org/apache/aurora/scheduler/HostOffer.java > 23f0600d64e1e15f4856f397e839e3d1c87f3b96 > src/main/java/org/apache/aurora/scheduler/base/Conversions.java > 8295216dc651eff357c4f3c51c8a53052244c6bf > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java > bb1a960a4c77f48b0ceaa213bd27546551f384f9 > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java > 60097d91d836e2686d6e90571f13a2fbfd88ae14 > src/main/java/org/apache/aurora/scheduler/mesos/Driver.java > 71547ce931e0161adfc5de43f367b3ec43aa17e8 > src/main/java/org/apache/aurora/scheduler/mesos/MesosCallbackHandler.java > 801551bce7879989d93d2d32a8fe28a891312c73 > src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java > f65a29d7ad8bc49784e324e674f30a6728a9d4ae > > src/main/java/org/apache/aurora/scheduler/mesos/VersionedMesosSchedulerImpl.java > 84e3f47636d95521600e9a4c4d5b8bc8bbbff8cf > > src/main/java/org/apache/aurora/scheduler/mesos/VersionedSchedulerDriverService.java > d928d02cab087991a8cd8896d4366f6e5dca0913 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java > e16e36ed360ef9ca371df9084365ea88cfb6e7ce > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java > 202cae96ffc5b49e638b973a273f7983137b5baf > > src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java > ba49e7a4ccfaddbd85218018b0bbad5efab41d99 > src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java > 574efc9e44a21fc7cdc0d316d6c51f47cd673ce3 > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java > da378e84ee65a658ff2382489d3ab6d5f6451b5f > > src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java > 1d7f9f45e7a65838e2c826b4b21a31c7944eab19 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosCallbackHandlerTest.java > 80f631e9024e266fe823d845193b19c1d559a5ef > > src/test/java/org/apache/aurora/scheduler/mesos/VersionedSchedulerDriverServiceTest.java > 72aede85829f087bc88760e8b564d25aceb8aed8 > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > 49d4e82cc03144b80292fe43066a6cc4d7aed88f > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > 02bfc51a7cba1116334dbfe30e0abe05ba3fbb4a > > src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java > ae83dea05e10ebab0c0b07d60386d0faf78fb7e9 > src/test/sh/org/apache/aurora/e2e/generate_mesos_maintenance_schedule.py > PRE-CREATION > src/test/sh/org/apache/aurora/e2e/http/http_example.aurora > de8179228d9359900eadf4084355ea257bea45ba > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > 80b4c54774a02fdc2ee0e36d26f81aedd2e0055e > > > Diff: https://reviews.apache.org/r/57717/diff/1/ > > > Testing > ------- > > e2e tests > > > Thanks, > > Zameer Manji > >
