epugh commented on code in PR #2417:
URL: https://github.com/apache/solr/pull/2417#discussion_r1576047123
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -28,6 +28,7 @@
import org.apache.solr.client.solrj.request.RequestWriter;
import org.apache.solr.client.solrj.request.UpdateRequest;
import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.SolrClassLoader;
Review Comment:
Do we need any additional testing around this on the `CloudHttp2SolrClient`,
`CloudLegacySolrClient`? You suggested they are covered already, just wanted
to confirm.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java:
##########
@@ -25,20 +25,21 @@
import org.apache.solr.common.SolrCloseable;
import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.SolrClassLoader;
import org.apache.solr.common.params.CollectionAdminParams;
/** Provides cluster state from some source */
public interface ClusterStateProvider extends SolrCloseable {
- static ClusterStateProvider newZkClusterStateProvider(
- Collection<String> zkHosts, String zkChroot, boolean canUseZkACLs) {
+ static ClusterStateProvider newZkClusterStateProvider(Collection<String>
zkHosts, String zkChroot, boolean canUseZkACLs,
+ SolrClassLoader solrClassLoader) {
// instantiate via reflection so that we don't depend on ZK
try {
var constructor =
Class.forName("org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider")
.asSubclass(ClusterStateProvider.class)
- .getConstructor(Collection.class, String.class, Boolean.TYPE);
- return constructor.newInstance(zkHosts, zkChroot, canUseZkACLs);
+ .getConstructor(Collection.class, String.class, Boolean.TYPE,
SolrClassLoader.class);
+ return constructor.newInstance(zkHosts, zkChroot, canUseZkACLs,
solrClassLoader);
Review Comment:
I wonder if a big of javadocs on this method would help expliain what the
magic is and shy we are pssing in solrClassLoader?
##########
solr/core/src/java/org/apache/solr/cli/SolrCLI.java:
##########
@@ -607,6 +611,46 @@ public static String getZkHost(CommandLine cli) throws
Exception {
return zkHost;
}
+ public static SolrZkClient getSolrZkClient(CommandLine cli) throws Exception
{
+ return getSolrZkClient(cli, getZkHost(cli));
+ }
+
+ public static SolrZkClient getSolrZkClient(CommandLine cli, String zkHost)
throws Exception {
+ if (zkHost == null) {
+ throw new IllegalStateException(
+ "Solr at "
+ + cli.getOptionValue("solrUrl")
+ + " is running in standalone server mode, this command can only
be used when running in SolrCloud mode.\n");
+ }
+ return new SolrZkClient.Builder()
+ .withUrl(zkHost)
+ .withTimeout(SolrZkClientTimeout.DEFAULT_ZK_CLIENT_TIMEOUT,
TimeUnit.MILLISECONDS)
+ .withSolrClassLoader(getSolrResourceLoader())
+ .build();
+ }
+
+ public static CloudHttp2SolrClient getCloudHttp2SolrClient(String zkHost) {
+ return getCloudHttp2SolrClient(zkHost, null);
+ }
+
+ public static CloudHttp2SolrClient getCloudHttp2SolrClient(
+ String zkHost, Http2SolrClient.Builder builder) {
+ return new CloudHttp2SolrClient.Builder(Collections.singletonList(zkHost),
Optional.empty())
+ .withSolrClassLoader(getSolrResourceLoader())
+ .withInternalClientBuilder(builder)
+ .build();
+ }
+
+ private static SolrResourceLoader getSolrResourceLoader() {
+ String dir = System.getProperty("solr.solr.home");
+ if (StrUtils.isNullOrEmpty(dir)) {
+ dir = System.getProperty("solr.install.dir");
+ }
+ final SolrResourceLoader loader = new SolrResourceLoader(Paths.get(dir));
+ NodeConfig.initModules(loader, null);
Review Comment:
This NodeConfig is kind of jarring to me.. I'm reading htorugh the code, it
all makes sense the methods and then "hey, there is a Nodeconfig! What the
heck is a node config??!!".. Maybe just some javadocs/docs on why we have
this.
##########
solr/core/src/java/org/apache/solr/cli/ZkMvTool.java:
##########
@@ -69,18 +67,8 @@ public String getName() {
public void runImpl(CommandLine cli) throws Exception {
SolrCLI.raiseLogLevelUnlessVerbose(cli);
String zkHost = SolrCLI.getZkHost(cli);
- if (zkHost == null) {
Review Comment:
we are losing this special case messaging, however I think I am totally fine
with that ;-). Since I want to get rid of standalone mode anyway.
##########
solr/core/src/test/org/apache/solr/cloud/SolrCLIZkUtilsTest.java:
##########
@@ -69,6 +70,7 @@ public static void closeConn() {
zkClient = null;
}
zkAddr = null;
+ System.clearProperty("solr.solr.home");
Review Comment:
isn't there a new / better way of handling system properties in tests?
@dsmiley ? If we are fixing this, maybe use the proper technique?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
