Using a retry with back off for auth refreshing. #504
Conversation
googlebot
added
the
cla: yes
|
Sounds like awesome! |
| } | ||
|
|
||
|
|
||
| protected HeaderCacheElement refreshCredentialsWithRetry(BackOff backoff) { |
There was a problem hiding this comment.
private HeaderCacheElement refreshCredentialsWithRetry(@nullable BackOff backoff) {
Can you document this?
|
I have no problem retrying getting credentials on IOException. If this is intended to fix issue: #475, we need to make sure retrying will actually help. |
| protected boolean sleepOrTerminate(BackOff backoff) { | ||
| long nextBackOffMillis = 0; | ||
| try { | ||
| nextBackOffMillis = backoff.nextBackOffMillis(); |
There was a problem hiding this comment.
Maybe we can clean this up a bit:
long nextBackOffMillis = backoff.nextBackOffMillis();
if (nextBackOffMillis == BackOff.STOP) {
return true;
}
try { ... }
There was a problem hiding this comment.
nextBackOffMillis() throws the exception. It's needs to be within the try.
| @@ -60,7 +48,7 @@ public void sleep(long ms) throws InterruptedException { | |||
| @VisibleForTesting | |||
| BackOff currentBackoff; | |||
| @VisibleForTesting | |||
| Sleeper sleeper = THREAD_SLEEPER; | |||
| Sleeper sleeper = Sleeper.THREAD_SLEEPER; | |||
There was a problem hiding this comment.
I missed adding it earlier. sorry about that.
| public interface Sleeper { | ||
| void sleep(long ms) throws InterruptedException; | ||
|
|
||
| static Sleeper THREAD_SLEEPER = new Sleeper() { |
There was a problem hiding this comment.
nit:
- private constructor
- static Sleeper INSTANCE
There was a problem hiding this comment.
Since we want multiple implementations for test purposes, INSTANCE seems less informative than THREAD_SLEEPER. Maybe DEFAULT_INSTANCE.
There was a problem hiding this comment.
It is a nit. It is up to you.
|
LGTM, some nits |
sduskis
force-pushed
the
retry_auth
branch
3 times, most recently
from
538923f
to
5ae164f
Compare
The Credential retrieval has occassional issues under load. Dataflow, for example, will start a whole bunch of jobs that need to get credentials at the same time, and we've seen intermittent failures because of that issue; the retry should fix, or at least improve, those types of situations. Adding a unit test for retry logic in the auth interceptor. Updating the Test as per Kevin's comments. Adding more logging around credential refreshes and retries. Using an explicit start time for the tests in RefreshingOAuth2CredentialsInterceptorTest.
Using a retry with back off for auth refreshing.
The Credential retrieval has occassional issues under load. Dataflow, for example, will start a whole bunch of jobs that need to get credentials at the same time, and we've seen intermittent failures because of that issue; the retry should fix, or at least improve, those types of situations.