dsmiley commented on code in PR #4214:
URL: https://github.com/apache/solr/pull/4214#discussion_r2933028497
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -938,13 +996,19 @@ public void waitForJettyToStop(JettySolrRunner runner)
throws TimeoutException {
}
}
- public void dumpMetrics(Path outputDirectory, String fileName) throws
IOException {
- for (JettySolrRunner jetty : jettys) {
- jetty.outputMetrics(outputDirectory, fileName);
+ @Override
+ public void dumpMetrics(PrintStream out) {
+ try {
+ for (JettySolrRunner jetty : jettys) {
+ jetty.outputMetrics(out);
Review Comment:
perhaps should add a banner comment (it's prometheus). The change to
dumpMetrics is that there is ultimately one stream/file whereas previously it
was multiple files in this directory where each file name was a combination of
the passed in fileName and also a netty node identifier
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -90,7 +93,7 @@
import org.slf4j.LoggerFactory;
/** "Mini" SolrCloud cluster to be used for testing */
-public class MiniSolrCloudCluster {
+public class MiniSolrCloudCluster implements SolrBackend {
Review Comment:
it's a pretty natural fit to implement directly here
##########
solr/test-framework/src/java/org/apache/solr/cloud/RemoteSolrBackend.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+import org.apache.solr.SolrBackend;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.ConfigSetAdminRequest;
+import org.apache.solr.common.util.IOUtils;
+import org.apache.solr.core.CoreContainer;
+
+/**
+ * {@link SolrBackend} that connects to a pre-existing remote SolrCloud
cluster. The caller supplies
+ * the ZooKeeper connection string at construction time (e.g. {@code
localhost:9983/solr}).
+ *
+ * <p>Can be subclassed to add a startup step (e.g. launching a Docker
container) before
+ * constructing the client.
+ */
+public class RemoteSolrBackend implements SolrBackend {
+
+ private final CloudSolrClient adminClient;
+
+ public RemoteSolrBackend(String baseUrl) {
+ this.adminClient = new CloudSolrClient.Builder(List.of(baseUrl)).build();
+ }
+
+ @Override
+ public CoreContainer getCoreContainer() {
+ return null;
+ }
+
+ @Override
+ public SolrClient newClient(String collection) {
+ return new CloudSolrClient.Builder(
+ List.of(adminClient.getClusterStateProvider().getQuorumHosts()))
Review Comment:
I suspect this may file if quorum hosts aren't qualified with the urlScheme
##########
solr/test-framework/src/java/org/apache/solr/embedded/EmbeddedSolrBackend.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.embedded;
+
+import java.nio.file.Path;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import org.apache.solr.SolrBackend;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.util.IOUtils;
+import org.apache.solr.core.CoreContainer;
+
+/**
+ * {@link SolrBackend} backed by an in-process {@link CoreContainer}/{@link
EmbeddedSolrServer}. No
+ * network or ZooKeeper overhead, although there's some request/response
serialization.
+ *
+ * <p>Data is persisted in the given {@code solrHome} directory.
+ */
+public class EmbeddedSolrBackend implements SolrBackend {
Review Comment:
EmbeddedSolrServer cannot implement SolrBackend directly because the latter
is in solr-core and SolrBackend is in test-framework
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -103,7 +105,7 @@
*
* @since solr 1.3
*/
-public class JettySolrRunner {
+public class JettySolrRunner implements SolrBackend {
Review Comment:
this ended up being slightly awkward to implement directly. An alternative
could be a new method `asSolrBackend`
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -725,6 +728,61 @@ public CloudSolrClient getSolrClient(String
collectionName) {
});
}
+ // SolrBackend interface methods
+
+ @Override
+ public SolrClient newClient(String collection) {
+ return new
CloudSolrClient.Builder(getSolrClient().getClusterStateProvider())
+ .withDefaultCollection(collection)
+ .build();
+ }
+
+ @Override
+ public SolrClient getAdminClient() {
+ return getSolrClient();
+ }
+
+ @Override
+ public void createCollection(CollectionAdminRequest.Create create)
+ throws SolrServerException, IOException {
+ String collectionName = create.getCollectionName();
+ create.process(getAdminClient());
+ int shards = create.getNumShards() != null ? create.getNumShards() : 1;
+ int replicas = create.getReplicationFactor() != null ?
create.getReplicationFactor() : 1;
+ waitForActiveCollection(collectionName, 15, TimeUnit.SECONDS, shards,
shards * replicas);
+ }
+
+ @Override
+ public boolean hasCollection(String name) {
+ return getZkStateReader().getClusterState().hasCollection(name);
+ }
+
+ @Override
+ public boolean hasConfigSet(String name) throws IOException {
+ try (SolrZkClient zkClient =
+ new SolrZkClient.Builder()
+ .withUrl(zkServer.getZkAddress())
+ .withTimeout(AbstractZkTestCase.TIMEOUT, TimeUnit.MILLISECONDS)
+ .withConnTimeOut(AbstractZkTestCase.TIMEOUT, TimeUnit.MILLISECONDS)
+ .build()) {
+ return new ZkConfigSetService(zkClient).checkConfigExists(name);
+ }
+ }
+
+ @Override
+ public CoreContainer getCoreContainer() {
+ return jettys.isEmpty() ? null : jettys.get(0).getCoreContainer();
+ }
+
+ @Override
+ public void close() {
+ try {
+ shutdown();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
Review Comment:
hmm; or we just log?
##########
solr/test-framework/src/java/org/apache/solr/cloud/RemoteSolrBackend.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+import org.apache.solr.SolrBackend;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.ConfigSetAdminRequest;
+import org.apache.solr.common.util.IOUtils;
+import org.apache.solr.core.CoreContainer;
+
+/**
+ * {@link SolrBackend} that connects to a pre-existing remote SolrCloud
cluster. The caller supplies
+ * the ZooKeeper connection string at construction time (e.g. {@code
localhost:9983/solr}).
+ *
+ * <p>Can be subclassed to add a startup step (e.g. launching a Docker
container) before
Review Comment:
I should remove this; out of scope and I suspect changes will be needed for
this to be a practical reality
##########
solr/test-framework/src/java/org/apache/solr/cloud/RemoteSolrBackend.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud;
Review Comment:
put into cloud package as it assumes SolrCloud
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -725,6 +728,61 @@ public CloudSolrClient getSolrClient(String
collectionName) {
});
}
+ // SolrBackend interface methods
+
+ @Override
+ public SolrClient newClient(String collection) {
+ return new
CloudSolrClient.Builder(getSolrClient().getClusterStateProvider())
+ .withDefaultCollection(collection)
+ .build();
+ }
+
+ @Override
+ public SolrClient getAdminClient() {
Review Comment:
my "nocommit" on SolrBackend.getAdminClient naming is partially motivated by
seeing this here. It'd be nice to simply call it `getSolrClient`.
##########
solr/test-framework/src/java/org/apache/solr/cloud/RemoteSolrBackend.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+import org.apache.solr.SolrBackend;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.ConfigSetAdminRequest;
+import org.apache.solr.common.util.IOUtils;
+import org.apache.solr.core.CoreContainer;
+
+/**
+ * {@link SolrBackend} that connects to a pre-existing remote SolrCloud
cluster. The caller supplies
+ * the ZooKeeper connection string at construction time (e.g. {@code
localhost:9983/solr}).
Review Comment:
oops; false; it's an HTTP URL
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -917,4 +916,49 @@ public Delay(String reason, int counter, int delay) {
public SocketProxy getProxy() {
return proxy;
}
+
+ // ---- SolrBackend implementation ----
+
+ @Override
+ public SolrClient newClient(String collection) {
+ return new HttpJettySolrClient.Builder(getBaseUrl().toString())
+ .withDefaultCollection(collection)
+ .build();
+ }
+
+ @Override
+ public SolrClient getAdminClient() {
+ return backendAdminClient;
+ }
+
+ @Override
+ public void createCollection(CollectionAdminRequest.Create create) {
+ new EmbeddedSolrBackend(backendAdminClient).createCollection(create);
+ }
+
+ @Override
+ public boolean hasCollection(String name) {
+ return new EmbeddedSolrBackend(backendAdminClient).hasCollection(name);
+ }
+
+ @Override
+ public void reloadCollection(String name) throws SolrServerException,
IOException {
+ new EmbeddedSolrBackend(backendAdminClient).reloadCollection(name);
+ }
+
+ @Override
+ public String getBaseUrl(Random r) {
+ return getBaseUrl().toString();
+ }
+
+ @Override
+ public void close() {
+ IOUtils.closeQuietly(backendAdminClient);
+ backendAdminClient = null;
+ try {
+ stop();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
Review Comment:
again; should probably log & not throw
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -722,29 +732,18 @@ public synchronized void stop() throws Exception {
}
}
- public void outputMetrics(Path outputDirectory, String fileName) throws
IOException {
+ public void outputMetrics(PrintStream out) throws IOException {
if (getCoreContainer() != null) {
-
- if (outputDirectory != null) {
- Path outDir = outputDirectory;
- Files.createDirectories(outDir);
- }
-
SolrMetricManager metricsManager = getCoreContainer().getMetricManager();
Set<String> registryNames = metricsManager.registryNames();
for (String registryName : registryNames) {
var prometheusReader =
metricsManager.getPrometheusMetricReader(registryName);
if (prometheusReader != null) {
- try (OutputStream os =
- outputDirectory == null
- ? OutputStream.nullOutputStream()
- : Files.newOutputStream(outputDirectory.resolve(registryName
+ "_" + fileName))) {
- new PrometheusTextFormatWriter(false).write(os,
prometheusReader.collect());
- }
+ out.println("Registry: " + registryName);
Review Comment:
Probably should improve to use a `#` comment style as it's prometheus. And
add some surrounding lines to make it easier to spot.
--
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]
