[Refactoring] Ec2 and GCS plugins build client lazily by albertzaharovits · Pull Request #31250 · elastic/elasticsearch

albertzaharovits · 2018-06-11T16:59:20Z

This PR is the refactoring mentioned here: #29135

TBC the clients (AmazonEC2 and Storage) 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-ec2 and repository-gcs plugins, it replaces the duality '(map) client settings' - '(map) client instance' with a lazy client supplier. The Supplier encapsulates 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

public AmazonS3Reference client(String clientName) {

) in a Lazy instance.
This change was not required for the repository-azure plugin 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 the repository-s3 plugin, 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

void overrideCredentialsFromClusterState(AwsS3Service awsService) {

), see also: 17d0155

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-ec2 and repository-s3 have Closeable, thread-safe clients, but the repository-azure clients are not thread-safe. repository-gcs clients are thread-safe but not Closeable.
the repository plugins use a dictionary of clients, but the discovery-ec2 plugin only uses one client.

Relates #29135
CC @elastic/es-security

elasticmachine · 2018-06-11T16:59:22Z

Pinging @elastic/es-core-infra

…-dict

jaymode

I like the concept; left some requests for changes. Also, we should have unit tests for the lazy class

jaymode · 2018-06-13T15:26:18Z

server/src/main/java/org/elasticsearch/common/util/Lazy.java

+import java.util.Objects;
+import java.util.function.Consumer;
+
+public class Lazy<T, E extends Exception> {


Please add javadocs for this class and all methods. Can we make the class final? Also, what about LazyInitializable as the name

jaymode · 2018-06-13T15:34:06Z

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

+                throw new ElasticsearchException("This is unexpected", e);
+            }
+            // the count will be decreased by {@codee AmazonEc2Reference#close}
+            result.incRef();


we increment reference twice on creation?

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.

jaymode · 2018-06-13T15:35:15Z

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

+            try {
+                result = super.getOrCompute();
+            } catch (final Exception e) {
+                throw new ElasticsearchException("This is unexpected", e);


use throw ExceptionsHelper.convertToRuntime(e);

good to know, thanks!

jaymode · 2018-06-13T15:38:02Z

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

        IdleConnectionReaper.shutdown();
    }

+    private static class LazyAmazonEc2Reference extends Lazy<AmazonEc2Reference, Exception> {


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

done, I think it's cleaner like you proposed since you don't have to study the super implementation when extending.

jaymode · 2018-06-13T15:38:53Z

server/src/main/java/org/elasticsearch/common/util/Lazy.java

+        return result == null ? maybeCompute(supplier) : result;
+    }
+
+    public synchronized void clear() {


maybe call this reset

albertzaharovits · 2018-06-15T18:57:44Z

Thanks for the review @jaymode . I have polished Lazy -> LazyInitializable by following all your comments.

Can you please take another look?

jaymode

I left a few naming suggestions. Otherwise LGTM

jaymode · 2018-06-15T20:58:24Z

server/src/main/java/org/elasticsearch/common/util/LazyInitializable.java

+public final class LazyInitializable<T, E extends Exception> {
+
+    private final CheckedSupplier<T, E> supplier;
+    private final Consumer<T> primer;


how about onGet as a name instead of primer

jaymode · 2018-06-15T20:58:59Z

server/src/main/java/org/elasticsearch/common/util/LazyInitializable.java

+
+    private final CheckedSupplier<T, E> supplier;
+    private final Consumer<T> primer;
+    private final Consumer<T> finalizer;


how about onReset instead of finalizer? finalizer makes me think of garbage collection

albertzaharovits · 2018-06-16T17:52:09Z

Thank you for the swift feedback! You are a rare breed of an engineer and a naming oracle 🎩

albertzaharovits added 3 commits June 11, 2018 17:46

Added Lazy

f98d75f

GCS removed double map (config + clients)

c54ffa1

EC2 plugin lazy client ditching cached settings

1184411

albertzaharovits added review :Core/Infra/Settings Settings infrastructure and APIs >refactoring labels Jun 11, 2018

albertzaharovits self-assigned this Jun 11, 2018

albertzaharovits requested a review from jaymode June 11, 2018 16:59

albertzaharovits mentioned this pull request Jun 11, 2018

Reloadable secure settings for plugins #29135

Closed

6 tasks

Merge branch 'reload-secure-store-action' into refactor-remove-double…

7f76048

…-dict

jaymode requested changes Jun 13, 2018

View reviewed changes

polished LazyInitializable

e5a9934

jaymode approved these changes Jun 15, 2018

View reviewed changes

Renames

86b3ebb

albertzaharovits merged commit 62debd1 into elastic:reload-secure-store-action Jun 16, 2018

albertzaharovits deleted the refactor-remove-double-dict branch June 16, 2018 17:56

albertzaharovits removed the review label Jul 1, 2018

Conversation

albertzaharovits commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 11, 2018

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Jun 15, 2018

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Jun 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

albertzaharovits commented Jun 11, 2018 •

edited

Loading