addressing issue 11, added support for exponential backoff #12

DrClick · 2017-09-08T07:44:34Z

I added support for this. Thanks for a great project!

CLAassistant · 2017-09-08T07:44:40Z

All committers have signed the CLA.

diego-alvarez-hs · 2017-09-08T16:46:33Z

Thanks for your contribution @DrClick , we'll take a look at this during the day and/or next week.

diego-alvarez-hs · 2017-09-08T20:38:56Z

We were wondering what was the reason behind needing an exponential backoff here? given that only 1 request makes it through every retryDelay.

So every retryDelay (let's say is configured to 2 seconds) will allow a single call pass to test if it can reset the CB, so there is no harm to let every 2 seconds to let pass a single operation.

Also having an unbounded exponential backoff would make the retryDelay become really large in a short period of time.

Unless you have really short configured retryDelay that would make the exponential backoff perform a little bit better, so this PR would need some sort of maxRetryDelay to have a limit on the calculated delay.

diego-alvarez-hs · 2017-09-08T20:44:09Z

src/main/scala/com/hootsuite/circuitbreaker/CircuitBreaker.scala

-    val retryAt: Long = System.currentTimeMillis() + cb.retryDelay.toMillis
+  class BrokenState(cb: CircuitBreaker, retryCount: Int=0) extends State {
+    val retryDelay: Long = cb.retryDelay.toMillis * Math.pow(2,
+      if (cb.isExponentialBackoff) retryCount else 1).toLong


when isExponentialBackoff is false, this will make the cb.retryDelay.toMillis to be multiplied by 2 as Math.pow(2.0, 1.0) == 2.0

Yup, good catch, I changed this. Will address.

DrClick · 2017-09-08T20:48:34Z

The reason behind this is when we run 1000s these processes, this allows the failure to be backed off where just the process of trying to allocate the resource causes a load on the resource. For instance Cassandra goes down, You have 1000s of clients connected, everyone is retrying every 2seconds causing massive load on other resources, I can agree on the unbounded I also thnk a more standard implementation adding jitter is probably reasonable.

I propose adding the following, jitter to the exponential delay (so not all clients try at the same time)
A maximum retry

If maximum retry is reached, I wonder what state you would like the CB left in?

Also see http://docs.aws.amazon.com/general/latest/gr/api-retries.html

jriecken · 2017-09-08T20:49:16Z

src/main/scala/com/hootsuite/circuitbreaker/CircuitBreaker.scala

@@ -345,6 +349,7 @@ case class CircuitBreakerBuilder(
  name: String,
  failLimit: Int,
  retryDelay: FiniteDuration,
+  isExponentialBackoff: Boolean = false,
  isResultFailure: PartialFunction[Any, Boolean] = { case _ => false },


Should probably add a maxRetryDelay: FiniteDuration that bounds the exponential growth.

Would also be nice if there were a withExponentialBackoff(maxRetryDelay: FiniteDuration): CircuitBreakerBuilder convenience method on the builder

jriecken · 2017-09-08T20:54:33Z

I think if the max retry is reached, you would just continue using that max value rather than continually increasing it.

Having a random jitter added/removed to the calculated retry sounds like a good idea to avoid thundering herds

diego-alvarez-hs · 2017-09-08T20:55:16Z

src/test/scala/com/hootsuite/circuitbreaker/CircuitBreakerTest.scala

  private val defaultRetryDelay = Duration(numMillisecondsForRetryDelay, TimeUnit.MILLISECONDS)

-  private def waitUntilRetryDelayHasExpired() = Thread.sleep(2 * numMillisecondsForRetryDelay)
+  private def waitUntilRetryDelayHasExpired(millis: Option[Long]=None) = {


🤔 why scalafmt didn't fix this...

diego-alvarez-hs · 2017-09-08T20:58:25Z

@DrClick what IDE are you using? as I run sbt test and I'm getting that scalafmt fixes some issues:

diego-alvarez-hs · 2017-09-08T21:01:17Z

@DrClick your use case seems reasonable to have the exponential backoff, I think we use differently here the CB so we have most of the time a few CB per external dependency, not 1000s as your case.

Please see @jriecken suggestions regarding the max retry value

DrClick · 2017-09-08T21:06:34Z

@diego-alvarez-hs I am using intellij. I noticed there were a few formatting differences. I am curious, I was consider refactoring the other tests as I did mine, wapping the repated calls in a method. Are you ok with this before I go forward, to be clear:

taking this:

      // first failure; let it through
      whenReady(Future(protectedOperation(1, 0)).failed, timeout(Span(100, Millis))) { e =>
        withClue(s"${cb.name} : first failure") {
          e shouldBe a[ArithmeticException]
        }
      }

and changing to this:

//first failure; let it through
assertArithException("first failure")

everywhere. I think this makes the test a bit more readable.

diego-alvarez-hs · 2017-09-08T21:11:25Z

Sure! that would be great, this code base needs some ❤️
We started adding scalafmt and we're planning to improve it a little bit with our current scala practices.
It's good to know that people like you are using it so we can prioritize this.

DrClick · 2017-09-11T18:58:00Z

I updated the code for the retrycap and cleaned up the tests

diego-alvarez-hs · 2017-09-13T16:13:25Z

Thanks @DrClick , I'll review both PRs later today/tomorrow.

MavenRain · 2019-02-06T08:17:29Z

src/main/scala/com/hootsuite/circuitbreaker/CircuitBreaker.scala

+        0
+      }
+
+      val retryCap = cb.exponentialRetryCap match {


Consider the following: val retryCap = cb.exponentialRetryCap.getOrElse(Int.MaxValue)

MavenRain · 2019-02-06T08:19:28Z

src/test/scala/com/hootsuite/circuitbreaker/CircuitBreakerTest.scala

+
+  object SimpleOperation {
+
+    def operation(x: Int, y: Int): Int = x / y


Because of division by zero, you are going to want this to be Try-able: def operation(x: Int, y: Int): Try[Int] = Try(x / y)

MavenRain · 2019-02-06T08:20:33Z

src/test/scala/com/hootsuite/circuitbreaker/CircuitBreakerTest.scala

+
+    def operation(x: Int, y: Int): Int = x / y
+
+    def asyncOperation(x: Int, y: Int): Future[Int] = Future {


Consider: def asyncOperation(x: Int, y: int): Future[Try[Int]] = Future(operation(x, y))

MavenRain · 2019-02-06T08:22:15Z

src/test/scala/com/hootsuite/circuitbreaker/CircuitBreakerTest.scala

+    simpleBuilders(retryDelay = Duration(2, TimeUnit.SECONDS)).map(_.build()).foreach { x =>
+      implicit val cb: CircuitBreaker = x
+
+      (1 to 2).foreach { i => assertArithException(s"failure #$i") }


For such a small range, I would just duplicate:

assertArithException("failure #$1") assertArithException("failure #$2")

addressing issue 11, added support for exponential backoff

0d9bf50

diego-alvarez-hs reviewed Sep 8, 2017

View reviewed changes

changed 1 to 0 for exponent

ff3eec3

jriecken reviewed Sep 8, 2017

View reviewed changes

diego-alvarez-hs reviewed Sep 8, 2017

View reviewed changes

added exponentialRetryCap, tests and cleaned up tests in general

d39dc4f

diego-alvarez-hs mentioned this pull request Sep 15, 2017

Add waiting state and clean up tests #15

Open

MavenRain reviewed Feb 6, 2019

View reviewed changes


		object SimpleOperation {

		def operation(x: Int, y: Int): Int = x / y


		def operation(x: Int, y: Int): Int = x / y

		def asyncOperation(x: Int, y: Int): Future[Int] = Future {

addressing issue 11, added support for exponential backoff #12

Are you sure you want to change the base?

addressing issue 11, added support for exponential backoff #12

Uh oh!

Conversation

DrClick commented Sep 8, 2017

Uh oh!

CLAassistant commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diego-alvarez-hs commented Sep 8, 2017

Uh oh!

diego-alvarez-hs commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrClick commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jriecken commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diego-alvarez-hs commented Sep 8, 2017

Uh oh!

diego-alvarez-hs commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrClick commented Sep 8, 2017

Uh oh!

diego-alvarez-hs commented Sep 8, 2017

Uh oh!

DrClick commented Sep 11, 2017

Uh oh!

diego-alvarez-hs commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MavenRain Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CLAassistant commented Sep 8, 2017 •

edited

Loading

DrClick commented Sep 8, 2017 •

edited

Loading

diego-alvarez-hs commented Sep 8, 2017 •

edited

Loading

MavenRain Feb 6, 2019 •

edited

Loading