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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
