-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Use common retry logic for GCS #138553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Use common retry logic for GCS #138553
Changes from 12 commits
1ebbae2
915be8a
709fdf1
f0fca99
a078ff8
39f8db6
f5502d5
1f4b905
b70d52f
5c03349
4981c7e
fabe579
d137a49
55c98eb
732121f
9da242f
63d6018
f6c2497
9d0e15b
36a5821
4636d9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ | |
|
|
||
| import static org.elasticsearch.common.io.Streams.readFully; | ||
| import static org.elasticsearch.repositories.blobstore.BlobStoreTestUtil.randomPurpose; | ||
| import static org.elasticsearch.repositories.blobstore.BlobStoreTestUtil.randomRetryingPurpose; | ||
| import static org.hamcrest.Matchers.blankOrNullString; | ||
| import static org.hamcrest.Matchers.containsString; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
|
|
@@ -115,7 +116,7 @@ public void testResumeAfterUpdate() { | |
| executeOnBlobStore(repo, container -> { | ||
| container.writeBlob(randomPurpose(), blobKey, new BytesArray(initialValue), true); | ||
|
|
||
| try (InputStream inputStream = container.readBlob(randomPurpose(), blobKey)) { | ||
| try (InputStream inputStream = container.readBlob(randomRetryingPurpose(), blobKey)) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have to be careful where we use |
||
| // Trigger the first request for the blob, partially read it | ||
| int read = inputStream.read(); | ||
| assert read != -1; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,6 +120,17 @@ public class GoogleCloudStorageClientSettings { | |
| () -> PROXY_HOST_SETTING | ||
| ); | ||
|
|
||
| /** | ||
| * The maximum number of retries to use when a GCS request fails. | ||
| * <p> | ||
| * Default to 5 to match {@link com.google.cloud.ServiceOptions#getDefaultRetrySettings()} | ||
| */ | ||
| static final Setting.AffixSetting<Integer> MAX_RETRIES_SETTING = Setting.affixKeySetting( | ||
| PREFIX, | ||
| "max_retries", | ||
| (key) -> Setting.intSetting(key, 5, 0, Setting.Property.NodeScope) | ||
| ); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We never configured number of retries for GCS previously. The default settings were for 6 attempts (aka 5 retries) |
||
|
|
||
| /** The credentials used by the client to connect to the Storage endpoint. */ | ||
| private final ServiceAccountCredentials credential; | ||
|
|
||
|
|
@@ -144,6 +155,8 @@ public class GoogleCloudStorageClientSettings { | |
| @Nullable | ||
| private final Proxy proxy; | ||
|
|
||
| private final int maxRetries; | ||
|
|
||
| GoogleCloudStorageClientSettings( | ||
| final ServiceAccountCredentials credential, | ||
| final String endpoint, | ||
|
|
@@ -152,7 +165,8 @@ public class GoogleCloudStorageClientSettings { | |
| final TimeValue readTimeout, | ||
| final String applicationName, | ||
| final URI tokenUri, | ||
| final Proxy proxy | ||
| final Proxy proxy, | ||
| final int maxRetries | ||
| ) { | ||
| this.credential = credential; | ||
| this.endpoint = endpoint; | ||
|
|
@@ -162,6 +176,7 @@ public class GoogleCloudStorageClientSettings { | |
| this.applicationName = applicationName; | ||
| this.tokenUri = tokenUri; | ||
| this.proxy = proxy; | ||
| this.maxRetries = maxRetries; | ||
| } | ||
|
|
||
| public ServiceAccountCredentials getCredential() { | ||
|
|
@@ -197,6 +212,10 @@ public Proxy getProxy() { | |
| return proxy; | ||
| } | ||
|
|
||
| public int getMaxRetries() { | ||
| return maxRetries; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
|
|
@@ -249,7 +268,8 @@ static GoogleCloudStorageClientSettings getClientSettings(final Settings setting | |
| getConfigValue(settings, clientName, READ_TIMEOUT_SETTING), | ||
| getConfigValue(settings, clientName, APPLICATION_NAME_SETTING), | ||
| getConfigValue(settings, clientName, TOKEN_URI_SETTING), | ||
| proxy | ||
| proxy, | ||
| getConfigValue(settings, clientName, MAX_RETRIES_SETTING) | ||
| ); | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test originally configured retries to be time-based (i.e. no limit on the attempts, just keep retrying for some amount of time). I changed it to just make the retry intervals small and depend on the configured retry limits because we don't support time-based retries anymore.