dsmiley commented on code in PR #2417:
URL: https://github.com/apache/solr/pull/2417#discussion_r1583658873
##########
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:
I appreciate it's frustrating given you've consulted with others already.
It's awkward for me because here I am commenting on this PR and the other
committers you speak of are not "here" and that it was one or two years ago,
and the scope is confusing (just CLI or also must address server). Hey
@gerlowskija you're one of them. My concern is adding ClassLoader setters in
places that feel out-of-place, especially CloudSolrClient classes which we
should highly scrutinize.
This PR is just for client CLI tools. Couldn't the CLI create a
ClassLoader, add the module JARs, and then go load CLI tool with this loader
that can in do whatever?
Server: Ideally the server should only instantiate one CloudSolrClient for
the current cluster, and when it does so it can provide its own ZkStateReader,
which we can assume already has the proper SolrZkClient loaded with the right
class path. The Thread.currentThread().getContextClassLoader() can be used to
initialize the classLoader field in SolrZkClient. CoreContainer will set the
Thread ContextClassLoader when it calls load().
--
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
