This is an automated email from the ASF dual-hosted git repository. mgaido pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-livy.git
The following commit(s) were added to refs/heads/master by this push: new 3a26856 [LIVY-745] Ensure that a single RSCClientFactory gets loaded. 3a26856 is described below commit 3a2685643670c1cd1530feb7a54516b01784e781 Author: Wing Yew Poon <wyp...@cloudera.com> AuthorDate: Sat Feb 15 22:42:47 2020 +0100 [LIVY-745] Ensure that a single RSCClientFactory gets loaded. ## What changes were proposed in this pull request? In LivyClientBuilder, a ServiceLoader is used to load configured LivyClientFactory providers (HttpClientFactory and RSCClientFactory). Correct behavior of RSCClientFactory implicitly depends on there being a single instance of it in the Livy server. Instead of instantiating a new ServiceLoader every time the build method is called on a LivyClientBuilder, keep a single ServiceLoader in a static field. ServiceLoader instances are not safe for use by multiple concurrent threads, so have the ServiceLoader load the LivyClientFactory implementations into a static List. Then LivyClientBuilder#build will use the single instance of either HttpClientFactory or RSCClientFactory in this List to create a LivyClient. ## How was this patch tested? Tested by having 20 clients connect to the Livy Thrift server simultaneously. Before the change, the behavior described in https://issues.apache.org/jira/browse/LIVY-745 was encountered. With the change, the problem is not encountered. Also added a unit test that fails before this change to LivyClientBuilder, and passes with the change. Author: Wing Yew Poon <wyp...@cloudera.com> Closes #275 from wypoon/LIVY-745. --- .../java/org/apache/livy/LivyClientBuilder.java | 26 ++++++++++++++++------ .../java/org/apache/livy/TestClientFactory.java | 12 +++++++++- .../org/apache/livy/TestLivyClientBuilder.java | 8 +++++++ 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/api/src/main/java/org/apache/livy/LivyClientBuilder.java b/api/src/main/java/org/apache/livy/LivyClientBuilder.java index 5bbd170..1d7ec01 100644 --- a/api/src/main/java/org/apache/livy/LivyClientBuilder.java +++ b/api/src/main/java/org/apache/livy/LivyClientBuilder.java @@ -23,6 +23,8 @@ import java.io.Reader; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; +import java.util.ArrayList; +import java.util.List; import java.util.Map; import java.util.Properties; import java.util.ServiceLoader; @@ -35,6 +37,19 @@ public final class LivyClientBuilder { public static final String LIVY_URI_KEY = "livy.uri"; + private static final ServiceLoader<LivyClientFactory> CLIENT_FACTORY_LOADER = + ServiceLoader.load(LivyClientFactory.class, classLoader()); + + private static List<LivyClientFactory> getLivyClientFactories() { + List<LivyClientFactory> factories = new ArrayList<>(); + for (LivyClientFactory f : CLIENT_FACTORY_LOADER) { + factories.add(f); + } + return factories; + } + + private static final List<LivyClientFactory> CLIENT_FACTORIES = getLivyClientFactories(); + private final Properties config; /** @@ -118,14 +133,11 @@ public final class LivyClientBuilder { } LivyClient client = null; - ServiceLoader<LivyClientFactory> loader = ServiceLoader.load(LivyClientFactory.class, - classLoader()); - if (!loader.iterator().hasNext()) { + if (CLIENT_FACTORIES.isEmpty()) { throw new IllegalStateException("No LivyClientFactory implementation was found."); } - Exception error = null; - for (LivyClientFactory factory : loader) { + for (LivyClientFactory factory : CLIENT_FACTORIES) { try { client = factory.createClient(uri, config); } catch (Exception e) { @@ -158,10 +170,10 @@ public final class LivyClientBuilder { return client; } - private ClassLoader classLoader() { + private static ClassLoader classLoader() { ClassLoader cl = Thread.currentThread().getContextClassLoader(); if (cl == null) { - cl = getClass().getClassLoader(); + cl = LivyClientBuilder.class.getClassLoader(); } return cl; } diff --git a/api/src/test/java/org/apache/livy/TestClientFactory.java b/api/src/test/java/org/apache/livy/TestClientFactory.java index 89edeec..622908c 100644 --- a/api/src/test/java/org/apache/livy/TestClientFactory.java +++ b/api/src/test/java/org/apache/livy/TestClientFactory.java @@ -21,9 +21,19 @@ import java.io.File; import java.net.URI; import java.util.Properties; import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicLong; public class TestClientFactory implements LivyClientFactory { + private static AtomicLong instanceCount = new AtomicLong(); + public static long getInstanceCount() { + return instanceCount.get(); + } + + public TestClientFactory() { + instanceCount.incrementAndGet(); + } + @Override public LivyClient createClient(URI uri, Properties config) { switch (uri.getPath()) { @@ -81,6 +91,6 @@ public class TestClientFactory implements LivyClientFactory { throw new UnsupportedOperationException(); } -} + } } diff --git a/api/src/test/java/org/apache/livy/TestLivyClientBuilder.java b/api/src/test/java/org/apache/livy/TestLivyClientBuilder.java index 568d633..213dc03 100644 --- a/api/src/test/java/org/apache/livy/TestLivyClientBuilder.java +++ b/api/src/test/java/org/apache/livy/TestLivyClientBuilder.java @@ -78,4 +78,12 @@ public class TestLivyClientBuilder { } } + @Test + public void testSinglenessOfClientFactory() throws Exception { + LivyClient client1 = new LivyClientBuilder().build(); + assertTrue(client1 instanceof TestClientFactory.Client); + LivyClient client2 = new LivyClientBuilder().build(); + assertTrue(client2 instanceof TestClientFactory.Client); + assertEquals(1L, TestClientFactory.getInstanceCount()); + } }