Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 #3156

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+110 −46
Open

SOLR-17582: Stream CLUSTERSTATUS API for SolrJ version >= 9.9 #3156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4b6c6f
Stream CLUSTERSTATUS API for SolrJ version >= 9.9
Feb 3, 2025
f0fbbf5
in-line map loop
Feb 3, 2025
ba76edc
Add solrVersion null check and test
mlbiscoc Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/cli/StatusTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ protected Map<String, String> getCloudStatus(SolrClient solrClient, String zkHos
cloudStatus.put("liveNodes", String.valueOf(liveNodes.size()));

// TODO get this as a metric from the metrics API instead, or something else.
var collections = (NamedList<Object>) json.findRecursive("cluster", "collections");
var collections = (Map<String, Object>) json.findRecursive("cluster", "collections");
cloudStatus.put("collections", String.valueOf(collections.size()));

return cloudStatus;
Expand Down
35 changes: 18 additions & 17 deletions solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.stream.Stream;
import org.apache.solr.client.api.util.SolrVersion;
import org.apache.solr.common.MapWriter;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.cloud.Aliases;
Expand All @@ -34,7 +35,6 @@
import org.apache.solr.common.cloud.PerReplicaStates;
import org.apache.solr.common.cloud.Replica;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.ShardParams;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.NamedList;
Expand Down Expand Up @@ -101,7 +101,7 @@ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
collection = params.get(ZkStateReader.COLLECTION_PROP);
}

public void getClusterStatus(NamedList<Object> results)
public void getClusterStatus(NamedList<Object> results, SolrVersion solrVersion)
throws KeeperException, InterruptedException {
NamedList<Object> clusterStatus = new SimpleOrderedMap<>();

Expand All @@ -127,7 +127,7 @@ public void getClusterStatus(NamedList<Object> results)

if (withCollection) {
assert liveNodes != null;
fetchClusterStatusForCollOrAlias(clusterStatus, liveNodes, aliases);
fetchClusterStatusForCollOrAlias(clusterStatus, liveNodes, aliases, solrVersion);
}

if (withAliases) {
Expand Down Expand Up @@ -158,7 +158,10 @@ public void getClusterStatus(NamedList<Object> results)
}

private void fetchClusterStatusForCollOrAlias(
NamedList<Object> clusterStatus, List<String> liveNodes, Aliases aliases) {
NamedList<Object> clusterStatus,
List<String> liveNodes,
Aliases aliases,
SolrVersion solrVersion) {

// read aliases
Map<String, List<String>> collectionVsAliases = new HashMap<>();
Expand Down Expand Up @@ -206,19 +209,7 @@ private void fetchClusterStatusForCollOrAlias(
}
}

// Because of back-compat for SolrJ, create the whole response into a NamedList
// Otherwise stream with MapWriter to save memory
if (CommonParams.JAVABIN.equals(solrParams.get(CommonParams.WT))) {
NamedList<Object> collectionProps = new SimpleOrderedMap<>();
collectionStream.forEach(
collectionState -> {
collectionProps.add(
collectionState.getName(),
buildResponseForCollection(
collectionState, collectionVsAliases, routeKey, liveNodes, requestedShards));
});
clusterStatus.add("collections", collectionProps);
} else {
if (solrVersion == null || solrVersion.greaterThanOrEqualTo(SolrVersion.valueOf("9.9.0"))) {
MapWriter collectionPropsWriter =
ew -> {
collectionStream.forEach(
Expand All @@ -234,6 +225,16 @@ private void fetchClusterStatusForCollOrAlias(
});
};
clusterStatus.add("collections", collectionPropsWriter);
} else {
NamedList<Object> collectionProps = new SimpleOrderedMap<>();
collectionStream.forEach(
collectionState -> {
collectionProps.add(
collectionState.getName(),
buildResponseForCollection(
collectionState, collectionVsAliases, routeKey, liveNodes, requestedShards));
});
clusterStatus.add("collections", collectionProps);
}
}

Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,7 @@ public Map<String, Object> execute(
CLUSTERSTATUS,
(req, rsp, h) -> {
new ClusterStatus(h.coreContainer.getZkController().getZkStateReader(), req.getParams())
.getClusterStatus(rsp.getValues());
.getClusterStatus(rsp.getValues(), req.getHttpSolrCall().getUserAgentSolrVersion());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm; I wonder if HttpSolrCall could be null. No; we don't use CollectionsHandler in SolrCloud with an EmbeddedSolrServer. I really wish Java had a null-safe navigation operator as used in Kotlin -- ?.. I suppose leave this be; not worth adding verbosity for an exotic / hypothetical case.

return null;
}),
ADDREPLICAPROP_OP(
Expand Down
32 changes: 17 additions & 15 deletions solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,11 @@ private void testModifyCollection() throws Exception {
.getResponse();
NamedList<?> cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
NamedList<?> collections = (NamedList<?>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertEquals(1, collections.size());
assertEquals("25", collections._getStr(List.of(COLLECTION_NAME, "replicationFactor"), null));
Map<?, ?> collectionProperties = (Map<?, ?>) collections.get(COLLECTION_NAME);
assertEquals("25", collectionProperties.get("replicationFactor"));

params = new ModifiableSolrParams();
params.set("action", CollectionParams.CollectionAction.MODIFYCOLLECTION.toString());
Expand All @@ -153,10 +154,11 @@ private void testModifyCollection() throws Exception {
System.out.println(rsp);
cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
collections = (NamedList<?>) cluster.get("collections");
collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertEquals(1, collections.size());
assertNull(collections._getStr(List.of(COLLECTION_NAME, "replicationFactor"), null));
collectionProperties = (Map<?, ?>) collections.get(COLLECTION_NAME);
assertNull(collectionProperties.get("replicationFactor"));

params = new ModifiableSolrParams();
params.set("action", CollectionParams.CollectionAction.MODIFYCOLLECTION.toString());
Expand Down Expand Up @@ -255,7 +257,7 @@ private void testNoConfigset() throws Exception {
NamedList<?> rsp = client.request(req);
NamedList<?> cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
NamedList<?> collections = (NamedList<?>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertNotNull(
"Testing to insure collections are returned", collections.get(COLLECTION_NAME1));
Expand All @@ -282,7 +284,7 @@ private void assertCountsForRepFactorAndNrtReplicas(CloudSolrClient client, Stri
NamedList<Object> rsp = client.request(request);
NamedList<?> cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
NamedList<?> collections = (NamedList<?>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertEquals(1, collections.size());
@SuppressWarnings({"unchecked"})
Expand All @@ -304,7 +306,7 @@ private void clusterStatusWithCollectionAndShard() throws IOException, SolrServe
NamedList<Object> rsp = client.request(request);
NamedList<?> cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
NamedList<?> collections = (NamedList<?>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertNotNull(collections.get(COLLECTION_NAME));
assertEquals(1, collections.size());
Expand All @@ -330,7 +332,7 @@ private void clusterStatusWithCollectionAndMultipleShards()
NamedList<Object> rsp = request.process(client).getResponse();
NamedList<?> cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
NamedList<?> collections = (NamedList<?>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertNotNull(collections.get(COLLECTION_NAME));
assertEquals(1, collections.size());
Expand Down Expand Up @@ -465,7 +467,7 @@ private void clusterStatusNoCollection() throws Exception {
NamedList<Object> rsp = client.request(request);
NamedList<?> cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
NamedList<?> collections = (NamedList<?>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertNotNull(collections.get(COLLECTION_NAME1));
assertEquals(4, collections.size());
Expand All @@ -487,7 +489,7 @@ private void clusterStatusWithCollection() throws IOException, SolrServerExcepti
NamedList<Object> rsp = client.request(request);
NamedList<?> cluster = (NamedList<?>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
NamedList<?> collections = (NamedList<?>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
Comment on lines -490 to +492
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor: consider use of var in any place where we cast so we don't repeat the type. But again this is minor; feel free to just let this be and consider this advice to future changes.

assertNotNull("Collections should not be null in cluster state", collections);
assertEquals(1, collections.size());
@SuppressWarnings({"unchecked"})
Expand Down Expand Up @@ -517,7 +519,7 @@ private void clusterStatusZNodeVersion() throws Exception {
NamedList<Object> rsp = client.request(request);
NamedList<Object> cluster = (NamedList<Object>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
NamedList<Object> collections = (NamedList<Object>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertEquals(1, collections.size());
Map<String, Object> collection = (Map<String, Object>) collections.get(cname);
Expand All @@ -533,7 +535,7 @@ private void clusterStatusZNodeVersion() throws Exception {

rsp = client.request(request);
cluster = (NamedList<Object>) rsp.get("cluster");
collections = (NamedList<Object>) cluster.get("collections");
collections = (Map<?, ?>) cluster.get("collections");
collection = (Map<String, Object>) collections.get(cname);
Integer newVersion = (Integer) collection.get("znodeVersion");
assertNotNull(newVersion);
Expand All @@ -560,7 +562,7 @@ private void clusterStatusWithRouteKey() throws IOException, SolrServerException
NamedList<Object> cluster = (NamedList<Object>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
@SuppressWarnings({"unchecked"})
NamedList<Object> collections = (NamedList<Object>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertNotNull(collections.get(DEFAULT_COLLECTION));
assertEquals(1, collections.size());
Expand Down Expand Up @@ -607,7 +609,7 @@ private void clusterStatusAliasTest() throws Exception {
DEFAULT_COLLECTION + "," + COLLECTION_NAME,
aliases.get("myalias"));

NamedList<Object> collections = (NamedList<Object>) cluster.get("collections");
Map<?, ?> collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertNotNull(collections.get(DEFAULT_COLLECTION));
Map<String, Object> collection = (Map<String, Object>) collections.get(DEFAULT_COLLECTION);
Expand All @@ -627,7 +629,7 @@ private void clusterStatusAliasTest() throws Exception {

cluster = (NamedList<Object>) rsp.get("cluster");
assertNotNull("Cluster state should not be null", cluster);
collections = (NamedList<Object>) cluster.get("collections");
collections = (Map<?, ?>) cluster.get("collections");
assertNotNull("Collections should not be null in cluster state", collections);
assertNotNull(collections.get(DEFAULT_COLLECTION));
assertNotNull(collections.get(COLLECTION_NAME));
Expand Down
15 changes: 5 additions & 10 deletions solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
Expand All @@ -46,7 +45,6 @@
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.CollectionUtil;
import org.apache.solr.common.util.EnvUtils;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.SimpleOrderedMap;
import org.apache.solr.common.util.URLUtil;
import org.apache.solr.common.util.Utils;
Expand Down Expand Up @@ -160,15 +158,12 @@ private ClusterState fetchClusterState(SolrClient client)
liveNodesTimestamp = System.nanoTime();
}

var collectionsNl = (NamedList<Map<String, Object>>) cluster.get("collections");
var collectionsMap = (Map<String, Map<String, Object>>) cluster.get("collections");

Map<String, DocCollection> collStateByName =
CollectionUtil.newLinkedHashMap(collectionsNl.size());
for (Entry<String, Map<String, Object>> entry : collectionsNl) {
collStateByName.put(
entry.getKey(), getDocCollectionFromObjects(entry.getKey(), entry.getValue()));
}

CollectionUtil.newLinkedHashMap(collectionsMap.size());
collectionsMap.forEach(
(key, value) -> collStateByName.put(key, getDocCollectionFromObjects(key, value)));
return new ClusterState(this.liveNodes, collStateByName);
}

Expand Down Expand Up @@ -205,7 +200,7 @@ private DocCollection fetchCollectionState(SolrClient client, String collection)
SimpleOrderedMap<?> cluster =
submitClusterStateRequest(client, collection, ClusterStateRequestType.FETCH_COLLECTION);

var collStateMap = (Map<String, Object>) cluster.findRecursive("collections", collection);
var collStateMap = (Map<String, Object>) cluster._get(List.of("collections", collection), null);
if (collStateMap == null) {
throw new NotACollectionException(); // probably an alias
}
Expand Down
70 changes: 68 additions & 2 deletions solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your thoroughness. I should merge this as-is.

But...

In an ideal world, you wouldn't have had any need to do this -- no time spent for you or an explicit test here to maintain in perpetuity. Solr is lacking backwards-compatibility test infrastructure. A hello-world SolrCloud would have caught this problem. Such a thing would nowadays almost certainly use Docker (to run old Solr versions) and use TestContainers in Java to arrange for that. We do this at work, albeit not for backwards-compatibility but more for realistic / production like testing. The comment thread in https://issues.apache.org/jira/browse/SOLR-14903 gets at this. I should create a JIRA issue to spell out this need more. Really should be on our roadmap. I'll do that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. Would you prefer I just remove these backwards compatibility tests I wrote?

Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.lang.reflect.InvocationTargetException;
import java.time.Instant;
import java.util.List;
Expand All @@ -36,6 +37,8 @@
import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.common.cloud.DocCollection;
import org.apache.solr.common.util.NamedList;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpHeader;
import org.hamcrest.Matchers;
import org.junit.After;
import org.junit.BeforeClass;
Expand Down Expand Up @@ -135,7 +138,6 @@ public void testGetState() throws Exception {
try (ClusterStateProvider provider = createClusterStateProvider()) {

ClusterState.CollectionRef collectionRef = provider.getState("testGetState");

DocCollection docCollection = collectionRef.get();
assertNotNull(docCollection);
assertEquals(
Expand All @@ -159,7 +161,7 @@ private Instant getCreationTimeFromClusterStatus(String collectionName)
NamedList<Object> response = clusterStatusResponse.getResponse();

NamedList<Object> cluster = (NamedList<Object>) response.get("cluster");
NamedList<Object> collections = (NamedList<Object>) cluster.get("collections");
Map<String, Object> collections = (Map<String, Object>) cluster.get("collections");
Map<String, Object> collection = (Map<String, Object>) collections.get(collectionName);
return Instant.ofEpochMilli((long) collection.get("creationTimeMillis"));
}
Expand Down Expand Up @@ -198,6 +200,70 @@ public void testClusterStateProvider() throws SolrServerException, IOException {
}
}

@Test
public void testClusterStateProviderOldVersion() throws SolrServerException, IOException {
CollectionAdminRequest.setClusterProperty("ext.foo", "bar").process(cluster.getSolrClient());
createCollection("col1");
createCollection("col2");

try (var cspZk = zkClientClusterStateProvider();
var cspHttp = http2ClusterStateProvider()) {
// SolrJ < version 9.9.0 for non streamed response
cspHttp
.getHttpClient()
.getHttpClient()
.setUserAgentField(
new HttpField(
HttpHeader.USER_AGENT,
"Solr[" + MethodHandles.lookup().lookupClass().getName() + "] " + "9.8.0"));

assertThat(cspHttp.getCollection("col1"), equalTo(cspZk.getCollection("col1")));

final var clusterStateZk = cspZk.getClusterState();
final var clusterStateHttp = cspHttp.getClusterState();
assertThat(
clusterStateHttp.getLiveNodes(),
containsInAnyOrder(clusterStateHttp.getLiveNodes().toArray()));
assertEquals(2, clusterStateZk.size());
assertEquals(clusterStateZk.size(), clusterStateHttp.size());
assertThat(
clusterStateHttp.collectionStream().toList(),
containsInAnyOrder(clusterStateHttp.collectionStream().toArray()));

assertThat(
clusterStateZk.getCollection("col2"), equalTo(clusterStateHttp.getCollection("col2")));
}
}

@Test
public void testClusterStateProviderEmptySolrVersion() throws SolrServerException, IOException {
CollectionAdminRequest.setClusterProperty("ext.foo", "bar").process(cluster.getSolrClient());
createCollection("col1");
createCollection("col2");

try (var cspZk = zkClientClusterStateProvider();
var cspHttp = http2ClusterStateProvider()) {

cspHttp.getHttpClient().getHttpClient().setUserAgentField(null);

assertThat(cspHttp.getCollection("col1"), equalTo(cspZk.getCollection("col1")));

final var clusterStateZk = cspZk.getClusterState();
final var clusterStateHttp = cspHttp.getClusterState();
assertThat(
clusterStateHttp.getLiveNodes(),
containsInAnyOrder(clusterStateHttp.getLiveNodes().toArray()));
assertEquals(2, clusterStateZk.size());
assertEquals(clusterStateZk.size(), clusterStateHttp.size());
assertThat(
clusterStateHttp.collectionStream().toList(),
containsInAnyOrder(clusterStateHttp.collectionStream().toArray()));

assertThat(
clusterStateZk.getCollection("col2"), equalTo(clusterStateHttp.getCollection("col2")));
}
}

@Test
public void testClusterStateProviderDownedInitialLiveNodes() throws Exception {
try (var cspHttp = http2ClusterStateProvider()) {
Expand Down
Loading