You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@solr.apache.org by kr...@apache.org on 2022/09/09 16:12:59 UTC

[solr] branch branch_9_0 updated (a96b3799d75 -> 1d9d6d81fa4)

This is an automated email from the ASF dual-hosted git repository.

krisden pushed a change to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git


    from a96b3799d75 SOLR-16264: Set versions for all Antora build tools (#917)
     new 1b48279e07c SOLR-16127 Ordered Executor orphans locks (#779)
     new e771ae5b4c8 SOLR-16401: Test thread leak linger should be 1s (#997)
     new d1f1f546596 SOLR-16399: ExportWriter fails with max values for fields (#995)
     new 319a569c524 SOLR-16190: Http2SolrClient should make sure to shutdown the executor (#848)
     new eff7eabc3be SOLR-16189: TestCancellableCollector thread leak (#847)
     new a066431bb59 SOLR-16187: ExecutorUtil#awaitTermination shouldn't wait forever (#840)
     new 05d1eb52c68 SOLR-16186: AuditLoggerPluginTest leaks threads
     new e4a622d0307 SOLR-16185: CloudSolrClientCacheTest leaks threads (#839)
     new d0412413e62 SOLR-16182: Close ZkClientClusterStateProvider in tests to avoid thread leak (#837)
     new 3a56ddf0918 SOLR-16174: Modernize TestBulkSchemaConcurrent (#829)
     new 3f7bd2d9564 SOLR-16091 Improve test logging (#736)
     new 1d9d6d81fa4 SOLR-16091 Wait for all nodes to register plugins in test (#747)

The 12 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 solr/CHANGES.txt                                   |   5 +-
 .../apache/solr/api/ContainerPluginsRegistry.java  |   2 -
 .../src/java/org/apache/solr/api/V2HttpCall.java   |  19 +-
 .../apache/solr/handler/export/ExportWriter.java   |   4 +-
 .../apache/solr/security/AuditLoggerPlugin.java    |  14 +-
 .../java/org/apache/solr/util/OrderedExecutor.java |   9 +-
 .../solr/filestore/TestDistribPackageStore.java    |  34 +-
 .../apache/solr/handler/TestContainerPlugin.java   | 551 +++++++++------------
 .../solr/schema/TestBulkSchemaConcurrent.java      | 289 ++++++-----
 .../solr/search/TestCancellableCollector.java      |  15 +-
 .../org/apache/solr/util/OrderedExecutorTest.java  |  51 ++
 .../solr/client/solrj/impl/CloudSolrClient.java    |   5 +-
 .../solr/client/solrj/impl/Http2SolrClient.java    |  19 +-
 .../org/apache/solr/common/util/ExecutorUtil.java  |  28 +-
 .../impl/CloudHttp2SolrClientBuilderTest.java      |  25 +-
 .../CloudHttp2SolrClientMultiConstructorTest.java  |   5 +-
 .../solrj/impl/CloudHttp2SolrClientTest.java       |  31 +-
 .../solrj/impl/CloudSolrClientBuilderTest.java     |  35 +-
 .../solrj/impl/CloudSolrClientCacheTest.java       |  21 +-
 .../impl/CloudSolrClientMultiConstructorTest.java  |  10 +-
 .../client/solrj/impl/CloudSolrClientTest.java     |  33 +-
 .../apache/solr/common/util/ExecutorUtilTest.java  |  98 ++++
 .../src/java/org/apache/solr/SolrTestCase.java     |   6 +-
 23 files changed, 735 insertions(+), 574 deletions(-)
 create mode 100644 solr/solrj/src/test/org/apache/solr/common/util/ExecutorUtilTest.java


[solr] 03/12: SOLR-16399: ExportWriter fails with max values for fields (#995)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit d1f1f5465969f5e5198b91a5153a1181237d88ce
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Tue Sep 6 10:23:23 2022 -0400

    SOLR-16399: ExportWriter fails with max values for fields (#995)
---
 solr/CHANGES.txt                                                    | 3 ++-
 solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 79ec41f3437..f4417863ba9 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -8,13 +8,14 @@ https://github.com/apache/solr/blob/main/solr/solr-ref-guide/modules/upgrade-not
 
 Bug Fixes
 ---------------------
-
 * SOLR-16191: Validate that installed ps utility supports -p flag, so that we do not inadvertantly stop the wrong process. (Mike Drob, Michael Gibney)
 
 * SOLR-16209: Rolling restart will no longer trigger as much PKI Plugin error logging. (Mike Drob, Tomás Fernández Löbbe)
 
 * SOLR-16264: Set versions for all Antora build tools (Houston Putman)
 
+* SOLR-16399: ExportWriter fails with max values for fields (Kevin Risden)
+
 ==================  9.0.0 ==================
 
 New Features
diff --git a/solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java b/solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java
index c943e1b3ee2..a6b8ac281c7 100644
--- a/solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java
+++ b/solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java
@@ -804,7 +804,9 @@ public class ExportWriter implements SolrCore.RawWriter, Closeable {
         int docId;
         while ((docId = it.nextDoc()) != DocIdSetIterator.NO_MORE_DOCS) {
           this.sortDoc.setValues(docId);
-          if (top.lessThan(this.sortDoc)) {
+          // Always set the top doc if previously not set, otherwise
+          // set the top if the sortDoc is greater than current
+          if (top.lessThan(this.sortDoc) || top.docId == -1) {
             top.setValues(this.sortDoc);
             top = queue.updateTop();
           }


[solr] 07/12: SOLR-16186: AuditLoggerPluginTest leaks threads

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit 05d1eb52c689b9207e3a8642f56f67208b8cc10a
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Mon May 9 21:59:52 2022 -0400

    SOLR-16186: AuditLoggerPluginTest leaks threads
---
 .../java/org/apache/solr/security/AuditLoggerPlugin.java   | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/solr/core/src/java/org/apache/solr/security/AuditLoggerPlugin.java b/solr/core/src/java/org/apache/solr/security/AuditLoggerPlugin.java
index 07a8dd73740..62d6d952220 100644
--- a/solr/core/src/java/org/apache/solr/security/AuditLoggerPlugin.java
+++ b/solr/core/src/java/org/apache/solr/security/AuditLoggerPlugin.java
@@ -42,9 +42,11 @@ import java.util.concurrent.atomic.AtomicInteger;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.util.ExecutorUtil;
 import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.apache.solr.common.util.TimeSource;
 import org.apache.solr.core.SolrInfoBean;
 import org.apache.solr.metrics.SolrMetricsContext;
 import org.apache.solr.security.AuditEvent.EventType;
+import org.apache.solr.util.TimeOut;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -348,7 +350,8 @@ public abstract class AuditLoggerPlugin implements Closeable, Runnable, SolrInfo
       waitForQueueToDrain(30);
       closed = true;
       log.info("Shutting down async Auditlogger background thread(s)");
-      executorService.shutdownNow();
+      ExecutorUtil.shutdownNowAndAwaitTermination(executorService);
+      executorService = null;
       try {
         SolrInfoBean.super.close();
       } catch (Exception e) {
@@ -364,8 +367,8 @@ public abstract class AuditLoggerPlugin implements Closeable, Runnable, SolrInfo
    */
   protected void waitForQueueToDrain(int timeoutSeconds) {
     if (async && executorService != null) {
-      int timeSlept = 0;
-      while ((!queue.isEmpty() || auditsInFlight.get() > 0) && timeSlept < timeoutSeconds) {
+      TimeOut timeOut = new TimeOut(timeoutSeconds, TimeUnit.SECONDS, TimeSource.NANO_TIME);
+      while ((!queue.isEmpty() || auditsInFlight.get() > 0) && !timeOut.hasTimedOut()) {
         try {
           if (log.isInfoEnabled()) {
             log.info(
@@ -373,9 +376,10 @@ public abstract class AuditLoggerPlugin implements Closeable, Runnable, SolrInfo
                 queue.size(),
                 auditsInFlight.get());
           }
-          Thread.sleep(1000);
-          timeSlept++;
+          timeOut.sleep(1000);
         } catch (InterruptedException ignored) {
+          // Preserve interrupt status
+          Thread.currentThread().interrupt();
         }
       }
     }


[solr] 05/12: SOLR-16189: TestCancellableCollector thread leak (#847)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit eff7eabc3beb97ca0e5bc56c4931b08d9eb4fa30
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Mon May 9 22:01:07 2022 -0400

    SOLR-16189: TestCancellableCollector thread leak (#847)
---
 .../org/apache/solr/search/TestCancellableCollector.java  | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/solr/core/src/test/org/apache/solr/search/TestCancellableCollector.java b/solr/core/src/test/org/apache/solr/search/TestCancellableCollector.java
index d31ca82a07c..7e76bacc2ec 100644
--- a/solr/core/src/test/org/apache/solr/search/TestCancellableCollector.java
+++ b/solr/core/src/test/org/apache/solr/search/TestCancellableCollector.java
@@ -39,9 +39,9 @@ import org.apache.lucene.search.TopDocs;
 import org.apache.lucene.search.TopScoreDocCollector;
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.util.BytesRef;
-import org.apache.lucene.util.NamedThreadFactory;
 import org.apache.solr.SolrTestCase;
 import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
 
 public class TestCancellableCollector extends SolrTestCase {
   Directory dir;
@@ -78,8 +78,8 @@ public class TestCancellableCollector extends SolrTestCase {
             4,
             0L,
             TimeUnit.MILLISECONDS,
-            new LinkedBlockingQueue<Runnable>(),
-            new NamedThreadFactory("TestIndexSearcher"));
+            new LinkedBlockingQueue<>(),
+            new SolrNamedThreadFactory(this.getClass().getSimpleName()));
   }
 
   @Override
@@ -89,10 +89,9 @@ public class TestCancellableCollector extends SolrTestCase {
     dir.close();
 
     if (executor != null) {
-      executor.shutdown();
+      ExecutorUtil.shutdownAndAwaitTermination(executor);
+      executor = null;
     }
-
-    executor = null;
   }
 
   private CancellableCollector buildCancellableCollector(
@@ -121,7 +120,6 @@ public class TestCancellableCollector extends SolrTestCase {
   private void cancelQuery(CancellableCollector cancellableCollector, final int sleepTime) {
     executor.submit(
         () -> {
-
           // Wait for some time to let the query start
           try {
             if (sleepTime > 0) {
@@ -130,6 +128,7 @@ public class TestCancellableCollector extends SolrTestCase {
 
             cancellableCollector.cancel();
           } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
             throw new RuntimeException(e.getMessage());
           }
         });
@@ -200,6 +199,7 @@ public class TestCancellableCollector extends SolrTestCase {
         try {
           Thread.sleep(50);
         } catch (InterruptedException e) {
+          Thread.currentThread().interrupt();
           throw new RuntimeException(e.getMessage());
         }
       }
@@ -212,6 +212,7 @@ public class TestCancellableCollector extends SolrTestCase {
             try {
               Thread.sleep(30);
             } catch (InterruptedException e) {
+              Thread.currentThread().interrupt();
               throw new RuntimeException(e.getMessage());
             }
           }


[solr] 12/12: SOLR-16091 Wait for all nodes to register plugins in test (#747)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit 1d9d6d81fa4962d80162baf13f8f90cad7a3ffc5
Author: Mike Drob <md...@apache.org>
AuthorDate: Fri Mar 18 09:19:13 2022 -0500

    SOLR-16091 Wait for all nodes to register plugins in test (#747)
    
    (cherry picked from commit c10fac98cb1207786cfc5f651456207edbf25c00)
---
 .../org/apache/solr/api/ContainerPluginsRegistry.java |  2 --
 .../core/src/java/org/apache/solr/api/V2HttpCall.java | 19 ++++++++++---------
 .../org/apache/solr/handler/TestContainerPlugin.java  | 18 +++++++-----------
 3 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java b/solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java
index 74affd64c8f..f8d2e64bf72 100644
--- a/solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java
+++ b/solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java
@@ -87,8 +87,6 @@ public class ContainerPluginsRegistry implements ClusterPropertiesListener, MapW
     refresh();
     Phaser localPhaser = phaser; // volatile read
     if (localPhaser != null) {
-      assert localPhaser.getRegisteredParties() == 1;
-      // we should be the only ones registered, so this will advance phase each time
       localPhaser.arrive();
     }
     return false;
diff --git a/solr/core/src/java/org/apache/solr/api/V2HttpCall.java b/solr/core/src/java/org/apache/solr/api/V2HttpCall.java
index 4680de629de..9ca12a8fc84 100644
--- a/solr/core/src/java/org/apache/solr/api/V2HttpCall.java
+++ b/solr/core/src/java/org/apache/solr/api/V2HttpCall.java
@@ -101,8 +101,8 @@ public class V2HttpCall extends HttpSolrCall {
       }
 
       boolean isCompositeApi = false;
+      api = getApiInfo(cores.getRequestHandlers(), path, req.getMethod(), fullPath, parts);
       if (knownPrefixes.contains(prefix)) {
-        api = getApiInfo(cores.getRequestHandlers(), path, req.getMethod(), fullPath, parts);
         if (api != null) {
           isCompositeApi = api instanceof CompositeApi;
           if (!isCompositeApi) {
@@ -110,6 +110,14 @@ public class V2HttpCall extends HttpSolrCall {
             return;
           }
         }
+      } else { // custom plugin
+        if (api != null) {
+          initAdminRequest(path);
+          return;
+        }
+        assert core == null;
+        throw new SolrException(
+            SolrException.ErrorCode.NOT_FOUND, "Could not load plugin at " + path);
       }
 
       if ("c".equals(prefix) || "collections".equals(prefix)) {
@@ -140,13 +148,6 @@ public class V2HttpCall extends HttpSolrCall {
       } else if ("cores".equals(prefix)) {
         origCorename = pathSegments.get(1);
         core = cores.getCore(origCorename);
-      } else {
-        api = getApiInfo(cores.getRequestHandlers(), path, req.getMethod(), fullPath, parts);
-        if (api != null) {
-          // custom plugin
-          initAdminRequest(path);
-          return;
-        }
       }
       if (core == null) {
         log.error(">> path: '{}'", path);
@@ -156,7 +157,7 @@ public class V2HttpCall extends HttpSolrCall {
         } else {
           throw new SolrException(
               SolrException.ErrorCode.NOT_FOUND,
-              "no core retrieved for core name:  " + origCorename + ". Path : " + path);
+              "no core retrieved for core name: " + origCorename + ". Path: " + path);
         }
       } else {
         Thread.currentThread().setContextClassLoader(core.getResourceLoader().getClassLoader());
diff --git a/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java b/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
index 0769613ef18..1c7b07b2f0d 100644
--- a/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
+++ b/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
@@ -47,7 +47,6 @@ import org.apache.solr.client.solrj.request.beans.Package;
 import org.apache.solr.client.solrj.request.beans.PluginMeta;
 import org.apache.solr.client.solrj.response.V2Response;
 import org.apache.solr.cloud.ClusterSingleton;
-import org.apache.solr.cloud.MiniSolrCloudCluster;
 import org.apache.solr.cloud.SolrCloudTestCase;
 import org.apache.solr.common.annotation.JsonProperty;
 import org.apache.solr.common.util.ReflectMapWriter;
@@ -78,7 +77,9 @@ public class TestContainerPlugin extends SolrCloudTestCase {
 
     int nodes = TEST_NIGHTLY ? 4 : 2;
     cluster = configureCluster(nodes).withJettyConfig(jetty -> jetty.enableV2(true)).configure();
-    cluster.getOpenOverseer().getCoreContainer().getContainerPluginsRegistry().setPhaser(phaser);
+    for (JettySolrRunner jetty : cluster.getJettySolrRunners()) {
+      jetty.getCoreContainer().getContainerPluginsRegistry().setPhaser(phaser);
+    }
   }
 
   @After
@@ -160,9 +161,8 @@ public class TestContainerPlugin extends SolrCloudTestCase {
       assertEquals(404, e.code());
       assertThat(
           e.getMetaData().findRecursive("error", "msg").toString(),
-          containsString("no core retrieved"));
-      // V2HttpCall will separately log the path and stack trace, probably could be fixed
-      assertEquals(2, errors.getCount());
+          containsString("Could not load plugin at"));
+      assertEquals(1, errors.getCount());
     }
 
     // test ClusterSingleton plugin
@@ -254,7 +254,6 @@ public class TestContainerPlugin extends SolrCloudTestCase {
     addPkgVersionReq.process(cluster.getSolrClient());
 
     waitForAllNodesToSync(
-        cluster,
         "/cluster/package",
         Map.of(
             ":result:packages:mypkg[0]:version",
@@ -450,14 +449,11 @@ public class TestContainerPlugin extends SolrCloudTestCase {
         .build();
   }
 
-  public static void waitForAllNodesToSync(
-      MiniSolrCloudCluster cluster, String path, Map<String, Object> expected) throws Exception {
+  public void waitForAllNodesToSync(String path, Map<String, Object> expected) throws Exception {
     for (JettySolrRunner jettySolrRunner : cluster.getJettySolrRunners()) {
       String baseUrl = jettySolrRunner.getBaseUrl().toString().replace("/solr", "/api");
       String url = baseUrl + path + "?wt=javabin";
-      // Allow multiple retries here because we need multiple nodes to update
-      // and our single phaser only ensures that one of them has reached expected state
-      TestDistribPackageStore.assertResponseValues(10, new Fetcher(url, jettySolrRunner), expected);
+      TestDistribPackageStore.assertResponseValues(1, new Fetcher(url, jettySolrRunner), expected);
     }
   }
 


[solr] 10/12: SOLR-16174: Modernize TestBulkSchemaConcurrent (#829)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit 3a56ddf0918d82da179e374ba6498cbff4778a52
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Mon May 2 15:05:59 2022 -0400

    SOLR-16174: Modernize TestBulkSchemaConcurrent (#829)
---
 .../solr/schema/TestBulkSchemaConcurrent.java      | 289 +++++++++++----------
 1 file changed, 157 insertions(+), 132 deletions(-)

diff --git a/solr/core/src/test/org/apache/solr/schema/TestBulkSchemaConcurrent.java b/solr/core/src/test/org/apache/solr/schema/TestBulkSchemaConcurrent.java
index 7aaed143e3e..e2d5b5a4257 100644
--- a/solr/core/src/test/org/apache/solr/schema/TestBulkSchemaConcurrent.java
+++ b/solr/core/src/test/org/apache/solr/schema/TestBulkSchemaConcurrent.java
@@ -27,12 +27,20 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.TimeUnit;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.cloud.AbstractFullDistribZkTestBase;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
 import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.TimeSource;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.util.RestTestHarness;
+import org.apache.solr.util.TimeOut;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.slf4j.Logger;
@@ -41,6 +49,9 @@ import org.slf4j.LoggerFactory;
 public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
+  private static final long TIMEOUT = TEST_NIGHTLY ? 10 : 1; // in seconds
+  private static final int THREAD_COUNT = TEST_NIGHTLY ? 5 : 2;
+
   @BeforeClass
   public static void initSysProperties() {
     System.setProperty("managed.schema.mutable", "true");
@@ -51,43 +62,56 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
     return "solrconfig-managed-schema.xml";
   }
 
+  @Before
+  public void setupTest() {
+    setupRestTestHarnesses();
+  }
+
+  @After
+  public void teardownTest() throws Exception {
+    closeRestTestHarnesses();
+  }
+
   @Test
-  @SuppressWarnings({"unchecked"})
   public void test() throws Exception {
+    final List<List<String>> collectErrors = Collections.synchronizedList(new ArrayList<>());
 
-    final int threadCount = 5;
-    setupRestTestHarnesses();
-    Thread[] threads = new Thread[threadCount];
-    @SuppressWarnings({"rawtypes"})
-    final List<List> collectErrors = Collections.synchronizedList(new ArrayList<>());
+    final ExecutorService executorService =
+        ExecutorUtil.newMDCAwareFixedThreadPool(
+            THREAD_COUNT, new SolrNamedThreadFactory(this.getClass().getSimpleName()));
 
-    for (int i = 0; i < threadCount; i++) {
+    List<Callable<Void>> callees = new ArrayList<>(THREAD_COUNT);
+    for (int i = 0; i < THREAD_COUNT; i++) {
       final int finalI = i;
-      threads[i] =
-          new Thread() {
-            @Override
-            public void run() {
-              @SuppressWarnings({"rawtypes"})
-              ArrayList errs = new ArrayList();
-              collectErrors.add(errs);
-              try {
-                invokeBulkAddCall(finalI, errs);
-                invokeBulkReplaceCall(finalI, errs);
-                invokeBulkDeleteCall(finalI, errs);
-              } catch (Exception e) {
-                e.printStackTrace();
-              }
+      Callable<Void> call =
+          () -> {
+            List<String> errs = new ArrayList<>();
+            collectErrors.add(errs);
+            try {
+              invokeBulkAddCall(finalI, errs);
+              invokeBulkReplaceCall(finalI, errs);
+              invokeBulkDeleteCall(finalI, errs);
+            } catch (InterruptedException interruptedException) {
+              Thread.currentThread().interrupt();
+            } catch (Exception e) {
+              // TODO this might be double logged, but safer to log here anyway
+              log.error("Exception from thread {}", finalI, e);
             }
+            return null;
           };
-
-      threads[i].start();
+      callees.add(call);
     }
 
-    for (Thread thread : threads) thread.join();
+    executorService.invokeAll(callees);
+    executorService.shutdown();
+
+    // TIMEOUT * 3 there are 3 tests - add, replace, delete each running for the length of TIMEOUT
+    assertTrue(
+        "Running for too long...", executorService.awaitTermination(TIMEOUT * 3, TimeUnit.SECONDS));
 
     boolean success = true;
 
-    for (@SuppressWarnings({"rawtypes"}) List e : collectErrors) {
+    for (List<String> e : collectErrors) {
       if (e != null && !e.isEmpty()) {
         success = false;
         log.error("{}", e);
@@ -98,7 +122,7 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
   }
 
   @SuppressWarnings({"unchecked"})
-  private void invokeBulkAddCall(int seed, ArrayList<String> errs) throws Exception {
+  private void invokeBulkAddCall(int seed, List<String> errs) throws Exception {
     String payload =
         "{\n"
             + "          'add-field' : {\n"
@@ -133,10 +157,10 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
     payload = payload.replace("replaceDynamicCopyFieldDest", dynamicCopyFldDest);
     payload = payload.replace("myNewFieldTypeName", newFieldTypeName);
 
+    // don't close publisher - gets closed at teardown
     RestTestHarness publisher = randomRestTestHarness(r);
     String response = publisher.post("/schema", SolrTestCaseJ4.json(payload));
-    @SuppressWarnings({"rawtypes"})
-    Map map = (Map) Utils.fromJSONString(response);
+    Map<String, Object> map = (Map<String, Object>) Utils.fromJSONString(response);
     Object errors = map.get("errors");
     if (errors != null) {
       errs.add(new String(Utils.toJSON(errors), StandardCharsets.UTF_8));
@@ -145,38 +169,38 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
 
     // get another node
     Set<String> errmessages = new HashSet<>();
+    // don't close harness - gets closed at teardown
     RestTestHarness harness = randomRestTestHarness(r);
-    try {
-      long startTime = System.nanoTime();
-      long maxTimeoutMillis = 100000;
-      while (TimeUnit.MILLISECONDS.convert(System.nanoTime() - startTime, TimeUnit.NANOSECONDS)
-          < maxTimeoutMillis) {
-        errmessages.clear();
-        @SuppressWarnings({"rawtypes"})
-        Map m = getObj(harness, aField, "fields");
-        if (m == null) errmessages.add(StrUtils.formatString("field {0} not created", aField));
-
-        m = getObj(harness, dynamicFldName, "dynamicFields");
-        if (m == null)
-          errmessages.add(StrUtils.formatString("dynamic field {0} not created", dynamicFldName));
-
-        @SuppressWarnings({"rawtypes"})
-        List l = getSourceCopyFields(harness, aField);
-        if (!checkCopyField(l, aField, dynamicCopyFldDest))
-          errmessages.add(
-              StrUtils.formatString(
-                  "CopyField source={0},dest={1} not created", aField, dynamicCopyFldDest));
-
-        m = getObj(harness, newFieldTypeName, "fieldTypes");
-        if (m == null)
-          errmessages.add(StrUtils.formatString("new type {0}  not created", newFieldTypeName));
-
-        if (errmessages.isEmpty()) break;
-
-        Thread.sleep(10);
+    TimeOut timeout = new TimeOut(TIMEOUT, TimeUnit.SECONDS, TimeSource.NANO_TIME);
+    while (!timeout.hasTimedOut()) {
+      errmessages.clear();
+      Map<?, ?> m = getObj(harness, aField, "fields");
+      if (m == null) {
+        errmessages.add(StrUtils.formatString("field {0} not created", aField));
       }
-    } finally {
-      harness.close();
+
+      m = getObj(harness, dynamicFldName, "dynamicFields");
+      if (m == null) {
+        errmessages.add(StrUtils.formatString("dynamic field {0} not created", dynamicFldName));
+      }
+
+      List<Map<String, String>> l = getSourceCopyFields(harness, aField);
+      if (!checkCopyField(l, aField, dynamicCopyFldDest)) {
+        errmessages.add(
+            StrUtils.formatString(
+                "CopyField source={0},dest={1} not created", aField, dynamicCopyFldDest));
+      }
+
+      m = getObj(harness, newFieldTypeName, "fieldTypes");
+      if (m == null) {
+        errmessages.add(StrUtils.formatString("new type {0}  not created", newFieldTypeName));
+      }
+
+      if (errmessages.isEmpty()) {
+        break;
+      }
+
+      timeout.sleep(10);
     }
     if (!errmessages.isEmpty()) {
       errs.addAll(errmessages);
@@ -184,7 +208,7 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
   }
 
   @SuppressWarnings({"unchecked"})
-  private void invokeBulkReplaceCall(int seed, ArrayList<String> errs) throws Exception {
+  private void invokeBulkReplaceCall(int seed, List<String> errs) throws Exception {
     String payload =
         "{\n"
             + "          'replace-field' : {\n"
@@ -213,10 +237,10 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
     payload = payload.replace("replaceDynamicField", dynamicFldName);
     payload = payload.replace("myNewFieldTypeName", newFieldTypeName);
 
+    // don't close publisher - gets closed at teardown
     RestTestHarness publisher = randomRestTestHarness(r);
     String response = publisher.post("/schema", SolrTestCaseJ4.json(payload));
-    @SuppressWarnings({"rawtypes"})
-    Map map = (Map) Utils.fromJSONString(response);
+    Map<String, Object> map = (Map<String, Object>) Utils.fromJSONString(response);
     Object errors = map.get("errors");
     if (errors != null) {
       errs.add(new String(Utils.toJSON(errors), StandardCharsets.UTF_8));
@@ -225,41 +249,39 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
 
     // get another node
     Set<String> errmessages = new HashSet<>();
+    // don't close harness - gets closed at teardown
     RestTestHarness harness = randomRestTestHarness(r);
-    try {
-      long startTime = System.nanoTime();
-      long maxTimeoutMillis = 100000;
-      while (TimeUnit.MILLISECONDS.convert(System.nanoTime() - startTime, TimeUnit.NANOSECONDS)
-          < maxTimeoutMillis) {
-        errmessages.clear();
-        @SuppressWarnings({"rawtypes"})
-        Map m = getObj(harness, aField, "fields");
-        if (m == null)
-          errmessages.add(StrUtils.formatString("field {0} no longer present", aField));
-
-        m = getObj(harness, dynamicFldName, "dynamicFields");
-        if (m == null)
-          errmessages.add(
-              StrUtils.formatString("dynamic field {0} no longer present", dynamicFldName));
-
-        @SuppressWarnings({"rawtypes"})
-        List l = getSourceCopyFields(harness, aField);
-        if (!checkCopyField(l, aField, dynamicCopyFldDest))
-          errmessages.add(
-              StrUtils.formatString(
-                  "CopyField source={0},dest={1} no longer present", aField, dynamicCopyFldDest));
-
-        m = getObj(harness, newFieldTypeName, "fieldTypes");
-        if (m == null)
-          errmessages.add(
-              StrUtils.formatString("new type {0} no longer present", newFieldTypeName));
-
-        if (errmessages.isEmpty()) break;
-
-        Thread.sleep(10);
+    TimeOut timeout = new TimeOut(TIMEOUT, TimeUnit.SECONDS, TimeSource.NANO_TIME);
+    while (!timeout.hasTimedOut()) {
+      errmessages.clear();
+      Map<?, ?> m = getObj(harness, aField, "fields");
+      if (m == null) {
+        errmessages.add(StrUtils.formatString("field {0} no longer present", aField));
       }
-    } finally {
-      harness.close();
+
+      m = getObj(harness, dynamicFldName, "dynamicFields");
+      if (m == null) {
+        errmessages.add(
+            StrUtils.formatString("dynamic field {0} no longer present", dynamicFldName));
+      }
+
+      List<Map<String, String>> l = getSourceCopyFields(harness, aField);
+      if (!checkCopyField(l, aField, dynamicCopyFldDest)) {
+        errmessages.add(
+            StrUtils.formatString(
+                "CopyField source={0},dest={1} no longer present", aField, dynamicCopyFldDest));
+      }
+
+      m = getObj(harness, newFieldTypeName, "fieldTypes");
+      if (m == null) {
+        errmessages.add(StrUtils.formatString("new type {0} no longer present", newFieldTypeName));
+      }
+
+      if (errmessages.isEmpty()) {
+        break;
+      }
+
+      timeout.sleep(10);
     }
     if (!errmessages.isEmpty()) {
       errs.addAll(errmessages);
@@ -267,7 +289,7 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
   }
 
   @SuppressWarnings({"unchecked"})
-  private void invokeBulkDeleteCall(int seed, ArrayList<String> errs) throws Exception {
+  private void invokeBulkDeleteCall(int seed, List<String> errs) throws Exception {
     String payload =
         "{\n"
             + "          'delete-copy-field' : {\n"
@@ -288,10 +310,10 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
     payload = payload.replace("replaceDynamicCopyFieldDest", dynamicCopyFldDest);
     payload = payload.replace("myNewFieldTypeName", newFieldTypeName);
 
+    // don't close publisher - gets closed at teardown
     RestTestHarness publisher = randomRestTestHarness(r);
     String response = publisher.post("/schema", SolrTestCaseJ4.json(payload));
-    @SuppressWarnings({"rawtypes"})
-    Map map = (Map) Utils.fromJSONString(response);
+    Map<String, Object> map = (Map<String, Object>) Utils.fromJSONString(response);
     Object errors = map.get("errors");
     if (errors != null) {
       errs.add(new String(Utils.toJSON(errors), StandardCharsets.UTF_8));
@@ -300,49 +322,52 @@ public class TestBulkSchemaConcurrent extends AbstractFullDistribZkTestBase {
 
     // get another node
     Set<String> errmessages = new HashSet<>();
+    // don't close harness - gets closed at teardown
     RestTestHarness harness = randomRestTestHarness(r);
-    try {
-      long startTime = System.nanoTime();
-      long maxTimeoutMillis = 100000;
-      while (TimeUnit.MILLISECONDS.convert(System.nanoTime() - startTime, TimeUnit.NANOSECONDS)
-          < maxTimeoutMillis) {
-        errmessages.clear();
-        @SuppressWarnings({"rawtypes"})
-        Map m = getObj(harness, aField, "fields");
-        if (m != null) errmessages.add(StrUtils.formatString("field {0} still exists", aField));
-
-        m = getObj(harness, dynamicFldName, "dynamicFields");
-        if (m != null)
-          errmessages.add(StrUtils.formatString("dynamic field {0} still exists", dynamicFldName));
-
-        @SuppressWarnings({"rawtypes"})
-        List l = getSourceCopyFields(harness, aField);
-        if (checkCopyField(l, aField, dynamicCopyFldDest))
-          errmessages.add(
-              StrUtils.formatString(
-                  "CopyField source={0},dest={1} still exists", aField, dynamicCopyFldDest));
-
-        m = getObj(harness, newFieldTypeName, "fieldTypes");
-        if (m != null)
-          errmessages.add(StrUtils.formatString("new type {0} still exists", newFieldTypeName));
-
-        if (errmessages.isEmpty()) break;
-
-        Thread.sleep(10);
+    TimeOut timeout = new TimeOut(TIMEOUT, TimeUnit.SECONDS, TimeSource.NANO_TIME);
+    while (!timeout.hasTimedOut()) {
+      errmessages.clear();
+      Map<?, ?> m = getObj(harness, aField, "fields");
+      if (m != null) {
+        errmessages.add(StrUtils.formatString("field {0} still exists", aField));
+      }
+
+      m = getObj(harness, dynamicFldName, "dynamicFields");
+      if (m != null) {
+        errmessages.add(StrUtils.formatString("dynamic field {0} still exists", dynamicFldName));
       }
-    } finally {
-      harness.close();
+
+      List<Map<String, String>> l = getSourceCopyFields(harness, aField);
+      if (checkCopyField(l, aField, dynamicCopyFldDest)) {
+        errmessages.add(
+            StrUtils.formatString(
+                "CopyField source={0},dest={1} still exists", aField, dynamicCopyFldDest));
+      }
+
+      m = getObj(harness, newFieldTypeName, "fieldTypes");
+      if (m != null) {
+        errmessages.add(StrUtils.formatString("new type {0} still exists", newFieldTypeName));
+      }
+
+      if (errmessages.isEmpty()) {
+        break;
+      }
+
+      timeout.sleep(10);
     }
     if (!errmessages.isEmpty()) {
       errs.addAll(errmessages);
     }
   }
 
-  private boolean checkCopyField(
-      @SuppressWarnings({"rawtypes"}) List<Map> l, String src, String dest) {
-    if (l == null) return false;
-    for (@SuppressWarnings({"rawtypes"}) Map map : l) {
-      if (src.equals(map.get("source")) && dest.equals(map.get("dest"))) return true;
+  private boolean checkCopyField(List<Map<String, String>> l, String src, String dest) {
+    if (l == null) {
+      return false;
+    }
+    for (Map<String, String> map : l) {
+      if (src.equals(map.get("source")) && dest.equals(map.get("dest"))) {
+        return true;
+      }
     }
     return false;
   }


[solr] 06/12: SOLR-16187: ExecutorUtil#awaitTermination shouldn't wait forever (#840)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit a066431bb59e49b32e13e41da2c9a7daf80cab6d
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Tue Sep 6 10:30:09 2022 -0400

    SOLR-16187: ExecutorUtil#awaitTermination shouldn't wait forever (#840)
---
 .../org/apache/solr/common/util/ExecutorUtil.java  | 28 +++++--
 .../apache/solr/common/util/ExecutorUtilTest.java  | 98 ++++++++++++++++++++++
 2 files changed, 118 insertions(+), 8 deletions(-)

diff --git a/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java b/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java
index d4e1673c906..e85470d70fd 100644
--- a/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java
+++ b/solr/solrj/src/java/org/apache/solr/common/util/ExecutorUtil.java
@@ -96,15 +96,27 @@ public class ExecutorUtil {
   }
 
   public static void awaitTermination(ExecutorService pool) {
-    boolean shutdown = false;
-    while (!shutdown) {
-      try {
-        // Wait a while for existing tasks to terminate
-        shutdown = pool.awaitTermination(60, TimeUnit.SECONDS);
-      } catch (InterruptedException ie) {
-        // Preserve interrupt status
-        Thread.currentThread().interrupt();
+    awaitTermination(pool, 60, TimeUnit.SECONDS);
+  }
+
+  // Used in testing to not have to wait the full 60 seconds.
+  static void awaitTermination(ExecutorService pool, long timeout, TimeUnit unit) {
+    try {
+      // Wait a while for existing tasks to terminate.
+      if (!pool.awaitTermination(timeout, unit)) {
+        // We want to force shutdown any remaining threads.
+        pool.shutdownNow();
+        // Wait again for forced threads to stop.
+        if (!pool.awaitTermination(timeout, unit)) {
+          log.error("Threads from pool {} did not forcefully stop.", pool);
+          throw new RuntimeException("Timeout waiting for pool " + pool + " to shutdown.");
+        }
       }
+    } catch (InterruptedException ie) {
+      // (Re-)Cancel if current thread also interrupted
+      pool.shutdownNow();
+      // Preserve interrupt status
+      Thread.currentThread().interrupt();
     }
   }
 
diff --git a/solr/solrj/src/test/org/apache/solr/common/util/ExecutorUtilTest.java b/solr/solrj/src/test/org/apache/solr/common/util/ExecutorUtilTest.java
new file mode 100644
index 00000000000..d7cfbe9c321
--- /dev/null
+++ b/solr/solrj/src/test/org/apache/solr/common/util/ExecutorUtilTest.java
@@ -0,0 +1,98 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.common.util;
+
+import com.carrotsearch.randomizedtesting.annotations.Timeout;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.util.TimeOut;
+import org.junit.Test;
+
+public class ExecutorUtilTest extends SolrTestCase {
+  @Test
+  // Must prevent runaway failures so limit this to short timeframe in case of failure
+  @Timeout(millis = 3000)
+  public void testExecutorUtilAwaitsTerminationEnds() throws Exception {
+    final long awaitTerminationTimeout = 100;
+    final long threadTimeoutDuration = 3 * awaitTerminationTimeout;
+    final TimeUnit testTimeUnit = TimeUnit.MILLISECONDS;
+
+    // check that if there is a non interruptable thread that awaitTermination eventually returns.
+
+    ExecutorService executorService =
+        ExecutorUtil.newMDCAwareSingleThreadExecutor(
+            new SolrNamedThreadFactory(this.getClass().getSimpleName() + "non-interruptable"));
+    final AtomicInteger interruptCount = new AtomicInteger();
+    Future<Boolean> nonInterruptableFuture =
+        executorService.submit(
+            () -> getTestThread(threadTimeoutDuration, testTimeUnit, interruptCount, false));
+    executorService.shutdownNow();
+    assertThrows(
+        RuntimeException.class,
+        () ->
+            ExecutorUtil.awaitTermination(executorService, awaitTerminationTimeout, testTimeUnit));
+
+    // Thread should not have finished in await termination.
+    assertFalse(nonInterruptableFuture.isDone());
+    assertTrue(interruptCount.get() > 0);
+
+    // Thread should have finished by now.
+    Thread.sleep(TimeUnit.MILLISECONDS.convert(threadTimeoutDuration, testTimeUnit));
+    assertTrue(nonInterruptableFuture.isDone());
+    assertTrue(nonInterruptableFuture.get());
+
+    // check that if there is an interruptable thread that awaitTermination forcefully returns.
+
+    ExecutorService executorService2 =
+        ExecutorUtil.newMDCAwareSingleThreadExecutor(
+            new SolrNamedThreadFactory(this.getClass().getSimpleName() + "interruptable"));
+    interruptCount.set(0);
+    Future<Boolean> interruptableFuture =
+        executorService2.submit(
+            () -> getTestThread(threadTimeoutDuration, testTimeUnit, interruptCount, true));
+    executorService2.shutdownNow();
+    ExecutorUtil.awaitTermination(executorService2, awaitTerminationTimeout, testTimeUnit);
+
+    // Thread should have been interrupted.
+    assertTrue(interruptableFuture.isDone());
+    assertTrue(interruptCount.get() > 0);
+    assertFalse(interruptableFuture.get());
+  }
+
+  private boolean getTestThread(
+      long threadTimeoutDuration,
+      TimeUnit testTimeUnit,
+      AtomicInteger interruptCount,
+      boolean interruptable) {
+    TimeOut threadTimeout = new TimeOut(threadTimeoutDuration, testTimeUnit, TimeSource.NANO_TIME);
+    while (!threadTimeout.hasTimedOut()) {
+      try {
+        threadTimeout.sleep(TimeUnit.MILLISECONDS.convert(threadTimeoutDuration, testTimeUnit));
+      } catch (InterruptedException interruptedException) {
+        interruptCount.incrementAndGet();
+        if (interruptable) {
+          Thread.currentThread().interrupt();
+          return false; // didn't run full time
+        }
+      }
+    }
+    return true; // ran full time
+  }
+}


[solr] 11/12: SOLR-16091 Improve test logging (#736)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit 3f7bd2d9564b4ca85a791a67308e41258d775401
Author: Mike Drob <md...@apache.org>
AuthorDate: Mon Mar 14 21:19:12 2022 -0500

    SOLR-16091 Improve test logging (#736)
    
    Improved logging in TestContainerPlugin and TestDistribPackageStore.
    
    Switch an instance of System.out to using a logger.
    Add ErrorLogMute blocks for expected errors.
    Reduce amount of retries when waiting for state since we have proper expected concurrency hooks in place.
    Randomize whether the test forces V2 APIs
    
    Co-authored-by: Christine Poerschke <cp...@apache.org>
    (cherry picked from commit 6d64c14bf238f4c67b91c757923c5818402d71a9)
---
 .../solr/filestore/TestDistribPackageStore.java    |  34 +-
 .../apache/solr/handler/TestContainerPlugin.java   | 547 +++++++++------------
 2 files changed, 247 insertions(+), 334 deletions(-)

diff --git a/solr/core/src/test/org/apache/solr/filestore/TestDistribPackageStore.java b/solr/core/src/test/org/apache/solr/filestore/TestDistribPackageStore.java
index ea9926402eb..0f7bbee6b69 100644
--- a/solr/core/src/test/org/apache/solr/filestore/TestDistribPackageStore.java
+++ b/solr/core/src/test/org/apache/solr/filestore/TestDistribPackageStore.java
@@ -23,6 +23,7 @@ import static org.hamcrest.CoreMatchers.containsString;
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.lang.invoke.MethodHandles;
 import java.nio.ByteBuffer;
 import java.nio.file.Paths;
 import java.util.Collections;
@@ -56,10 +57,13 @@ import org.apache.zookeeper.server.ByteBufferInputStream;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 @LogLevel(
     "org.apache.solr.filestore.PackageStoreAPI=DEBUG;org.apache.solr.filestore.DistribPackageStore=DEBUG")
 public class TestDistribPackageStore extends SolrCloudTestCase {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
   @Before
   public void setup() {
@@ -225,6 +229,11 @@ public class TestDistribPackageStore extends SolrCloudTestCase {
     }
   }
 
+  public static <T extends NavigableObject> T assertResponseValues(
+      Callable<T> callable, Map<String, Object> vals) throws Exception {
+    return assertResponseValues(1, callable, vals);
+  }
+
   public static NavigableObject assertResponseValues(
       int repeats, SolrClient client, SolrRequest<?> req, Map<String, Object> vals)
       throws Exception {
@@ -245,9 +254,9 @@ public class TestDistribPackageStore extends SolrCloudTestCase {
    * @throws Exception if the callable throws an Exception, or on interrupt between retries
    */
   @SuppressWarnings({"unchecked"})
-  public static NavigableObject assertResponseValues(
-      int repeats, Callable<NavigableObject> callable, Map<String, Object> vals) throws Exception {
-    NavigableObject rsp = null;
+  public static <T extends NavigableObject> T assertResponseValues(
+      int repeats, Callable<T> callable, Map<String, Object> vals) throws Exception {
+    T rsp = null;
 
     for (int i = 0; i < repeats; i++) {
       if (i > 0) {
@@ -290,6 +299,9 @@ public class TestDistribPackageStore extends SolrCloudTestCase {
               Utils.toJSONString(actual));
         }
       }
+      if (passed) {
+        break;
+      }
     }
     return rsp;
   }
@@ -300,19 +312,9 @@ public class TestDistribPackageStore extends SolrCloudTestCase {
     try (HttpSolrClient client = (HttpSolrClient) jetty.newClient()) {
       PackageUtils.uploadKey(
           bytes, path, Paths.get(jetty.getCoreContainer().getSolrHome()), client);
-      Object resp =
-          Utils.executeGET(
-              client.getHttpClient(),
-              jetty.getBaseURLV2().toString() + "/node/files" + path + "?sync=true",
-              null);
-      System.out.println(
-          "sync resp: "
-              + jetty.getBaseURLV2().toString()
-              + "/node/files"
-              + path
-              + "?sync=true"
-              + " ,is: "
-              + resp);
+      String url = jetty.getBaseURLV2() + "/node/files" + path + "?sync=true";
+      Object resp = Utils.executeGET(client.getHttpClient(), url, null);
+      log.info("sync resp: {} was {}", url, resp);
     }
     checkAllNodesForFile(
         cluster,
diff --git a/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java b/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
index 39b939dd80a..0769613ef18 100644
--- a/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
+++ b/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
@@ -22,8 +22,8 @@ import static java.util.Collections.singletonMap;
 import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET;
 import static org.apache.solr.filestore.TestDistribPackageStore.readFile;
 import static org.apache.solr.filestore.TestDistribPackageStore.uploadKey;
+import static org.hamcrest.Matchers.containsString;
 
-import com.google.common.collect.ImmutableMap;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.ByteBuffer;
@@ -49,7 +49,6 @@ import org.apache.solr.client.solrj.response.V2Response;
 import org.apache.solr.cloud.ClusterSingleton;
 import org.apache.solr.cloud.MiniSolrCloudCluster;
 import org.apache.solr.cloud.SolrCloudTestCase;
-import org.apache.solr.common.NavigableObject;
 import org.apache.solr.common.annotation.JsonProperty;
 import org.apache.solr.common.util.ReflectMapWriter;
 import org.apache.solr.core.CoreContainer;
@@ -61,6 +60,7 @@ import org.apache.solr.pkg.TestPackages;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.SolrQueryResponse;
 import org.apache.solr.security.PermissionNameProvider;
+import org.apache.solr.util.ErrorLogMuter;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -68,354 +68,243 @@ import org.junit.Test;
 public class TestContainerPlugin extends SolrCloudTestCase {
   private Phaser phaser;
 
+  private boolean forceV2;
+
   @Before
-  public void setup() {
+  public void setup() throws Exception {
     System.setProperty("enable.packages", "true");
     phaser = new Phaser();
+    forceV2 = random().nextBoolean();
+
+    int nodes = TEST_NIGHTLY ? 4 : 2;
+    cluster = configureCluster(nodes).withJettyConfig(jetty -> jetty.enableV2(true)).configure();
+    cluster.getOpenOverseer().getCoreContainer().getContainerPluginsRegistry().setPhaser(phaser);
   }
 
   @After
-  public void teardown() {
+  public void teardown() throws Exception {
+    shutdownCluster();
     System.clearProperty("enable.packages");
   }
 
   @Test
   public void testApi() throws Exception {
-    MiniSolrCloudCluster cluster =
-        configureCluster(4).withJettyConfig(jetty -> jetty.enableV2(true)).configure();
-    ContainerPluginsRegistry pluginsRegistry =
-        cluster.getOpenOverseer().getCoreContainer().getContainerPluginsRegistry();
-    pluginsRegistry.setPhaser(phaser);
-
     int version = phaser.getPhase();
 
-    String errPath = "/error/details[0]/errorMessages[0]";
-    try {
-      PluginMeta plugin = new PluginMeta();
+    PluginMeta plugin = new PluginMeta();
+    V2Request addPlugin = postPlugin(singletonMap("add", plugin));
+
+    // test with an invalid class
+    try (ErrorLogMuter errors = ErrorLogMuter.substring("TestContainerPlugin$C2")) {
       plugin.name = "testplugin";
       plugin.klass = C2.class.getName();
-      // test with an invalid class
-      V2Request req =
-          new V2Request.Builder("/cluster/plugin")
-              .forceV2(true)
-              .POST()
-              .withPayload(singletonMap("add", plugin))
-              .build();
-      expectError(req, cluster.getSolrClient(), errPath, "No method with @Command in class");
-
-      // test with a valid class. This should succeed now
-      plugin.klass = C3.class.getName();
-      req.process(cluster.getSolrClient());
-
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      // just check if the plugin is indeed registered
-      V2Request readPluginState =
-          new V2Request.Builder("/cluster/plugin").forceV2(true).GET().build();
-      V2Response rsp = readPluginState.process(cluster.getSolrClient());
-      assertEquals(C3.class.getName(), rsp._getStr("/plugin/testplugin/class", null));
-
-      // let's test the plugin
-      TestDistribPackageStore.assertResponseValues(
-          10,
-          () ->
-              new V2Request.Builder("/plugin/my/plugin")
-                  .forceV2(true)
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()),
-          ImmutableMap.of("/testkey", "testval"));
-
-      // now remove the plugin
-      new V2Request.Builder("/cluster/plugin")
-          .POST()
-          .forceV2(true)
-          .withPayload("{remove : testplugin}")
-          .build()
-          .process(cluster.getSolrClient());
-
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      // verify it is removed
-      rsp = readPluginState.process(cluster.getSolrClient());
-      assertEquals(null, rsp._get("/plugin/testplugin/class", null));
+      expectError(addPlugin, "No method with @Command in class");
+      assertEquals(1, errors.getCount());
+    }
+
+    // test with a valid class. This should succeed now
+    plugin.klass = C3.class.getName();
+    addPlugin.process(cluster.getSolrClient());
 
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    // just check if the plugin is indeed registered
+    Callable<V2Response> readPluginState = getPlugin("/cluster/plugin");
+    V2Response rsp = readPluginState.call();
+    assertEquals(C3.class.getName(), rsp._getStr("/plugin/testplugin/class", null));
+
+    // let's test the plugin
+    TestDistribPackageStore.assertResponseValues(
+        getPlugin("/plugin/my/plugin"), Map.of("/testkey", "testval"));
+
+    // now remove the plugin
+    postPlugin("{remove : testplugin}").process(cluster.getSolrClient());
+
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    // verify it is removed
+    rsp = readPluginState.call();
+    assertNull(rsp._get("/plugin/testplugin/class", null));
+
+    try (ErrorLogMuter errors = ErrorLogMuter.substring("TestContainerPlugin$C4")) {
       // test with a class  @EndPoint methods. This also uses a template in the path name
       plugin.klass = C4.class.getName();
       plugin.name = "collections";
       plugin.pathPrefix = "collections";
-      expectError(
-          req, cluster.getSolrClient(), errPath, "path must not have a prefix: collections");
-
-      plugin.name = "my-random-name";
-      plugin.pathPrefix = "my-random-prefix";
-
-      req.process(cluster.getSolrClient());
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      // let's test the plugin
-      TestDistribPackageStore.assertResponseValues(
-          10,
-          () ->
-              new V2Request.Builder("/my-random-name/my/plugin")
-                  .forceV2(true)
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()),
-          ImmutableMap.of("/method.name", "m1"));
-
-      TestDistribPackageStore.assertResponseValues(
-          10,
-          () ->
-              new V2Request.Builder("/my-random-prefix/their/plugin")
-                  .forceV2(true)
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()),
-          ImmutableMap.of("/method.name", "m2"));
-      // now remove the plugin
-      new V2Request.Builder("/cluster/plugin")
-          .POST()
-          .forceV2(true)
-          .withPayload("{remove : my-random-name}")
-          .build()
-          .process(cluster.getSolrClient());
-
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      expectFail(
-          () ->
-              new V2Request.Builder("/my-random-prefix/their/plugin")
-                  .forceV2(true)
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()));
-      expectFail(
-          () ->
-              new V2Request.Builder("/my-random-prefix/their/plugin")
-                  .forceV2(true)
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()));
-
-      // test ClusterSingleton plugin
-      plugin.name = "clusterSingleton";
-      plugin.klass = C6.class.getName();
-      req.process(cluster.getSolrClient());
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      // just check if the plugin is indeed registered
-      readPluginState = new V2Request.Builder("/cluster/plugin").forceV2(true).GET().build();
-      rsp = readPluginState.process(cluster.getSolrClient());
-      assertEquals(C6.class.getName(), rsp._getStr("/plugin/clusterSingleton/class", null));
-
-      assertTrue("ccProvided", C6.ccProvided);
-      assertTrue("startCalled", C6.startCalled);
-      assertFalse("stopCalled", C6.stopCalled);
-
-      assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new CC()));
-      assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new CC1()));
-      assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new CC2()));
-
-      CConfig cfg = new CConfig();
-      cfg.boolVal = Boolean.TRUE;
-      cfg.strVal = "Something";
-      cfg.longVal = 1234L;
-      PluginMeta p = new PluginMeta();
-      p.name = "hello";
-      p.klass = CC.class.getName();
-      p.config = cfg;
-
-      new V2Request.Builder("/cluster/plugin")
-          .forceV2(true)
-          .POST()
-          .withPayload(singletonMap("add", p))
-          .build()
-          .process(cluster.getSolrClient());
-
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      TestDistribPackageStore.assertResponseValues(
-          10,
-          () ->
-              new V2Request.Builder("hello/plugin")
-                  .forceV2(true)
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()),
-          ImmutableMap.of(
-              "/config/boolVal", "true", "/config/strVal", "Something", "/config/longVal", "1234"));
-
-      cfg.strVal = "Something else";
-      new V2Request.Builder("/cluster/plugin")
-          .forceV2(true)
-          .POST()
-          .withPayload(singletonMap("update", p))
-          .build()
-          .process(cluster.getSolrClient());
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      TestDistribPackageStore.assertResponseValues(
-          10,
-          () ->
-              new V2Request.Builder("hello/plugin")
-                  .forceV2(true)
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()),
-          ImmutableMap.of(
-              "/config/boolVal", "true", "/config/strVal", cfg.strVal, "/config/longVal", "1234"));
-
-      // kill the Overseer leader
-      for (JettySolrRunner jetty : cluster.getJettySolrRunners()) {
-        if (!jetty.getCoreContainer().getZkController().getOverseer().isClosed()) {
-          cluster.stopJettySolrRunner(jetty);
-          cluster.waitForJettyToStop(jetty);
-        }
-      }
-      assertTrue("stopCalled", C6.stopCalled);
-    } finally {
-      cluster.shutdown();
+      expectError(addPlugin, "path must not have a prefix: collections");
+      assertEquals(1, errors.getCount());
     }
-  }
 
-  private void expectFail(ThrowingRunnable runnable) throws Exception {
-    for (int i = 0; i < 20; i++) {
-      try {
-        runnable.run();
-      } catch (Throwable throwable) {
-        return;
+    plugin.name = "my-random-name";
+    plugin.pathPrefix = "my-random-prefix";
+
+    addPlugin.process(cluster.getSolrClient());
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    // let's test the plugin
+    TestDistribPackageStore.assertResponseValues(
+        getPlugin("/my-random-name/my/plugin"), Map.of("/method.name", "m1"));
+
+    TestDistribPackageStore.assertResponseValues(
+        getPlugin("/my-random-prefix/their/plugin"), Map.of("/method.name", "m2"));
+    // now remove the plugin
+    postPlugin("{remove : my-random-name}").process(cluster.getSolrClient());
+
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    try (ErrorLogMuter errors = ErrorLogMuter.substring("/my-random-prefix/their/plugin")) {
+      RemoteExecutionException e =
+          assertThrows(
+              RemoteExecutionException.class,
+              () -> getPlugin("/my-random-prefix/their/plugin").call());
+      assertEquals(404, e.code());
+      assertThat(
+          e.getMetaData().findRecursive("error", "msg").toString(),
+          containsString("no core retrieved"));
+      // V2HttpCall will separately log the path and stack trace, probably could be fixed
+      assertEquals(2, errors.getCount());
+    }
+
+    // test ClusterSingleton plugin
+    plugin.name = "clusterSingleton";
+    plugin.klass = C6.class.getName();
+    addPlugin.process(cluster.getSolrClient());
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    // just check if the plugin is indeed registered
+    rsp = readPluginState.call();
+    assertEquals(C6.class.getName(), rsp._getStr("/plugin/clusterSingleton/class", null));
+
+    assertTrue("ccProvided", C6.ccProvided);
+    assertTrue("startCalled", C6.startCalled);
+    assertFalse("stopCalled", C6.stopCalled);
+
+    assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new CC()));
+    assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new CC1()));
+    assertEquals(CConfig.class, ContainerPluginsRegistry.getConfigClass(new CC2()));
+
+    CConfig cfg = new CConfig();
+    cfg.boolVal = Boolean.TRUE;
+    cfg.strVal = "Something";
+    cfg.longVal = 1234L;
+    PluginMeta p = new PluginMeta();
+    p.name = "hello";
+    p.klass = CC.class.getName();
+    p.config = cfg;
+
+    postPlugin(singletonMap("add", p)).process(cluster.getSolrClient());
+
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    TestDistribPackageStore.assertResponseValues(
+        getPlugin("hello/plugin"),
+        Map.of(
+            "/config/boolVal", "true", "/config/strVal", "Something", "/config/longVal", "1234"));
+
+    cfg.strVal = "Something else";
+    postPlugin(singletonMap("update", p)).process(cluster.getSolrClient());
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    TestDistribPackageStore.assertResponseValues(
+        getPlugin("hello/plugin"),
+        Map.of("/config/boolVal", "true", "/config/strVal", cfg.strVal, "/config/longVal", "1234"));
+
+    // kill the Overseer leader
+    for (JettySolrRunner jetty : cluster.getJettySolrRunners()) {
+      if (!jetty.getCoreContainer().getZkController().getOverseer().isClosed()) {
+        cluster.stopJettySolrRunner(jetty);
+        cluster.waitForJettyToStop(jetty);
       }
-      Thread.sleep(100);
     }
-    fail("should have failed with an exception");
+    assertTrue("stopCalled", C6.stopCalled);
   }
 
   @Test
   public void testApiFromPackage() throws Exception {
-    MiniSolrCloudCluster cluster =
-        configureCluster(4).withJettyConfig(jetty -> jetty.enableV2(true)).configure();
     String FILE1 = "/myplugin/v1.jar";
     String FILE2 = "/myplugin/v2.jar";
-    ContainerPluginsRegistry pluginsRegistry =
-        cluster.getOpenOverseer().getCoreContainer().getContainerPluginsRegistry();
-    pluginsRegistry.setPhaser(phaser);
 
     int version = phaser.getPhase();
 
-    String errPath = "/error/details[0]/errorMessages[0]";
-    try {
-      byte[] derFile = readFile("cryptokeys/pub_key512.der");
-      uploadKey(derFile, PackageStoreAPI.KEYS_DIR + "/pub_key512.der", cluster);
-      TestPackages.postFileAndWait(
-          cluster,
-          "runtimecode/containerplugin.v.1.jar.bin",
-          FILE1,
-          "pmrmWCDafdNpYle2rueAGnU2J6NYlcAey9mkZYbqh+5RdYo2Ln+llLF9voyRj+DDivK9GV1XdtKvD9rgCxlD7Q==");
-      TestPackages.postFileAndWait(
-          cluster,
-          "runtimecode/containerplugin.v.2.jar.bin",
-          FILE2,
-          "StR3DmqaUSL7qjDOeVEiCqE+ouiZAkW99fsL48F9oWG047o7NGgwwZ36iGgzDC3S2tPaFjRAd9Zg4UK7OZLQzg==");
-
-      // We have two versions of the plugin in 2 different jar files. they are already uploaded to
-      // the package store
-      Package.AddVersion add = new Package.AddVersion();
-      add.version = "1.0";
-      add.pkg = "mypkg";
-      add.files = singletonList(FILE1);
-      V2Request addPkgVersionReq =
-          new V2Request.Builder("/cluster/package")
-              .forceV2(true)
-              .POST()
-              .withPayload(singletonMap("add", add))
-              .build();
-      addPkgVersionReq.process(cluster.getSolrClient());
-
-      waitForAllNodesToSync(
-          cluster,
-          "/cluster/package",
-          Map.of(
-              ":result:packages:mypkg[0]:version",
-              "1.0",
-              ":result:packages:mypkg[0]:files[0]",
-              FILE1));
-
-      // Now lets create a plugin using v1 jar file
-      PluginMeta plugin = new PluginMeta();
-      plugin.name = "myplugin";
-      plugin.klass = "mypkg:org.apache.solr.handler.MyPlugin";
-      plugin.version = add.version;
-      final V2Request req1 =
-          new V2Request.Builder("/cluster/plugin")
-              .forceV2(true)
-              .POST()
-              .withPayload(singletonMap("add", plugin))
-              .build();
-      req1.process(cluster.getSolrClient());
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      // verify the plugin creation
-      TestDistribPackageStore.assertResponseValues(
-          10,
-          () ->
-              new V2Request.Builder("/cluster/plugin")
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()),
-          ImmutableMap.of(
-              "/plugin/myplugin/class", plugin.klass,
-              "/plugin/myplugin/version", plugin.version));
-      // let's test this now
-      Callable<NavigableObject> invokePlugin =
-          () ->
-              new V2Request.Builder("/plugin/my/path")
-                  .forceV2(true)
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient());
-      TestDistribPackageStore.assertResponseValues(
-          10, invokePlugin, ImmutableMap.of("/myplugin.version", "1.0"));
-
-      // now let's upload the jar file for version 2.0 of the plugin
-      add.version = "2.0";
-      add.files = singletonList(FILE2);
-      addPkgVersionReq.process(cluster.getSolrClient());
-
-      // here the plugin version is updated
-      plugin.version = add.version;
-      new V2Request.Builder("/cluster/plugin")
-          .forceV2(true)
-          .POST()
-          .withPayload(singletonMap("update", plugin))
-          .build()
-          .process(cluster.getSolrClient());
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-
-      // now verify if it is indeed updated
-      TestDistribPackageStore.assertResponseValues(
-          10,
-          () ->
-              new V2Request.Builder("/cluster/plugin")
-                  .GET()
-                  .build()
-                  .process(cluster.getSolrClient()),
-          ImmutableMap.of(
-              "/plugin/myplugin/class", plugin.klass, "/plugin/myplugin/version", "2.0"));
-      // invoke the plugin and test thye output
-      TestDistribPackageStore.assertResponseValues(
-          10, invokePlugin, ImmutableMap.of("/myplugin.version", "2.0"));
-
-      plugin.name = "plugin2";
-      plugin.klass = "mypkg:" + C5.class.getName();
-      plugin.version = "2.0";
-      req1.process(cluster.getSolrClient());
-      version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
-      assertNotNull(C5.classData);
-      assertEquals(1452, C5.classData.limit());
-    } finally {
-      cluster.shutdown();
-    }
+    byte[] derFile = readFile("cryptokeys/pub_key512.der");
+    uploadKey(derFile, PackageStoreAPI.KEYS_DIR + "/pub_key512.der", cluster);
+    TestPackages.postFileAndWait(
+        cluster,
+        "runtimecode/containerplugin.v.1.jar.bin",
+        FILE1,
+        "pmrmWCDafdNpYle2rueAGnU2J6NYlcAey9mkZYbqh+5RdYo2Ln+llLF9voyRj+DDivK9GV1XdtKvD9rgCxlD7Q==");
+    TestPackages.postFileAndWait(
+        cluster,
+        "runtimecode/containerplugin.v.2.jar.bin",
+        FILE2,
+        "StR3DmqaUSL7qjDOeVEiCqE+ouiZAkW99fsL48F9oWG047o7NGgwwZ36iGgzDC3S2tPaFjRAd9Zg4UK7OZLQzg==");
+
+    // We have two versions of the plugin in 2 different jar files. they are already uploaded to
+    // the package store
+    Package.AddVersion add = new Package.AddVersion();
+    add.version = "1.0";
+    add.pkg = "mypkg";
+    add.files = singletonList(FILE1);
+    V2Request addPkgVersionReq =
+        new V2Request.Builder("/cluster/package")
+            .forceV2(forceV2)
+            .POST()
+            .withPayload(singletonMap("add", add))
+            .build();
+    addPkgVersionReq.process(cluster.getSolrClient());
+
+    waitForAllNodesToSync(
+        cluster,
+        "/cluster/package",
+        Map.of(
+            ":result:packages:mypkg[0]:version",
+            "1.0",
+            ":result:packages:mypkg[0]:files[0]",
+            FILE1));
+
+    // Now lets create a plugin using v1 jar file
+    PluginMeta plugin = new PluginMeta();
+    plugin.name = "myplugin";
+    plugin.klass = "mypkg:org.apache.solr.handler.MyPlugin";
+    plugin.version = add.version;
+    final V2Request addPluginReq = postPlugin(singletonMap("add", plugin));
+    addPluginReq.process(cluster.getSolrClient());
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    // verify the plugin creation
+    TestDistribPackageStore.assertResponseValues(
+        getPlugin("/cluster/plugin"),
+        Map.of(
+            "/plugin/myplugin/class", plugin.klass,
+            "/plugin/myplugin/version", plugin.version));
+    // let's test this now
+    Callable<V2Response> invokePlugin = getPlugin("/plugin/my/path");
+    TestDistribPackageStore.assertResponseValues(invokePlugin, Map.of("/myplugin.version", "1.0"));
+
+    // now let's upload the jar file for version 2.0 of the plugin
+    add.version = "2.0";
+    add.files = singletonList(FILE2);
+    addPkgVersionReq.process(cluster.getSolrClient());
+
+    // here the plugin version is updated
+    plugin.version = add.version;
+    postPlugin(singletonMap("update", plugin)).process(cluster.getSolrClient());
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+
+    // now verify if it is indeed updated
+    TestDistribPackageStore.assertResponseValues(
+        getPlugin("/cluster/plugin"),
+        Map.of("/plugin/myplugin/class", plugin.klass, "/plugin/myplugin/version", "2.0"));
+    // invoke the plugin and test thye output
+    TestDistribPackageStore.assertResponseValues(invokePlugin, Map.of("/myplugin.version", "2.0"));
+
+    plugin.name = "plugin2";
+    plugin.klass = "mypkg:" + C5.class.getName();
+    plugin.version = "2.0";
+    addPluginReq.process(cluster.getSolrClient());
+    version = phaser.awaitAdvanceInterruptibly(version, 10, TimeUnit.SECONDS);
+    assertNotNull(C5.classData);
+    assertEquals(1452, C5.classData.limit());
   }
 
   public static class CC1 extends CC {}
@@ -548,16 +437,38 @@ public class TestContainerPlugin extends SolrCloudTestCase {
     }
   }
 
+  private Callable<V2Response> getPlugin(String path) {
+    V2Request req = new V2Request.Builder(path).forceV2(forceV2).GET().build();
+    return () -> req.process(cluster.getSolrClient());
+  }
+
+  private V2Request postPlugin(Object payload) {
+    return new V2Request.Builder("/cluster/plugin")
+        .forceV2(forceV2)
+        .POST()
+        .withPayload(payload)
+        .build();
+  }
+
   public static void waitForAllNodesToSync(
       MiniSolrCloudCluster cluster, String path, Map<String, Object> expected) throws Exception {
     for (JettySolrRunner jettySolrRunner : cluster.getJettySolrRunners()) {
       String baseUrl = jettySolrRunner.getBaseUrl().toString().replace("/solr", "/api");
       String url = baseUrl + path + "?wt=javabin";
+      // Allow multiple retries here because we need multiple nodes to update
+      // and our single phaser only ensures that one of them has reached expected state
       TestDistribPackageStore.assertResponseValues(10, new Fetcher(url, jettySolrRunner), expected);
     }
   }
 
-  private void expectError(V2Request req, SolrClient client, String errPath, String expectErrorMsg)
+  private void expectError(V2Request req, String expectErrorMsg)
+      throws IOException, SolrServerException {
+    String errPath = "/error/details[0]/errorMessages[0]";
+    expectError(req, cluster.getSolrClient(), errPath, expectErrorMsg);
+  }
+
+  private static void expectError(
+      V2Request req, SolrClient client, String errPath, String expectErrorMsg)
       throws IOException, SolrServerException {
     RemoteExecutionException e =
         expectThrows(RemoteExecutionException.class, () -> req.process(client));


[solr] 01/12: SOLR-16127 Ordered Executor orphans locks (#779)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit 1b48279e07cc86f0eff2a58cf33e86fa3c1d4ebe
Author: Mike Drob <md...@apache.org>
AuthorDate: Fri Apr 1 15:23:32 2022 -0500

    SOLR-16127 Ordered Executor orphans locks (#779)
---
 .../java/org/apache/solr/util/OrderedExecutor.java |  9 +++-
 .../org/apache/solr/util/OrderedExecutorTest.java  | 51 ++++++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/solr/core/src/java/org/apache/solr/util/OrderedExecutor.java b/solr/core/src/java/org/apache/solr/util/OrderedExecutor.java
index 15c1072e729..923560a8ac6 100644
--- a/solr/core/src/java/org/apache/solr/util/OrderedExecutor.java
+++ b/solr/core/src/java/org/apache/solr/util/OrderedExecutor.java
@@ -103,7 +103,14 @@ public class OrderedExecutor implements Executor {
         // myLock was successfully inserted
       }
       // won the lock
-      sizeSemaphore.acquire();
+      try {
+        sizeSemaphore.acquire();
+      } catch (InterruptedException e) {
+        if (t != null) {
+          map.remove(t).countDown();
+        }
+        throw e;
+      }
     }
 
     public void remove(T t) {
diff --git a/solr/core/src/test/org/apache/solr/util/OrderedExecutorTest.java b/solr/core/src/test/org/apache/solr/util/OrderedExecutorTest.java
index 24bb7c3f3a2..01f79978d43 100644
--- a/solr/core/src/test/org/apache/solr/util/OrderedExecutorTest.java
+++ b/solr/core/src/test/org/apache/solr/util/OrderedExecutorTest.java
@@ -230,4 +230,55 @@ public class OrderedExecutorTest extends SolrTestCase {
   private static class IntBox {
     int value;
   }
+
+  @Test
+  public void testMaxSize() throws InterruptedException {
+    OrderedExecutor orderedExecutor =
+        new OrderedExecutor(1, ExecutorUtil.newMDCAwareCachedThreadPool("single"));
+
+    CountDownLatch isRunning = new CountDownLatch(1);
+    CountDownLatch blockingLatch = new CountDownLatch(1);
+
+    try {
+      orderedExecutor.execute(
+          () -> {
+            // This will aquire and hold the single max size semaphore permit
+            try {
+              isRunning.countDown();
+              blockingLatch.await();
+            } catch (InterruptedException e) {
+              e.printStackTrace();
+            }
+          });
+
+      isRunning.await(2, TimeUnit.SECONDS);
+
+      // Add another task in a background thread so that we can interrupt it
+      // This _should_ be blocked on the first task because there is only one execution slot
+      CountDownLatch taskTwoFinished = new CountDownLatch(1);
+      Thread t = new Thread(() -> orderedExecutor.execute(2, taskTwoFinished::countDown));
+      t.start();
+      // Interrupt the thread now, but it won't throw until it calls acquire()
+      t.interrupt();
+      // It should complete gracefully from here
+      t.join();
+
+      // Release the first thread
+      assertFalse("Did not expect task #2 to complete", taskTwoFinished.await(0, TimeUnit.SECONDS));
+      blockingLatch.countDown();
+
+      // Tasks without a lock can safely execute again
+      orderedExecutor.execute(() -> {});
+
+      // New threads for lock #2 should be able to execute as well
+      t = new Thread(() -> orderedExecutor.execute(2, () -> {}));
+      t.start();
+
+      // This will also get caught by ThreadLeakControl if it fails
+      t.join(TimeUnit.SECONDS.toMillis(2));
+      assertFalse("Task should have completed", t.isAlive());
+    } finally {
+      orderedExecutor.shutdownAndAwaitTermination();
+    }
+  }
 }


[solr] 04/12: SOLR-16190: Http2SolrClient should make sure to shutdown the executor (#848)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit 319a569c5246c240459283ab741a66a23ca94380
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Wed May 18 08:50:39 2022 -0400

    SOLR-16190: Http2SolrClient should make sure to shutdown the executor (#848)
---
 solr/CHANGES.txt                                      |  2 ++
 .../solr/client/solrj/impl/Http2SolrClient.java       | 19 +++++++++++--------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index f4417863ba9..d5284d83df1 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -16,6 +16,8 @@ Bug Fixes
 
 * SOLR-16399: ExportWriter fails with max values for fields (Kevin Risden)
 
+* SOLR-16190: Http2SolrClient should make sure to shutdown the executor (Kevin Risden)
+
 ==================  9.0.0 ==================
 
 New Features
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
index a12d03c41f7..a10d578a6a3 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
@@ -187,9 +187,9 @@ public class Http2SolrClient extends SolrClient {
   private HttpClient createHttpClient(Builder builder) {
     HttpClient httpClient;
 
-    BlockingArrayQueue<Runnable> queue = new BlockingArrayQueue<>(256, 256);
     executor = builder.executor;
     if (executor == null) {
+      BlockingArrayQueue<Runnable> queue = new BlockingArrayQueue<>(256, 256);
       this.executor =
           new ExecutorUtil.MDCAwareThreadPoolExecutor(
               32, 256, 60, TimeUnit.SECONDS, queue, new SolrNamedThreadFactory("h2sc"));
@@ -241,6 +241,7 @@ public class Http2SolrClient extends SolrClient {
     try {
       httpClient.start();
     } catch (Exception e) {
+      close(); // make sure we clean up
       throw new RuntimeException(e);
     }
 
@@ -250,16 +251,18 @@ public class Http2SolrClient extends SolrClient {
   public void close() {
     // we wait for async requests, so far devs don't want to give sugar for this
     asyncTracker.waitForComplete();
-    if (closeClient) {
-      try {
+    try {
+      if (closeClient) {
         httpClient.setStopTimeout(1000);
         httpClient.stop();
-      } catch (Exception e) {
-        throw new RuntimeException("Exception on closing client", e);
+        httpClient.destroy();
+      }
+    } catch (Exception e) {
+      throw new RuntimeException("Exception on closing client", e);
+    } finally {
+      if (shutdownExecutor) {
+        ExecutorUtil.shutdownAndAwaitTermination(executor);
       }
-    }
-    if (shutdownExecutor) {
-      ExecutorUtil.shutdownAndAwaitTermination(executor);
     }
 
     assert ObjectReleaseTracker.release(this);


[solr] 09/12: SOLR-16182: Close ZkClientClusterStateProvider in tests to avoid thread leak (#837)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit d0412413e6233615119abd05ad531c38981ec554
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Thu May 5 13:14:00 2022 -0400

    SOLR-16182: Close ZkClientClusterStateProvider in tests to avoid thread leak (#837)
---
 .../impl/CloudHttp2SolrClientBuilderTest.java      | 25 +++++++++++-----
 .../CloudHttp2SolrClientMultiConstructorTest.java  |  5 +++-
 .../solrj/impl/CloudHttp2SolrClientTest.java       | 31 ++++++++++---------
 .../solrj/impl/CloudSolrClientBuilderTest.java     | 35 ++++++++++++++--------
 .../impl/CloudSolrClientMultiConstructorTest.java  | 10 +++++--
 .../client/solrj/impl/CloudSolrClientTest.java     | 33 ++++++++++++--------
 6 files changed, 87 insertions(+), 52 deletions(-)

diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java
index dd5b8a96df4..85062638463 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientBuilderTest.java
@@ -43,9 +43,12 @@ public class CloudHttp2SolrClientBuilderTest extends SolrTestCase {
         new CloudHttp2SolrClient.Builder(
                 Collections.singletonList(ANY_ZK_HOST), Optional.of(ANY_CHROOT))
             .build()) {
-      final String clientZkHost = ZkClientClusterStateProvider.from(createdClient).getZkHost();
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(createdClient)) {
+        final String clientZkHost = zkClientClusterStateProvider.getZkHost();
 
-      assertTrue(clientZkHost.contains(ANY_ZK_HOST));
+        assertTrue(clientZkHost.contains(ANY_ZK_HOST));
+      }
     }
   }
 
@@ -56,10 +59,13 @@ public class CloudHttp2SolrClientBuilderTest extends SolrTestCase {
     zkHostList.add(ANY_OTHER_ZK_HOST);
     try (CloudHttp2SolrClient createdClient =
         new CloudHttp2SolrClient.Builder(zkHostList, Optional.of(ANY_CHROOT)).build()) {
-      final String clientZkHost = ZkClientClusterStateProvider.from(createdClient).getZkHost();
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(createdClient)) {
+        final String clientZkHost = zkClientClusterStateProvider.getZkHost();
 
-      assertTrue(clientZkHost.contains(ANY_ZK_HOST));
-      assertTrue(clientZkHost.contains(ANY_OTHER_ZK_HOST));
+        assertTrue(clientZkHost.contains(ANY_ZK_HOST));
+        assertTrue(clientZkHost.contains(ANY_OTHER_ZK_HOST));
+      }
     }
   }
 
@@ -70,10 +76,13 @@ public class CloudHttp2SolrClientBuilderTest extends SolrTestCase {
     zkHosts.add(ANY_OTHER_ZK_HOST);
     try (CloudHttp2SolrClient createdClient =
         new CloudHttp2SolrClient.Builder(zkHosts, Optional.of(ANY_CHROOT)).build()) {
-      final String clientZkHost = ZkClientClusterStateProvider.from(createdClient).getZkHost();
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(createdClient)) {
+        final String clientZkHost = zkClientClusterStateProvider.getZkHost();
 
-      assertTrue(clientZkHost.contains(ANY_ZK_HOST));
-      assertTrue(clientZkHost.contains(ANY_OTHER_ZK_HOST));
+        assertTrue(clientZkHost.contains(ANY_ZK_HOST));
+        assertTrue(clientZkHost.contains(ANY_OTHER_ZK_HOST));
+      }
     }
   }
 
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientMultiConstructorTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientMultiConstructorTest.java
index 5ee6acc2f35..2eb6b4a00a6 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientMultiConstructorTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientMultiConstructorTest.java
@@ -71,7 +71,10 @@ public class CloudHttp2SolrClientMultiConstructorTest extends SolrTestCase {
     try (CloudHttp2SolrClient client =
         new CloudHttp2SolrClient.Builder(new ArrayList<>(hosts), Optional.ofNullable(clientChroot))
             .build()) {
-      assertEquals(sb.toString(), ZkClientClusterStateProvider.from(client).getZkHost());
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(client)) {
+        assertEquals(sb.toString(), zkClientClusterStateProvider.getZkHost());
+      }
     }
   }
 
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java
index e54f3e8d953..cd023603aa6 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java
@@ -75,9 +75,7 @@ import org.apache.solr.handler.admin.CoreAdminHandler;
 import org.junit.After;
 import org.junit.AfterClass;
 import org.junit.Before;
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -824,27 +822,28 @@ public class CloudHttp2SolrClientTest extends SolrCloudTestCase {
   @Test
   public void testShutdown() throws IOException {
     try (CloudSolrClient client = getCloudSolrClient(DEAD_HOST_1)) {
-      ZkClientClusterStateProvider.from(client).setZkConnectTimeout(100);
-      client.connect();
-      fail("Expected exception");
-    } catch (SolrException e) {
-      assertTrue(e.getCause() instanceof TimeoutException);
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(client)) {
+        zkClientClusterStateProvider.setZkConnectTimeout(100);
+        SolrException e = assertThrows(SolrException.class, client::connect);
+        assertTrue(e.getCause() instanceof TimeoutException);
+      }
     }
   }
 
-  @Rule public ExpectedException exception = ExpectedException.none();
-
   @Test
   public void testWrongZkChrootTest() throws IOException {
-    exception.expect(SolrException.class);
-    exception.expectMessage("cluster not found/not ready");
-    exception.expectMessage("Expected node '" + ZkStateReader.ALIASES + "' does not exist");
-
     try (CloudSolrClient client =
         getCloudSolrClient(cluster.getZkServer().getZkAddress() + "/xyz/foo")) {
-      ZkClientClusterStateProvider.from(client).setZkClientTimeout(1000 * 60);
-      client.connect();
-      fail("Expected exception");
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(client)) {
+        zkClientClusterStateProvider.setZkClientTimeout(1000 * 60);
+        SolrException e = assertThrows(SolrException.class, client::connect);
+        assertTrue(e.getMessage().contains("cluster not found/not ready"));
+        assertTrue(
+            e.getMessage()
+                .contains("Expected node '" + ZkStateReader.ALIASES + "' does not exist"));
+      }
     }
   }
 
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientBuilderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientBuilderTest.java
index 60e74ff0ee3..d4d90ae211e 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientBuilderTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientBuilderTest.java
@@ -34,9 +34,12 @@ public class CloudSolrClientBuilderTest extends SolrTestCase {
   @Test
   public void testSingleZkHostSpecified() throws IOException {
     try (CloudSolrClient createdClient =
-        new Builder(Collections.singletonList(ANY_ZK_HOST), Optional.of(ANY_CHROOT)).build()) {
-      final String clientZkHost = ZkClientClusterStateProvider.from(createdClient).getZkHost();
-      assertTrue(clientZkHost.contains(ANY_ZK_HOST));
+        new Builder(Collections.singletonList(ANY_ZK_HOST), Optional.of(ANY_CHROOT)).build(); ) {
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(createdClient)) {
+        final String clientZkHost = zkClientClusterStateProvider.getZkHost();
+        assertTrue(clientZkHost.contains(ANY_ZK_HOST));
+      }
     }
   }
 
@@ -46,9 +49,12 @@ public class CloudSolrClientBuilderTest extends SolrTestCase {
     zkHostList.add(ANY_ZK_HOST);
     zkHostList.add(ANY_OTHER_ZK_HOST);
     try (CloudSolrClient createdClient = new Builder(zkHostList, Optional.of(ANY_CHROOT)).build()) {
-      final String clientZkHost = ZkClientClusterStateProvider.from(createdClient).getZkHost();
-      assertTrue(clientZkHost.contains(ANY_ZK_HOST));
-      assertTrue(clientZkHost.contains(ANY_OTHER_ZK_HOST));
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(createdClient)) {
+        final String clientZkHost = zkClientClusterStateProvider.getZkHost();
+        assertTrue(clientZkHost.contains(ANY_ZK_HOST));
+        assertTrue(clientZkHost.contains(ANY_OTHER_ZK_HOST));
+      }
     }
   }
 
@@ -58,9 +64,12 @@ public class CloudSolrClientBuilderTest extends SolrTestCase {
     zkHosts.add(ANY_ZK_HOST);
     zkHosts.add(ANY_OTHER_ZK_HOST);
     try (CloudSolrClient createdClient = new Builder(zkHosts, Optional.of(ANY_CHROOT)).build()) {
-      final String clientZkHost = ZkClientClusterStateProvider.from(createdClient).getZkHost();
-      assertTrue(clientZkHost.contains(ANY_ZK_HOST));
-      assertTrue(clientZkHost.contains(ANY_OTHER_ZK_HOST));
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(createdClient)) {
+        final String clientZkHost = zkClientClusterStateProvider.getZkHost();
+        assertTrue(clientZkHost.contains(ANY_ZK_HOST));
+        assertTrue(clientZkHost.contains(ANY_OTHER_ZK_HOST));
+      }
     }
   }
 
@@ -68,7 +77,7 @@ public class CloudSolrClientBuilderTest extends SolrTestCase {
   public void testByDefaultConfiguresClientToSendUpdatesOnlyToShardLeaders() throws IOException {
     try (CloudSolrClient createdClient =
         new Builder(Collections.singletonList(ANY_ZK_HOST), Optional.of(ANY_CHROOT)).build()) {
-      assertTrue(createdClient.isUpdatesToLeaders() == true);
+      assertTrue(createdClient.isUpdatesToLeaders());
     }
   }
 
@@ -83,10 +92,12 @@ public class CloudSolrClientBuilderTest extends SolrTestCase {
   @Test
   @SuppressWarnings({"try"})
   public void test0Timeouts() throws IOException {
-    try (var createdClient =
+    try (CloudSolrClient createdClient =
         new CloudLegacySolrClient.Builder(Collections.singletonList(ANY_ZK_HOST), Optional.empty())
             .withSocketTimeout(0)
             .withConnectionTimeout(0)
-            .build()) {}
+            .build()) {
+      assertNotNull(createdClient);
+    }
   }
 }
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientMultiConstructorTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientMultiConstructorTest.java
index 37abe2021a8..43a6cce0a44 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientMultiConstructorTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientMultiConstructorTest.java
@@ -71,7 +71,10 @@ public class CloudSolrClientMultiConstructorTest extends SolrTestCase {
     try (CloudSolrClient client =
         (new CloudSolrClient.Builder(new ArrayList<>(hosts), Optional.ofNullable(clientChroot))
             .build())) {
-      assertEquals(sb.toString(), ZkClientClusterStateProvider.from(client).getZkHost());
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(client)) {
+        assertEquals(sb.toString(), zkClientClusterStateProvider.getZkHost());
+      }
     }
   }
 
@@ -99,7 +102,10 @@ public class CloudSolrClientMultiConstructorTest extends SolrTestCase {
     final Optional<String> chrootOption =
         withChroot == false ? Optional.empty() : Optional.of(chroot);
     try (var client = new CloudLegacySolrClient.Builder(hosts, chrootOption).build()) {
-      assertEquals(sb.toString(), ZkClientClusterStateProvider.from(client).getZkHost());
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(client)) {
+        assertEquals(sb.toString(), zkClientClusterStateProvider.getZkHost());
+      }
     }
   }
 
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
index d0318b95ccb..c4036a42c74 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
@@ -826,9 +826,12 @@ public class CloudSolrClientTest extends SolrCloudTestCase {
   @Test
   public void testShutdown() throws IOException {
     try (CloudSolrClient client = getCloudSolrClient(DEAD_HOST_1)) {
-      ZkClientClusterStateProvider.from(client).setZkConnectTimeout(100);
-      SolrException ex = expectThrows(SolrException.class, client::connect);
-      assertTrue(ex.getCause() instanceof TimeoutException);
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(client)) {
+        zkClientClusterStateProvider.setZkConnectTimeout(100);
+        SolrException ex = expectThrows(SolrException.class, client::connect);
+        assertTrue(ex.getCause() instanceof TimeoutException);
+      }
     }
   }
 
@@ -838,16 +841,20 @@ public class CloudSolrClientTest extends SolrCloudTestCase {
   public void testWrongZkChrootTest() throws IOException {
     try (CloudSolrClient client =
         getCloudSolrClient(cluster.getZkServer().getZkAddress() + "/xyz/foo")) {
-      ZkClientClusterStateProvider.from(client).setZkConnectTimeout(1000 * 60);
-      SolrException ex = expectThrows(SolrException.class, client::connect);
-      MatcherAssert.assertThat(
-          "Wrong error message for empty chRoot",
-          ex.getMessage(),
-          Matchers.containsString("cluster not found/not ready"));
-      MatcherAssert.assertThat(
-          "Wrong node missing message for empty chRoot",
-          ex.getMessage(),
-          Matchers.containsString("Expected node '" + ZkStateReader.ALIASES + "' does not exist"));
+      try (ZkClientClusterStateProvider zkClientClusterStateProvider =
+          ZkClientClusterStateProvider.from(client)) {
+        zkClientClusterStateProvider.setZkConnectTimeout(1000 * 60);
+        SolrException ex = expectThrows(SolrException.class, client::connect);
+        MatcherAssert.assertThat(
+            "Wrong error message for empty chRoot",
+            ex.getMessage(),
+            Matchers.containsString("cluster not found/not ready"));
+        MatcherAssert.assertThat(
+            "Wrong node missing message for empty chRoot",
+            ex.getMessage(),
+            Matchers.containsString(
+                "Expected node '" + ZkStateReader.ALIASES + "' does not exist"));
+      }
     }
   }
 


[solr] 08/12: SOLR-16185: CloudSolrClientCacheTest leaks threads (#839)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit e4a622d0307d8730fa77d7108d8fa0926bae92a5
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Fri May 6 12:51:25 2022 -0400

    SOLR-16185: CloudSolrClientCacheTest leaks threads (#839)
---
 .../solr/client/solrj/impl/CloudSolrClient.java     |  5 +++--
 .../client/solrj/impl/CloudSolrClientCacheTest.java | 21 ++++++++++++++-------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java
index fdd3c615d80..6ae24ec85bf 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java
@@ -99,7 +99,7 @@ public abstract class CloudSolrClient extends SolrClient {
   // no of times collection state to be reloaded if stale state error is received
   private static final int MAX_STALE_RETRIES =
       Integer.parseInt(System.getProperty("cloudSolrClientMaxStaleRetries", "5"));
-  private Random rand = new Random();
+  private final Random rand = new Random();
 
   private final boolean updatesToLeaders;
   private final boolean directUpdatesToLeadersOnly;
@@ -304,7 +304,8 @@ public abstract class CloudSolrClient extends SolrClient {
   @Override
   public void close() throws IOException {
     if (this.threadPool != null && !this.threadPool.isShutdown()) {
-      this.threadPool.shutdown();
+      ExecutorUtil.shutdownAndAwaitTermination(this.threadPool);
+      this.threadPool = null;
     }
   }
 
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java
index 4c9bebf369f..35c404d33a3 100644
--- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java
+++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java
@@ -78,10 +78,11 @@ public class CloudSolrClientCacheTest extends SolrTestCaseJ4 {
 
     LBHttpSolrClient mockLbclient = getMockLbHttpSolrClient(responses);
     AtomicInteger lbhttpRequestCount = new AtomicInteger();
-    try (CloudSolrClient cloudClient =
-        new CloudSolrClientBuilder(getStateProvider(livenodes, refs))
-            .withLBHttpSolrClient(mockLbclient)
-            .build()) {
+    try (ClusterStateProvider clusterStateProvider = getStateProvider(livenodes, refs);
+        CloudSolrClient cloudClient =
+            new CloudSolrClientBuilder(clusterStateProvider)
+                .withLBHttpSolrClient(mockLbclient)
+                .build()) {
       livenodes.addAll(ImmutableSet.of("192.168.1.108:7574_solr", "192.168.1.108:8983_solr"));
       ClusterState cs =
           ClusterState.createFromJson(1, coll1State.getBytes(UTF_8), Collections.emptySet());
@@ -91,9 +92,15 @@ public class CloudSolrClientCacheTest extends SolrTestCaseJ4 {
           "request",
           o -> {
             int i = lbhttpRequestCount.incrementAndGet();
-            if (i == 1) return new ConnectException("TEST");
-            if (i == 2) return new SocketException("TEST");
-            if (i == 3) return new NoHttpResponseException("TEST");
+            if (i == 1) {
+              return new ConnectException("TEST");
+            }
+            if (i == 2) {
+              return new SocketException("TEST");
+            }
+            if (i == 3) {
+              return new NoHttpResponseException("TEST");
+            }
             return okResponse;
           });
       UpdateRequest update = new UpdateRequest().add("id", "123", "desc", "Something 0");


[solr] 02/12: SOLR-16401: Test thread leak linger should be 1s (#997)

Posted by kr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

krisden pushed a commit to branch branch_9_0
in repository https://gitbox.apache.org/repos/asf/solr.git

commit e771ae5b4c8094e5ed5dc354bbcbdc811c0fa844
Author: Kevin Risden <ri...@users.noreply.github.com>
AuthorDate: Tue Sep 6 11:40:31 2022 -0400

    SOLR-16401: Test thread leak linger should be 1s (#997)
---
 solr/test-framework/src/java/org/apache/solr/SolrTestCase.java | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/solr/test-framework/src/java/org/apache/solr/SolrTestCase.java b/solr/test-framework/src/java/org/apache/solr/SolrTestCase.java
index 14d05a97c29..5a891e2c052 100644
--- a/solr/test-framework/src/java/org/apache/solr/SolrTestCase.java
+++ b/solr/test-framework/src/java/org/apache/solr/SolrTestCase.java
@@ -59,7 +59,11 @@ import org.slf4j.LoggerFactory;
 @ThreadLeakFilters(
     defaultFilters = true,
     filters = {SolrIgnoredThreadsFilter.class, QuickPatchThreadsFilter.class})
-@ThreadLeakLingering(linger = 0)
+// The ThreadLeakLingering is set to 1s to allow ThreadPools to finish
+// joining on termination. Ideally this should only be 10-100ms, but
+// on slow machines it could take up to 1s. See discussion on SOLR-15660
+// and SOLR-16187 regarding why this is necessary.
+@ThreadLeakLingering(linger = 1000)
 public class SolrTestCase extends LuceneTestCase {
 
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());