[GitHub] [solr] dsmiley commented on a diff in pull request #1149: SOLR-16498: Tests to retrieve url from SolrClient

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #1149:
URL: https://github.com/apache/solr/pull/1149#discussion_r1033551745


##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -92,7 +90,7 @@
  *
  * @since 1.4
  */
-@Nightly
+// @Nightly

Review Comment:
   Not on purpose?



##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -261,23 +259,22 @@ private NamedList<Object> getIndexVersion(SolrClient s) throws Exception {
     return res;
   }
 
-  private NamedList<Object> reloadCore(SolrClient s, String core) throws Exception {
+  private void reloadCore(JettySolrRunner jettySolrRunner, String core) throws Exception {
 
     ModifiableSolrParams params = new ModifiableSolrParams();
     params.set("action", "reload");
     params.set("core", core);
     params.set("qt", "/admin/cores");
     QueryRequest req = new QueryRequest(params);
 
-    try (SolrClient adminClient = adminClient(s)) {
+    try (SolrClient adminClient = adminClient(jettySolrRunner)) {
       NamedList<Object> res = adminClient.request(req);
       assertNotNull("null response from server", res);
-      return res;
     }
   }
 
-  private SolrClient adminClient(SolrClient client) {
-    String adminUrl = ((HttpSolrClient) client).getBaseURL().replace("/collection1", "");
+  private SolrClient adminClient(JettySolrRunner client) {
+    String adminUrl = client.getBaseUrl().toString().replace("/collection1", "");
     return getHttpSolrClient(adminUrl);
   }

Review Comment:
   This method demonstrates a problem that has bothered me with Solr's Http clients for a long time.  The user can construct an HttpSolrClient with a URL that includes the collection, but HttpSolrClient doesn't "know" this.  This has consequences, such as the existence of this method and issues elsewhere.  Ideally, the "base URL" would be provided and optionally a default collection/core name.  Then, here, we wouldn't need another SolrClient instance simply because we want to issue admin commands.  In a test setting, we could even avoid Jetty/HTTP.  Granted, addressing that is definitely out of scope.  
   
   For now, without trying to fix all imperfections at once, the change here is fine.



##########
solr/core/src/test/org/apache/solr/servlet/CacheHeaderTest.java:
##########
@@ -59,7 +59,7 @@ public void testCacheVetoHandler() throws Exception {
             f.getCanonicalPath(),
             CommonParams.STREAM_CONTENTTYPE,
             "text/csv");
-    HttpResponse response = getClient().execute(m);
+    HttpResponse response = getHttpClient().execute(m);

Review Comment:
   normally I wouldn't like the direction of this -- generic to HTTP specific, but here, clearly the test is related to HTTP.



##########
solr/core/src/test/org/apache/solr/handler/TestReplicationHandler.java:
##########
@@ -261,23 +259,22 @@ private NamedList<Object> getIndexVersion(SolrClient s) throws Exception {
     return res;
   }
 
-  private NamedList<Object> reloadCore(SolrClient s, String core) throws Exception {
+  private void reloadCore(JettySolrRunner jettySolrRunner, String core) throws Exception {

Review Comment:
   Previously we needed a SolrClient (thus could even be EmbeddedSolrServer in theory) and now we require Jetty, an HTTP server.  Since this test is private, and I'm merely observing from the diff, maybe this is moot since it's a test that presumably is already using Jetty so there is no practical difference.  But I'd prefer we not insist on Jetty unless we actually need to work with HTTP for some reason.



##########
solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java:
##########
@@ -1489,6 +1489,12 @@ private String getBaseUrl(String id) {
     return slice.getLeader().getCoreUrl();
   }
 
+  protected String getBaseUrl(HttpSolrClient client) {
+    return client
+        .getBaseURL()
+        .substring(0, client.getBaseURL().length() - DEFAULT_COLLECTION.length() - 1);
+  }

Review Comment:
   add a comment to say that we assume that client's URL ends with `/collection1`.
   But you still have client.getBaseURL which we're trying to get rid of.  So...



##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractSyncSliceTestBase.java:
##########
@@ -102,9 +101,7 @@ public void test() throws Exception {
     QueryRequest request = new QueryRequest(params);
     request.setPath("/admin/collections");
 
-    String baseUrl =
-        ((HttpSolrClient) shardToJetty.get("shard1").get(2).client.solrClient).getBaseURL();
-    baseUrl = baseUrl.substring(0, baseUrl.length() - "collection1".length());
+    String baseUrl = shardToJetty.get(SHARD1).get(2).jetty.getBaseUrl().toString();

Review Comment:
   much nicer



-- 
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