search

sunng87 / diehard

Clojure resilience library for flexible retry, circuit breaker and rate limiter
Eclipse Public License 2.0
330 stars 27 forks source link

0.10.1 regression: max-retries not respected #43

Closed mrichards42 closed 3 years ago

mrichards42 commented 3 years ago

With the changes in #42, retry-policy-from-config with just a policy and no overrides ends up overwriting :max-retries with -1. A minimal reproduction, based on an existing test

(defretrypolicy the-test-policy
  {:max-retries 4
   :retry-if (fn [v e] (< v 10))})

(with-retry {:policy the-test-policy}
  *executions*)
; => 10

It looks like :max-retries is the only option with a default. Removing the default would fix this issue, but I'm not sure if it's safe to remove: https://github.com/sunng87/diehard/blob/871137cc3530cd873597e536c9b581cd8a1b3924/src/diehard/core.clj#L83-L84

sunng87 commented 3 years ago

Good catch. I don't remember the reason to specify this default. As in latest failsafe, the default max retry is set to 2.

I think it's ok to remove this default value.

sunng87 commented 3 years ago

hmmm.. I was wrong. In order to maintain backward-compatibility, I should keep -1 as default value of max-retries.

sunng87 commented 3 years ago

Fixed in https://github.com/sunng87/diehard/commit/0db1e95040f2bab761701661abf091c4e97dda7d and released in 0.10.2