twitter / storehaus

Storehaus is a library that makes it easy to work with asynchronous key value stores
Apache License 2.0
464 stars 92 forks source link

RetryingReadableStore to support retrying on only certain exceptions #214

Open tanin47 opened 10 years ago

tanin47 commented 10 years ago

I would like to only retry on some exceptions. For example, these exceptions:

  val DefaultRetryExceptions = Seq(
    classOf[GlobalRequestTimeoutException],
    classOf[ServiceTimeoutException],
    classOf[InterruptedException],
    classOf[NoBrokersAvailableException])

But I wouldn't want to retry on some other exceptions (e.g. application-level exceptions). Therefore, I need a way to specify it.

rubanm commented 10 years ago

What do you think of passing in (shouldRetry : Exception => Boolean) instead?

tanin47 commented 10 years ago

Good idea! I think that'd be more flexible for consumers.

tanin47 commented 10 years ago

I'm not sure if I should modify pred: Option[V] => Boolean to pred:Try[Option[V]] => Try[Option[V]]. In this way, a consumer can decide to ignore certain exceptions and retry on some exceptions.

It'll look like:

class RetryingReadableStore[-K, +V](store: ReadableStore[K, V], backoffs: Iterable[Duration])(pred: Try[Option[V]] => Try[Option[V]])(implicit timer: Timer) extends ReadableStore[K, V] {

  private[this] def getWithRetry(k: K, backoffs: Iterable[Duration]): Future[Option[V]] =
    store.get(k) transform { vOpt =>
      Future.const(pred(vOpt))
    } transform {
      case Return(t) => Future.value(t)
      case Throw(e) =>
        backoffs.headOption match {
          case None => FutureOps.retriesExhaustedFor(k)
          case Some(interval) => interval match {
            case Duration.Zero => getWithRetry(k, backoffs.tail)
            case Duration.Top => FutureOps.missingValueFor(k)
            case _ => Future.flatten {
              timer.doLater(interval) {
                getWithRetry(k, backoffs.tail)
              }
            }
          }
        }
    }

  override def get(k: K) = getWithRetry(k, backoffs)
}

Please let me know what you think

rubanm commented 10 years ago

Not sure what pred should produce to denote an ignorable exception here? With the above code, seems like it would produce Return(optv) which doesn't say if this is the actual value to be returned to the consumer, or a retryable case?

An alternative can perhaps be Try[Option[V]] => Try[Either[Option[V], Unit]]. Left would mean predicate passed, Right would mean retry. I feel this bloats the predicate function though. Might be better to keep them separate if there's no simple way to combine.