[Refactoring] Ec2 and GCS plugins build client lazily #31250
Conversation
albertzaharovits
added
review
:Core/Infra/Settings
|
Pinging @elastic/es-core-infra |
jaymode
left a comment
jaymode
left a comment
There was a problem hiding this comment.
I like the concept; left some requests for changes. Also, we should have unit tests for the lazy class
| import java.util.Objects; | ||
| import java.util.function.Consumer; | ||
|
|
||
| public class Lazy<T, E extends Exception> { |
There was a problem hiding this comment.
Please add javadocs for this class and all methods. Can we make the class final? Also, what about LazyInitializable as the name
| throw new ElasticsearchException("This is unexpected", e); | ||
| } | ||
| // the count will be decreased by {@codee AmazonEc2Reference#close} | ||
| result.incRef(); |
There was a problem hiding this comment.
we increment reference twice on creation?
There was a problem hiding this comment.
Yes, this was a mistake, good catch. Ideally this should've been catched by a test. I think there's a TODO (if not for this plugin, for S3) about this test missing. I will address in a follow-up PR.
| try { | ||
| result = super.getOrCompute(); | ||
| } catch (final Exception e) { | ||
| throw new ElasticsearchException("This is unexpected", e); |
There was a problem hiding this comment.
use throw ExceptionsHelper.convertToRuntime(e);
There was a problem hiding this comment.
good to know, thanks!
| IdleConnectionReaper.shutdown(); | ||
| } | ||
|
|
||
| private static class LazyAmazonEc2Reference extends Lazy<AmazonEc2Reference, Exception> { |
There was a problem hiding this comment.
rather than a class for this, can we add a Consumer for the value on get/compute to increment the ref count? Then lazy can be final
There was a problem hiding this comment.
done, I think it's cleaner like you proposed since you don't have to study the super implementation when extending.
| return result == null ? maybeCompute(supplier) : result; | ||
| } | ||
|
|
||
| public synchronized void clear() { |
|
Thanks for the review @jaymode . I have polished Can you please take another look? |
jaymode
left a comment
jaymode
left a comment
There was a problem hiding this comment.
I left a few naming suggestions. Otherwise LGTM
| public final class LazyInitializable<T, E extends Exception> { | ||
|
|
||
| private final CheckedSupplier<T, E> supplier; | ||
| private final Consumer<T> primer; |
There was a problem hiding this comment.
how about onGet as a name instead of primer
|
|
||
| private final CheckedSupplier<T, E> supplier; | ||
| private final Consumer<T> primer; | ||
| private final Consumer<T> finalizer; |
There was a problem hiding this comment.
how about onReset instead of finalizer? finalizer makes me think of garbage collection
|
Thank you for the swift feedback! You are a rare breed of an engineer and a naming oracle 🎩 |
3 participants
This PR is the refactoring mentioned here: #29135
TBC the clients (
AmazonEC2andStorage) were already built lazily (and cached and reused) for the afore mentioned plugins. This is the refactoring of that logic. The 'client settings' and the 'client instance' always had to go together which made the code bug prone, i.e. when the settings changed you had to make sure the client was destroyed so that it was lazily rebuilt using the new settings.For the
discovery-ec2andrepository-gcsplugins, it replaces the duality '(map) client settings' - '(map) client instance' with a lazy client supplier. TheSupplierencapsulates the settings required to build the client. The lazy supplier caches the returned value once computed. This factors out the memoize supplier behavior (e.g.elasticsearch/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java
Line 75 in 918827c
Lazyinstance.This change was not required for the
repository-azureplugin as it does not caches client instances (hence only the settings are stored) because client instances are not thread-safe. This change cannot be applied to therepository-s3plugin, until the feature branch reload-secure-store-action is merged, because this plugin explicitly requires the settings to be stored so that they can be overriden from the cluster state (elasticsearch/plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java
Line 223 in 918827c
Originally, the goal for this refactoring was much more ambitious: to have an abstract component that creates and caches clients for all plugins that have clients using secure settings. But this is currently not possible, as the plugin requirements are quite diverse, eg:
discovery-ec2andrepository-s3haveCloseable, thread-safe clients, but therepository-azureclients are not thread-safe.repository-gcsclients are thread-safe but notCloseable.discovery-ec2plugin only uses one client.Relates #29135
CC @elastic/es-security