Make EC2 Discovery Plugin Retry Requests #50550

original-brownbear · 2020-01-02T13:25:15Z

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

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

elasticmachine · 2020-01-02T13:25:18Z

Pinging @elastic/es-distributed (:Distributed/Discovery-Plugins)

original-brownbear · 2020-01-02T13:32:03Z

plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/EC2RetriesTests.java

+import static org.hamcrest.Matchers.is;
+
+@SuppressForbidden(reason = "use a http server")
+public class EC2RetriesTests extends ESTestCase {


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.

original-brownbear · 2020-01-02T13:33:17Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

@@ -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(
-            RetryPolicy.RetryCondition.NO_RETRY_CONDITION,
+            null,


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

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.

DaveCTurner · 2020-01-02T13:35:54Z

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

@@ -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(


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?

Right, that's much nicer. Moved to the default and kept retries at 10 for now in d015576 :)

original-brownbear · 2020-01-02T13:54:54Z

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.

Wanna do that here or in a follow-up?

plugins/discovery-ec2/src/main/java/org/elasticsearch/discovery/ec2/AwsEc2ServiceImpl.java

plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/EC2RetriesTests.java

DaveCTurner · 2020-01-02T14:14:53Z

plugins/discovery-ec2/src/test/java/org/elasticsearch/discovery/ec2/EC2RetriesTests.java

+                    if (auth == null || auth.contains(accessKey) == false) {
+                        throw new IllegalArgumentException("wrong access key: " + auth);
+                    }
+                    if (failedRequests.add(exchange.getRequestHeaders().getFirst("Amz-sdk-invocation-id"))) {


Could we also check that we retry repeatedly? The first five retries take less than a second.

DaveCTurner · 2020-01-02T14:18:38Z

Wanna do that here or in a follow-up?

Yes a follow-up makes more sense.

original-brownbear · 2020-01-02T14:45:37Z

@DaveCTurner thanks, all points addressed I think :)

DaveCTurner

LGTM

original-brownbear · 2020-01-02T15:29:57Z

Thanks David!

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

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

Move EC2 discovery tests to using the mock REST API introduced in elastic#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.

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.

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.

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.

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.

Move EC2 discovery tests to using the mock REST API introduced in elastic#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.

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.

Make EC2 Discovery Plugin Retry Requests

8945922

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

original-brownbear added >non-issue :Distributed Coordination/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure v8.0.0 v7.6.0 labels Jan 2, 2020

original-brownbear added 3 commits January 2, 2020 14:26

nicer

7a5b21f

nicer

9c8c589

nicer

2d835a8

original-brownbear commented Jan 2, 2020

View reviewed changes

original-brownbear requested a review from DaveCTurner January 2, 2020 13:33

DaveCTurner requested changes Jan 2, 2020

View reviewed changes

default retry backoff

d015576

original-brownbear requested a review from DaveCTurner January 2, 2020 13:54

DaveCTurner reviewed Jan 2, 2020

View reviewed changes

original-brownbear added 3 commits January 2, 2020 15:23

Merge remote-tracking branch 'elastic/master' into 50462

ba20261

Merge remote-tracking branch 'elastic/master' into 50462

fb28d07

CR: comments

ec56d95

original-brownbear requested a review from DaveCTurner January 2, 2020 14:45

DaveCTurner approved these changes Jan 2, 2020

View reviewed changes

DaveCTurner added >bug and removed >non-issue labels Jan 2, 2020

original-brownbear merged commit 789f790 into elastic:master Jan 2, 2020

original-brownbear deleted the 50462 branch January 2, 2020 15:30

original-brownbear mentioned this pull request Jan 2, 2020

Make EC2 Discovery Plugin Retry Requests (#50550) #50558

Merged

original-brownbear mentioned this pull request Jan 3, 2020

Move EC2 Discovery Tests to Mock Rest API #50605

Merged

original-brownbear mentioned this pull request Jan 3, 2020

Make EC2 Discovery Cache Empty Seed Hosts List #50607

Merged

original-brownbear mentioned this pull request Jan 3, 2020

Make EC2 Discovery Cache Empty Seed Hosts List (#50607) #50626

Merged

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

original-brownbear mentioned this pull request Feb 12, 2020

Move EC2 Discovery Tests to Mock Rest API (#50605) #52270

Merged

original-brownbear restored the 50462 branch August 6, 2020 18:23

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make EC2 Discovery Plugin Retry Requests #50550

Make EC2 Discovery Plugin Retry Requests #50550

original-brownbear commented Jan 2, 2020

elasticmachine commented Jan 2, 2020

original-brownbear Jan 2, 2020

original-brownbear Jan 2, 2020

DaveCTurner left a comment

DaveCTurner Jan 2, 2020

original-brownbear Jan 2, 2020

original-brownbear commented Jan 2, 2020

DaveCTurner Jan 2, 2020

original-brownbear Jan 2, 2020

DaveCTurner commented Jan 2, 2020

original-brownbear commented Jan 2, 2020

DaveCTurner left a comment

original-brownbear commented Jan 2, 2020

Make EC2 Discovery Plugin Retry Requests #50550

Make EC2 Discovery Plugin Retry Requests #50550

Conversation

original-brownbear commented Jan 2, 2020

elasticmachine commented Jan 2, 2020

original-brownbear Jan 2, 2020

Choose a reason for hiding this comment

original-brownbear Jan 2, 2020

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner Jan 2, 2020

Choose a reason for hiding this comment

original-brownbear Jan 2, 2020

Choose a reason for hiding this comment

original-brownbear commented Jan 2, 2020

DaveCTurner Jan 2, 2020

Choose a reason for hiding this comment

original-brownbear Jan 2, 2020

Choose a reason for hiding this comment

DaveCTurner commented Jan 2, 2020

original-brownbear commented Jan 2, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

original-brownbear commented Jan 2, 2020