From reviews-return-20502-archive-asf-public=cust-asf.ponee.io@aurora.apache.org Fri Jan 19 19:12:13 2018 Return-Path: X-Original-To: archive-asf-public@eu.ponee.io Delivered-To: archive-asf-public@eu.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by mx-eu-01.ponee.io (Postfix) with ESMTP id 58B10180607 for ; Fri, 19 Jan 2018 19:12:13 +0100 (CET) Received: by cust-asf.ponee.io (Postfix) id 48BEA160C36; Fri, 19 Jan 2018 18:12:13 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 684C9160C1B for ; Fri, 19 Jan 2018 19:12:12 +0100 (CET) Received: (qmail 54094 invoked by uid 500); 19 Jan 2018 18:12:11 -0000 Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@aurora.apache.org Delivered-To: mailing list reviews@aurora.apache.org Received: (qmail 54082 invoked by uid 99); 19 Jan 2018 18:12:11 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 19 Jan 2018 18:12:11 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id CF688C00C9; Fri, 19 Jan 2018 18:12:10 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 2.54 X-Spam-Level: ** X-Spam-Status: No, score=2.54 tagged_above=-999 required=6.31 tests=[HTML_MESSAGE=2, KAM_LAZY_DOMAIN_SECURITY=1, KAM_LOTSOFHASH=0.25, RCVD_IN_DNSWL_LOW=-0.7, T_RP_MATCHES_RCVD=-0.01] autolearn=disabled Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id CJv6PgTt0F1l; Fri, 19 Jan 2018 18:12:09 +0000 (UTC) Received: from mailrelay1-us-west.apache.org (mailrelay1-us-west.apache.org [209.188.14.139]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTP id CBBED5F3FE; Fri, 19 Jan 2018 18:12:08 +0000 (UTC) Received: from reviews.apache.org (unknown [10.41.0.12]) by mailrelay1-us-west.apache.org (ASF Mail Server at mailrelay1-us-west.apache.org) with ESMTP id 8677CE0373; Fri, 19 Jan 2018 18:12:07 +0000 (UTC) Received: from reviews-vm2.apache.org (localhost [IPv6:::1]) by reviews.apache.org (ASF Mail Server at reviews-vm2.apache.org) with ESMTP id 2C073C405CF; Fri, 19 Jan 2018 18:12:06 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============3532494005806133046==" MIME-Version: 1.0 Subject: Re: Review Request 65233: Allow for injection of custom OfferSets, removed OfferOrder and OfferSelector From: Bill Farner To: Bill Farner , David McLaughlin , Stephan Erb Cc: Aurora , Jordan Ly Date: Fri, 19 Jan 2018 18:12:06 -0000 Message-ID: <20180119181206.64895.12365@reviews-vm2.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Bill Farner X-ReviewGroup: Aurora X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/65233/ X-Sender: Bill Farner X-ReviewBoard-ShipIt: 1 References: <20180119181135.64727.62637@reviews-vm2.apache.org> In-Reply-To: <20180119181135.64727.62637@reviews-vm2.apache.org> X-ReviewBoard-Diff-For: src/test/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelectorTest.java X-ReviewBoard-Diff-For: src/main/java/org/apache/aurora/scheduler/scheduling/OfferSelector.java X-ReviewBoard-Diff-For: src/main/java/org/apache/aurora/scheduler/offers/OfferOrderBuilder.java X-ReviewBoard-Diff-For: src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java X-ReviewBoard-Diff-For: src/main/java/org/apache/aurora/scheduler/offers/OfferOrder.java X-ReviewBoard-Diff-For: src/main/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelector.java X-ReviewBoard-Diff-For: src/main/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelectorModule.java X-ReviewBoard-Diff-For: src/main/java/org/apache/aurora/scheduler/offers/OfferSetImpl.java Reply-To: Bill Farner X-ReviewRequest-Repository: aurora --===============3532494005806133046== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65233/#review195838 ----------------------------------------------------------- Ship it! All nits, nice patch! src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java Line 66 (original), 65 (patched) Inject `OfferSet` rather than `OfferSettings`. src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java Lines 180-182 (original), 177-180 (patched) Revert. If there is no downside to chaining, i think it's much more readable. src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java Line 96 (original), 96 (patched) Revert noop src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java Line 91 (original), 91 (patched) `offer_set_module` singular -> no custom splitter, no `List`. src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 9 (patched) s/collection/set/. That distinction makes me comfortable leaving most of these methods without docs. src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 12 (patched) Remove VisibleForTesting src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 23 (patched) Up to you, but i'm not seeing a good reason to return `Iterable` over `Stream` here and below. It has the upside of no extra hoops for filtering and no need to protect from modification. src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 23-30 (patched) How about this renaming: `getAll()` -> `values()` (aligning with Collection terminology) `getAll(...)` -> `getOrdered(...)` src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java Lines 26 (patched) Please expand this doc a bit. What does context mean? src/main/java/org/apache/aurora/scheduler/offers/OfferSetImpl.java Lines 78 (patched) Omit comment, non-informative src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java Lines 72 (patched) remove src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java Line 241 (original), 239 (patched) Remove comment src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java Line 192 (original) If i'm reading correctly, this test is still relevant with the default `OfferSetImpl`. Any reason we should remove it? - Bill Farner On Jan. 19, 2018, 10:11 a.m., Jordan Ly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65233/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2018, 10:11 a.m.) > > > Review request for Aurora, David McLaughlin, Stephan Erb, and Bill Farner. > > > Repository: aurora > > > Description > ------- > > The goal of this patch is to provide a more reasonable abstraction for custom scheduling. > > Currently, we have `OfferSelector`, `OfferOrder`, and recently I proposed `FilterableOfferCollection` (https://reviews.apache.org/r/65225/). These were all created in order to provide more customization within the scheduling loop. However, they suffer from being leaky and too disparate. This patch hopes to combine all of those components into a single `OfferSet` which can be injected and used by HostOffers. This interface allows for custom scheduling logic to be co-located with custom data structures for `offers` as opposed to being passed around as different components. > > The following options will be removed from the last 0.19 to now: > ``` > -offer_order > -offer_order_modules > ``` > > > Diffs > ----- > > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java f44cb61f5d1805aeb0f98135453dbc82f3bc9ad4 > src/main/java/org/apache/aurora/scheduler/config/CliOptions.java e4e535824adb86c98c6173a20f97a9a90b08483a > src/main/java/org/apache/aurora/scheduler/offers/HostOffers.java 2ea7a01085b87c8ed6765537a8005e2349784ab0 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8f9e33d81be9087821784a8a08079a1736d9cb63 > src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java de16c14c887d4225e0629b1580eb5e740798f1f7 > src/main/java/org/apache/aurora/scheduler/offers/OfferOrder.java 7c99c309e0e97519719905f725bb9f6a59d2a7f5 > src/main/java/org/apache/aurora/scheduler/offers/OfferOrderBuilder.java 1260ef19506acb8e8a937d4fd7b7152361bd3c40 > src/main/java/org/apache/aurora/scheduler/offers/OfferSet.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OfferSetImpl.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java 1e36b2c3094e99f05aa4a4f098a48df2293b4320 > src/main/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelector.java 5f4f452cb133e2be73f881700a54ba4c8e60a35a > src/main/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelectorModule.java 4d36487f430483444314a8cefd133e7c766075de > src/main/java/org/apache/aurora/scheduler/scheduling/OfferSelector.java 6f5d55c5a586ac923b5f375d1935ec2049673ec0 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImpl.java ec416cc12b2fb4a11a273c77dbe72b79f96fa7e1 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java d101a497896ba3b8ca7984a63f861494cbddbb35 > src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java c84fad3b615c49ca7527ae3f7fa116aa8f90ff0c > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 28224a5c587b235ccc86a286e796eeca66929232 > src/test/java/org/apache/aurora/scheduler/scheduling/FirstFitOfferSelectorTest.java e6b2b7465fb4fd432cda50d1e714dfc587764689 > src/test/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplTest.java f84941edd96d974a6c2d7e087324bc8fbbac3b0a > > > Diff: https://reviews.apache.org/r/65233/diff/2/ > > > Testing > ------- > > `./gradlew test` > `./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh` > > Testing injection of a custom `OfferSet` onto a test cluster. > > > Thanks, > > Jordan Ly > > --===============3532494005806133046==--