[GitHub] [solr] gerlowskija commented on a diff in pull request #1508: SOLR-16687: Add a SolrClassLoader to SolrZkClient

["gerlowskija (via GitHub)" <gi...@apache.org>]

gerlowskija commented on code in PR #1508:
URL: https://github.com/apache/solr/pull/1508#discussion_r1194268355


##########
solr/solrj-zookeeper/src/test/org/apache/solr/common/cloud/SolrZkClientTest.java:
##########
@@ -281,4 +282,42 @@ public void testCheckInterrupted() {
     SolrZkClient.checkInterrupted(new InterruptedException());
     assertTrue(Thread.currentThread().isInterrupted());
   }
+
+  @Test
+  public void testWithSolrResourceLoader() {
+    setupSystemProperties();
+    SolrResourceLoader solrResourceLoader =
+        new SolrResourceLoader(Path.of("."), ClassLoader.getSystemClassLoader());
+    new SolrZkClient.Builder()
+        .withUrl(zkServer.getZkHost())
+        .withTimeout(AbstractZkTestCase.TIMEOUT, TimeUnit.MILLISECONDS)
+        .withSolrClassLoader(solrResourceLoader)
+        .build()
+        .close(); // no more tests needed. We only test class instantiation
+    clearSystemProperties();
+  }
+
+  private static void setupSystemProperties() {

Review Comment:
   [0] Small nit: this might benefit from a name that describes what sysprops you're setting up. e.g. `enableCustomCredentialsProvider` or something similar.
   
   But if you like that name as-is, that's also fine.



##########
solr/solrj-zookeeper/src/test/org/apache/solr/common/cloud/SolrZkClientTest.java:
##########
@@ -281,4 +282,42 @@ public void testCheckInterrupted() {
     SolrZkClient.checkInterrupted(new InterruptedException());
     assertTrue(Thread.currentThread().isInterrupted());
   }
+
+  @Test
+  public void testWithSolrResourceLoader() {
+    setupSystemProperties();
+    SolrResourceLoader solrResourceLoader =
+        new SolrResourceLoader(Path.of("."), ClassLoader.getSystemClassLoader());
+    new SolrZkClient.Builder()
+        .withUrl(zkServer.getZkHost())
+        .withTimeout(AbstractZkTestCase.TIMEOUT, TimeUnit.MILLISECONDS)
+        .withSolrClassLoader(solrResourceLoader)
+        .build()
+        .close(); // no more tests needed. We only test class instantiation
+    clearSystemProperties();

Review Comment:
   [Q] Do you know whether these system properties actually need cleared?  Solr tests all run with a JUnit '@Rule' (called "SystemPropertiesRestoreRule") that is supposed to clear them in between each test case.
   
   If that's not working, I can file a separate ticket to fix that.  But if it is, we should remove redundant sysprop removal here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org