fix: re-use existing connection info during force refresh #1441
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hessjcg
reviewed
Aug 25, 2023
| instance.getSslData(); | ||
| assertThat(refreshCount.get()).isEqualTo(2); | ||
| // refresh count hasn't changed because we re-use the existing connection info | ||
| assertThat(refreshCount.get()).isEqualTo(1); |
There was a problem hiding this comment.
We need to add on to this test that forceRefresh() still works. That eventually assertThat(refreshCount.get()).isEqualTo(2); is true.
When the connector initiates a force refresh, it would previously discard existing connection info and block on the refresh operation completing. In case the cause of the connection failure is transient, we keep the current connection info around and refresh in the background. When the current connection info is in fact invalid, this will cause callers to see additional failures until the refresh completes (usually < 1s).
enocom
force-pushed
the
better-refresh
branch
from
d1e6430
to
350d227
Compare
enocom
commented
Sep 6, 2023
| @@ -52,91 +51,7 @@ public void initialize(HttpRequest var1) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| @Test(timeout = 20000) | |||
| public void testThatForceRefreshBalksWhenARefreshIsInProgress() throws Exception { | |||
There was a problem hiding this comment.
I think this test was built around the old refresh behavior -- where current would block on the next refresh. I need to double check that we have good coverage for balking before I ask for another review.
hessjcg
reviewed
Sep 8, 2023
| Thread.sleep(100); | ||
| } | ||
|
|
||
| fail(String.format("refresh count should be 2, got = %d", refreshCount.get())); |
hessjcg
approved these changes
Sep 8, 2023
This was referenced Sep 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the connector initiates a force refresh, it would previously discard existing connection info and block on the refresh operation completing. In case the cause of the connection failure is transient, we keep the current connection info around and refresh in the background. When the current connection info is in fact invalid, this will cause callers to see additional failures until the refresh completes (usually < 1s).
Fixes #1450.