Update ec2 secure settings #29134
Update ec2 secure settings #29134
Conversation
albertzaharovits
added
>feature
:Distributed/Discovery-Plugins
|
Pinging @elastic/es-distributed |
| public void close() throws IOException { | ||
| if (client != null) { | ||
| client.shutdown(); | ||
| public AmazonEc2Reference client() { |
There was a problem hiding this comment.
I wonder if we can generalizer this and maybe have a utils class in core that does what we do here. Operate on refcounted and maintain a map. Then we can also have a factory method and that way we share most of the logic. Maybe a followup.
There was a problem hiding this comment.
++ This is becoming a pattern and the code is duplicated. I will do the refactoring as the last PR to this branch, to make sure I account for all distinctions of all use cases.
| import org.elasticsearch.common.unit.TimeValue; | ||
| import java.util.Locale; | ||
|
|
||
| public class Ec2ClientSettings { |
There was a problem hiding this comment.
++ make the class final? and add a short javadoc?
| for (int i=0; i<3; i++) { | ||
| provider.buildDynamicNodes(); | ||
| assertThat(provider.fetchCount, is(1)); | ||
| Thread.sleep(1_000L); // wait for cache to expire |
There was a problem hiding this comment.
while we are on it.. maybe there is a way to expire the cache manually without sleeping?
There was a problem hiding this comment.
To expire the cache manually it will require mocking the DiscoNodesCache which is part of what this test is about. I think we should not do this.
I think this is related to #28452 , which requires a more "smarter" discovery nodes cache. I will address this test in the scope of that issue.
This is the analogous of #28517 for the ec2 client settings.
The approach is the same: use a client registry that allows for the client settings to change. Users should not cache the client, instead request one on each use.
Note:
This intentionally duplicates some code from the S3 reload settings PR. The factoring of the common bits will be done as the last step, when all reload requirements have been addressed.