Return-Path: X-Original-To: apmail-aurora-reviews-archive@minotaur.apache.org Delivered-To: apmail-aurora-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id DC7B41851C for ; Sun, 3 Jan 2016 00:55:16 +0000 (UTC) Received: (qmail 22238 invoked by uid 500); 3 Jan 2016 00:55:16 -0000 Delivered-To: apmail-aurora-reviews-archive@aurora.apache.org Received: (qmail 22179 invoked by uid 500); 3 Jan 2016 00:55:16 -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 22162 invoked by uid 99); 3 Jan 2016 00:55:16 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 03 Jan 2016 00:55:16 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 3EF421CC3EF; Sun, 3 Jan 2016 00:55:15 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============3870670334298092890==" MIME-Version: 1.0 Subject: Re: Review Request 41717: Very Very WIP: Add jittering as an option to BackoffStrategy. From: "Bill Farner" To: "Bill Farner" , "Stephan Erb" Cc: "Tony Dong" , "Aurora" Date: Sun, 03 Jan 2016 00:55:15 -0000 Message-ID: <20160103005515.4181.77136@reviews.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/41717/ X-Sender: "Bill Farner" References: <20151227221112.4181.83644@reviews.apache.org> In-Reply-To: <20151227221112.4181.83644@reviews.apache.org> Reply-To: "Bill Farner" X-ReviewRequest-Repository: aurora --===============3870670334298092890== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Dec. 27, 2015, 2:11 p.m., Stephan Erb wrote: > > Thanks for your effort. > > > > I believe `ExpBackoffEqualJitter.java` is the most usefull strategy. It is the closest to the current design while still resolving the problem depicted in the AWS article http://d3j06wsuecq21p.cloudfront.net/assets/backoff/backoff_expo_ts.png. Providing different strategies and allowing the enduser/operator to select one might be overkill. There is no obvious tradeoff that we have to make configurable. > > Bill Farner wrote: > I agree. This is an interesting experiment, but i think it's best to choose an approximation for a universal solution and avoid adding knobs. > > Tony Dong wrote: > Sounds reasonable, how would you guys like to enable jitter? I feel like a config that flips it on/off is not the most flexible thing, but that isn't a big issue if we never add more strategies. > > Stephan Erb wrote: > If the jittering code works as expected, I don't see a reason for a toggle at all. Bill, what do you think? +1 make it work out of the box, no toggle. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41717/#review111942 ----------------------------------------------------------- On Dec. 25, 2015, 2:18 a.m., Tony Dong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41717/ > ----------------------------------------------------------- > > (Updated Dec. 25, 2015, 2:18 a.m.) > > > Review request for Aurora, Stephan Erb and Bill Farner. > > > Repository: aurora > > > Description > ------- > > The general idea is that we should be able to specify different > types of BackoffStrategies, with varying configurations and all > the legwork will be handled via injection. This change not only > introduces different BackoffStrategies which can be selected via > command line arguments, but also introduces a BackoffOptions > class which makes configuring these different strategies more > standardized. > > - TODO: Update everywhere that uses BackoffStrategy to use Injection. > - TODO: Add tests for all the different backoff strategies. > - TODO: Add tests for nextLong(). > - TODO: Make BackoffHelper injectable and configurable with the BackoffStrategy Types. > - TODO: Update Docs everywhere. > > > Diffs > ----- > > commons/src/main/java/org/apache/aurora/common/args/parsers/EnumParser.java 9f6a3ff977ef6a0e2d472a8e2cf90e7f9a63a985 > commons/src/main/java/org/apache/aurora/common/util/BackoffHelper.java b251e9b84aa33c7c1e1366aaf8872a367864e408 > commons/src/main/java/org/apache/aurora/common/util/BackoffStrategy.java d954762496a2cbc0a098964bc2686fdaf6213e06 > commons/src/main/java/org/apache/aurora/common/util/Random.java 90d111e357c7aded81c3c79c4b814adf55424802 > commons/src/main/java/org/apache/aurora/common/util/TruncatedBinaryBackoff.java fd74b9f37c6cc24c7ea1cb239ba6354661d931e2 > commons/src/main/java/org/apache/aurora/common/util/backoff/BackoffStrategyType.java PRE-CREATION > commons/src/main/java/org/apache/aurora/common/util/backoff/ExpBackoffDecorrelatedJitter.java PRE-CREATION > commons/src/main/java/org/apache/aurora/common/util/backoff/ExpBackoffEqualJitter.java PRE-CREATION > commons/src/main/java/org/apache/aurora/common/util/backoff/ExpBackoffFullJitter.java PRE-CREATION > commons/src/main/java/org/apache/aurora/common/util/backoff/TruncatedBinaryBackoff.java PRE-CREATION > commons/src/main/java/org/apache/aurora/common/zookeeper/Group.java d3681700e7f993da711f3b1ac87dc0335075cf71 > commons/src/main/java/org/apache/aurora/common/zookeeper/ServerSetImpl.java 6a0a314ffc27e73024ac8ac7c9e3a7ba83567c3a > commons/src/test/java/org/apache/aurora/common/util/BackoffHelperTest.java 78ba8fe7e0640b7c4b04b78384e15a33e1b81b2f > commons/src/test/java/org/apache/aurora/common/util/SystemRandomTest.java PRE-CREATION > commons/src/test/java/org/apache/aurora/common/util/TruncatedBinaryBackoffTest.java 127a60331f8560d98733bd16654087f840abef72 > commons/src/test/java/org/apache/aurora/common/util/backoff/BackoffOptionsTest.java PRE-CREATION > commons/src/test/java/org/apache/aurora/common/util/backoff/ExpBackoffDecorrelatedJitterTest.java PRE-CREATION > commons/src/test/java/org/apache/aurora/common/util/backoff/ExpBackoffEqualJitterTest.java PRE-CREATION > commons/src/test/java/org/apache/aurora/common/util/backoff/ExpBackoffFullJitterTest.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java f355bc101252fb433c7437a791e6e92f94462fa6 > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronModule.java 264537180da91f59173301bf20b549ea01c0d5cb > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java fbc589e9a7592cce6d92c4e987cde2e056406c3a > src/main/java/org/apache/aurora/scheduler/reconciliation/KillRetry.java 119ef71ab2993f1ac74bab25bec3ec5cd4a67896 > src/main/java/org/apache/aurora/scheduler/reconciliation/ReconciliationModule.java 7dae70c4c9cb2efbf66e1d269f96676ff450eeaf > src/main/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculator.java 291bf5f0baefef6dd10d19ec89e173ce495e6380 > src/main/java/org/apache/aurora/scheduler/scheduling/SchedulingModule.java 577edcbf362493d577e2f12c876f1dbb9387ad79 > src/main/java/org/apache/aurora/scheduler/scheduling/TaskGroups.java c044ebe6f72183a67462bbd8e5be983eb592c3e9 > src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java 5dd4aba92b2627b646087fce8118d5ebfeb75f49 > src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 19c8a1fe06a333324022da11f74d7c96b2942587 > src/test/java/org/apache/aurora/scheduler/reconciliation/KillRetryTest.java a561d0909cef27b24334165f0d40cfd734b2c9a6 > src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java b380f21ac169b4991158f39dc70526e11fca54f0 > src/test/java/org/apache/aurora/scheduler/scheduling/TaskGroupsTest.java 95cf25eda0a5bfc0cc4c46d1439ebe9d5359ce79 > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 2024b2c50d5d1e44f3f95b915c8bcd58e39379cb > > Diff: https://reviews.apache.org/r/41717/diff/ > > > Testing > ------- > > > Thanks, > > Tony Dong > > --===============3870670334298092890==--