ACCUMULO-4847 Fix Retry utility's API #399
Conversation
ctubbsii
force-pushed
the
1.7-retry-fixes-stuck-locator
branch
from
42cff94
to
a1528ef
Compare
| return new RetryFactoryBuilder(); | ||
| } | ||
|
|
||
| private static class RetryFactoryBuilder implements NeedsRetries, NeedsRetryDelay, NeedsTimeIncrement, NeedsMaxWait, NeedsLogInterval, BuilderDone, |
There was a problem hiding this comment.
Since you changed to the builder pattern why not rename to "RetryBuilder". I think its confusing to intermingle the terms.
There was a problem hiding this comment.
Technically... it's a builder for RetryFactory AND Retry.
There was a problem hiding this comment.
Seems like overkill since we only use the default settings.
There was a problem hiding this comment.
We use other settings, too, but the intent was to have a flexible class to get consistent retry implementations. This change doesn't modify that original intent... it just makes the API safer so it can't be easily used incorrectly.
There was a problem hiding this comment.
Yeah I understand why you did it that way, it just seems overly complex for internal code. I was thinking we could just modify the factory constructor to not have any parameters and static methods to change something other than the defaults.
There was a problem hiding this comment.
I was trying to move away from "defaults", because every retry scenario is different, and code in Accumulo tends to be written with the bad habit of using what's there, rather than thinking through what makes sense in the specific context.
| public void logRetry(Logger log, String message, Throwable t) { | ||
| // log the first time as debug, and then after every logInterval as a warning | ||
| long now = System.nanoTime(); | ||
| if (lastRetryLog < 0) { |
There was a problem hiding this comment.
should this check hasNeverLogged?
There was a problem hiding this comment.
Yup. Looks like I missed that.
| @Override | ||
| public NeedsRetryDelay maxRetries(long max) { | ||
| checkState(); | ||
| Preconditions.checkArgument(max >= 0, "Maximum number of retries must not be negative"); |
There was a problem hiding this comment.
could add unit test for these new checks
|
|
||
| @Override | ||
| public Retry createRetry() { | ||
| return new Retry(maxRetries, initialWait, waitIncrement, maxWait, logInterval); |
There was a problem hiding this comment.
I manually checked the args and they line up. Then I looked for a test that does this, it seems like the properArgumentsInRetry test should check that these line up.
Create a builder API for Retry and RetryFactory, so that the parameters (which all have the same type) cannot be easily confused for one another and cause "sleep-until-heat-death-of-the-universe" bugs. Also fix a minor bug which is caused by the assumption that nanoTime is always positive. It is normal for nanoTime to be negative, only the differences between two times is guaranteed to be positive.
ctubbsii
force-pushed
the
1.7-retry-fixes-stuck-locator
branch
from
734ceaf
to
db04591
Compare
Create a builder API for Retry and RetryFactory, so that the parameters
(which all have the same type) cannot be easily confused for one another
and cause "sleep-until-heat-death-of-the-universe" bugs.
Also fix a minor bug which is caused by the assumption that nanoTime is
always positive. It is normal for nanoTime to be negative, only the
differences between two times is guaranteed to be positive.