Make EC2 Discovery Plugin Retry Requests#50550
Make EC2 Discovery Plugin Retry Requests#50550original-brownbear merged 8 commits intoelastic:masterfrom original-brownbear:50462
Conversation
Use the default retry condition instead of never retrying in the discovery plugin causing hot retries upstream and add a test that verifies retrying works. Closes #50462
|
Pinging @elastic/es-distributed (:Distributed/Discovery-Plugins) |
| import static org.hamcrest.Matchers.is; | ||
|
|
||
| @SuppressForbidden(reason = "use a http server") | ||
| public class EC2RetriesTests extends ESTestCase { |
There was a problem hiding this comment.
Admittedly quite a bit of code, but I'm hoping we can just extend the REST layer mocking here and dry things up against the fixture step-by-step like we did for S3 and the other cloud providers in the repository tests to get rid of all our complicated mocking of actual AWS SDK classes and just mock on the REST layer.
| final Random rand = Randomness.get(); | ||
| final RetryPolicy retryPolicy = new RetryPolicy( | ||
| RetryPolicy.RetryCondition.NO_RETRY_CONDITION, | ||
| null, |
There was a problem hiding this comment.
Just using the default retry condition here now. We may want to make the retry interval configurable, if only internally for tests. The current settings make the first retry happen after almost 20s but I didn't want to add that complication here yet.
DaveCTurner
left a comment
There was a problem hiding this comment.
I left a question.
I also think we should adjust the behaviour of AwsEc2SeedHostsProvider to respect discovery.ec2.node_cache_time even if the last attempt resulted in an exception and/or an empty list.
| @@ -79,7 +79,7 @@ static ClientConfiguration buildConfiguration(Logger logger, Ec2ClientSettings c | |||
| // Increase the number of retries in case of 5xx API responses | |||
| final Random rand = Randomness.get(); | |||
| final RetryPolicy retryPolicy = new RetryPolicy( | |||
There was a problem hiding this comment.
Can we avoid setting our own retry policy at all and use the SDK default? The SDK makes a more nuanced decision about how much to back off based on the error type. If we really want 10 retries then that can be overridden with setMaxErrorRetry(10) instead?
There was a problem hiding this comment.
Right, that's much nicer. Moved to the default and kept retries at 10 for now in d015576 :)
Wanna do that here or in a follow-up? |
plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java
Outdated
Show resolved
Hide resolved
plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/EC2RetriesTests.java
Show resolved
Hide resolved
plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/EC2RetriesTests.java
Show resolved
Hide resolved
| if (auth == null || auth.contains(accessKey) == false) { | ||
| throw new IllegalArgumentException("wrong access key: " + auth); | ||
| } | ||
| if (failedRequests.add(exchange.getRequestHeaders().getFirst("Amz-sdk-invocation-id"))) { |
There was a problem hiding this comment.
Could we also check that we retry repeatedly? The first five retries take less than a second.
Yes a follow-up makes more sense. |
|
@DaveCTurner thanks, all points addressed I think :) |
|
Thanks David! |
Follow up to #50550. Cache empty nodes lists (`fetchDynamicNodes` will return an empty list in case of failure) now that the plugin properly retries requests to AWS EC2 APIs.
Use the default retry condition instead of never retrying in the discovery plugin causing hot retries upstream and add a test that verifies retrying works. Closes elastic#50462
Follow up to elastic#50550. Cache empty nodes lists (`fetchDynamicNodes` will return an empty list in case of failure) now that the plugin properly retries requests to AWS EC2 APIs.
Move EC2 discovery tests to using the mock REST API introduced in #50550 instead of mocking the AWS SDK classes manually. Move the trivial remaining AWS SDK mocks to the single test suit that was using them.
Use the default retry condition instead of never retrying in the discovery plugin causing hot retries upstream and
add a test that verifies retrying works.
Closes #50462