dsmiley commented on code in PR #2853:
URL: https://github.com/apache/solr/pull/2853#discussion_r1874956309
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -128,100 +129,85 @@ public ClusterState.CollectionRef getState(String
collection) {
}
@SuppressWarnings("unchecked")
- private ClusterState fetchClusterState(
- SolrClient client, String collection, Map<String, Object>
clusterProperties)
+ private ClusterState fetchClusterState(SolrClient client)
throws SolrServerException, IOException, NotACollectionException {
SimpleOrderedMap<?> cluster =
- submitClusterStateRequest(client, collection,
ClusterStateRequestType.FETCH_COLLECTION);
-
- Map<String, Object> collectionsMap;
- if (collection != null) {
- collectionsMap =
- Collections.singletonMap(
- collection, ((NamedList<?>)
cluster.get("collections")).get(collection));
- } else {
- collectionsMap = ((NamedList<?>) cluster.get("collections")).asMap(10);
- }
- int znodeVersion;
- Map<String, Object> collFromStatus = (Map<String, Object>)
(collectionsMap).get(collection);
- if (collection != null && collFromStatus == null) {
- throw new NotACollectionException(); // probably an alias
- }
- if (collection != null) { // can be null if alias
- znodeVersion = (int) collFromStatus.get("znodeVersion");
- } else {
- znodeVersion = -1;
- }
+ submitClusterStateRequest(client, null,
ClusterStateRequestType.FETCH_CLUSTER_STATE);
- ClusterState cs = new ClusterState(this.liveNodes, new HashMap<>());
List<String> liveNodesList = (List<String>) cluster.get("live_nodes");
if (liveNodesList != null) {
- Set<String> liveNodes = new HashSet<>(liveNodesList);
- this.liveNodes = liveNodes;
+ this.liveNodes = Set.copyOf(liveNodesList);
liveNodesTimestamp = System.nanoTime();
- cs = new ClusterState(liveNodes, new HashMap<>());
}
- for (Map.Entry<String, Object> e : collectionsMap.entrySet()) {
- @SuppressWarnings("rawtypes")
- Map m = (Map) e.getValue();
- Long creationTimeMillisFromClusterStatus = (Long)
m.get("creationTimeMillis");
- Instant creationTime =
- creationTimeMillisFromClusterStatus == null
- ? Instant.EPOCH
- : Instant.ofEpochMilli(creationTimeMillisFromClusterStatus);
- cs = cs.copyWith(e.getKey(), fillPrs(znodeVersion, e, creationTime, m));
- }
+ var collectionsNl = (NamedList<Map<String, Object>>)
cluster.get("collections");
- if (clusterProperties != null) {
- Map<String, Object> properties = (Map<String, Object>)
cluster.get("properties");
- if (properties != null) {
- clusterProperties.putAll(properties);
- }
+ Map<String, DocCollection> collStateByName = new
LinkedHashMap<>(collectionsNl.size());
+ for (Entry<String, Map<String, Object>> entry : collectionsNl) {
+ collStateByName.put(
+ entry.getKey(), getDocCollectionFromObjects(entry.getKey(),
entry.getValue()));
}
- return cs;
- }
- private SimpleOrderedMap<?> submitClusterStateRequest(
- SolrClient client, String collection, ClusterStateRequestType
requestType)
- throws SolrServerException, IOException {
-
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.set("action", "CLUSTERSTATUS");
+ return new ClusterState(this.liveNodes, collStateByName);
+ }
- if (requestType == ClusterStateRequestType.FETCH_COLLECTION && collection
!= null) {
- params.set("collection", collection);
- } else if (requestType == ClusterStateRequestType.FETCH_LIVE_NODES) {
- params.set("liveNodes", "true");
- } else if (requestType == ClusterStateRequestType.FETCH_CLUSTER_PROP) {
- params.set("clusterProperties", "true");
- } else if (requestType == ClusterStateRequestType.FETCH_NODE_ROLES) {
- params.set("roles", "true");
- }
+ @SuppressWarnings("unchecked")
+ private DocCollection getDocCollectionFromObjects(
+ String collectionName, Map<String, Object> collStateMap) {
+ int zNodeVersion = (int) collStateMap.get("znodeVersion");
- params.set("includeAll", "false");
- params.set("prs", "true");
- QueryRequest request = new QueryRequest(params);
- request.setPath("/admin/collections");
- return (SimpleOrderedMap<?>) client.request(request).get("cluster");
- }
+ Long creationTimeMillis = (Long) collStateMap.get("creationTimeMillis");
+ Instant creationTime =
+ creationTimeMillis == null ? Instant.EPOCH :
Instant.ofEpochMilli(creationTimeMillis);
- @SuppressWarnings({"rawtypes", "unchecked"})
- private DocCollection fillPrs(
Review Comment:
I inlined `fillPrs` into `getDocCollectionFromObjects` and added a bit more
responsibility here so that the callers needn't handle zk version.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -221,7 +175,43 @@ private DocCollection fillPrs(
}
return ClusterState.collectionFromObjects(
- e.getKey(), m, znodeVersion, creationTime, prsSupplier);
+ collectionName, collStateMap, zNodeVersion, creationTime, prsSupplier);
+ }
+
+ @SuppressWarnings("unchecked")
+ private DocCollection fetchCollectionState(SolrClient client, String
collection)
+ throws SolrServerException, IOException, NotACollectionException {
+
+ SimpleOrderedMap<?> cluster =
+ submitClusterStateRequest(client, collection,
ClusterStateRequestType.FETCH_COLLECTION);
+
+ var collStateMap = (Map<String, Object>)
cluster.findRecursive("collections", collection);
+ if (collStateMap == null) {
+ throw new NotACollectionException(); // probably an alias
+ }
+ return getDocCollectionFromObjects(collection, collStateMap);
+ }
+
+ private SimpleOrderedMap<?> submitClusterStateRequest(
+ SolrClient client, String collection, ClusterStateRequestType
requestType)
+ throws SolrServerException, IOException {
+
+ ModifiableSolrParams params = new ModifiableSolrParams();
+ params.set("action", "CLUSTERSTATUS");
+
+ params.set("includeAll", false); // will flip flor CLUSTER_STATE
+ switch (requestType) {
+ case FETCH_CLUSTER_STATE -> params.set("includeAll", true);
+ case FETCH_COLLECTION -> {
+ if (collection != null) params.set("collection", collection);
+ }
+ case FETCH_LIVE_NODES -> params.set("liveNodes", true);
+ case FETCH_CLUSTER_PROP -> params.set("clusterProperties", true);
+ case FETCH_NODE_ROLES -> params.set("roles", true);
+ }
+ params.set("prs", true);
+ var request = new GenericSolrRequest(METHOD.GET, "/admin/collections",
params);
Review Comment:
formerly was QueryRequest which was wrong even though it apparently worked.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -47,19 +54,44 @@ public static void setupCluster() throws Exception {
.configure();
}
- private ClusterStateProvider createClusterStateProvider() throws Exception {
- return !usually() ? http2ClusterStateProvider() :
zkClientClusterStateProvider();
+ @ParametersFactory
+ public static Iterable<String[]> parameters() throws NoSuchMethodException {
+ return List.of(
+ new String[] {"http2ClusterStateProvider"}, new String[]
{"zkClientClusterStateProvider"});
}
- private ClusterStateProvider http2ClusterStateProvider() throws Exception {
- return new Http2ClusterStateProvider(
- List.of(cluster.getJettySolrRunner(0).getBaseUrl().toString()), null);
+ private static ClusterStateProvider http2ClusterStateProvider() {
+ try {
+ return new Http2ClusterStateProvider(
+ List.of(cluster.getJettySolrRunner(0).getBaseUrl().toString()),
null);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
}
- private ClusterStateProvider zkClientClusterStateProvider() {
+ private static ClusterStateProvider zkClientClusterStateProvider() {
return new ZkClientClusterStateProvider(cluster.getZkStateReader());
}
+ private final Supplier<ClusterStateProvider> cspSupplier;
+
+ public ClusterStateProviderTest(String method) throws Exception {
Review Comment:
by using a String arg that refers to a method, the printed test execution is
clear as to what is being invoked. This uses reflection allows deferred
execution of construction of the CSP, which is useful.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
Review Comment:
Refactored this test to a parameterized test to guarantee we test with both
impls.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -128,100 +129,85 @@ public ClusterState.CollectionRef getState(String
collection) {
}
@SuppressWarnings("unchecked")
- private ClusterState fetchClusterState(
- SolrClient client, String collection, Map<String, Object>
clusterProperties)
+ private ClusterState fetchClusterState(SolrClient client)
throws SolrServerException, IOException, NotACollectionException {
SimpleOrderedMap<?> cluster =
- submitClusterStateRequest(client, collection,
ClusterStateRequestType.FETCH_COLLECTION);
-
- Map<String, Object> collectionsMap;
- if (collection != null) {
- collectionsMap =
- Collections.singletonMap(
- collection, ((NamedList<?>)
cluster.get("collections")).get(collection));
- } else {
- collectionsMap = ((NamedList<?>) cluster.get("collections")).asMap(10);
- }
- int znodeVersion;
- Map<String, Object> collFromStatus = (Map<String, Object>)
(collectionsMap).get(collection);
- if (collection != null && collFromStatus == null) {
- throw new NotACollectionException(); // probably an alias
- }
- if (collection != null) { // can be null if alias
- znodeVersion = (int) collFromStatus.get("znodeVersion");
- } else {
- znodeVersion = -1;
- }
+ submitClusterStateRequest(client, null,
ClusterStateRequestType.FETCH_CLUSTER_STATE);
- ClusterState cs = new ClusterState(this.liveNodes, new HashMap<>());
List<String> liveNodesList = (List<String>) cluster.get("live_nodes");
if (liveNodesList != null) {
- Set<String> liveNodes = new HashSet<>(liveNodesList);
- this.liveNodes = liveNodes;
+ this.liveNodes = Set.copyOf(liveNodesList);
liveNodesTimestamp = System.nanoTime();
- cs = new ClusterState(liveNodes, new HashMap<>());
}
- for (Map.Entry<String, Object> e : collectionsMap.entrySet()) {
- @SuppressWarnings("rawtypes")
- Map m = (Map) e.getValue();
- Long creationTimeMillisFromClusterStatus = (Long)
m.get("creationTimeMillis");
- Instant creationTime =
- creationTimeMillisFromClusterStatus == null
- ? Instant.EPOCH
- : Instant.ofEpochMilli(creationTimeMillisFromClusterStatus);
- cs = cs.copyWith(e.getKey(), fillPrs(znodeVersion, e, creationTime, m));
- }
+ var collectionsNl = (NamedList<Map<String, Object>>)
cluster.get("collections");
- if (clusterProperties != null) {
- Map<String, Object> properties = (Map<String, Object>)
cluster.get("properties");
- if (properties != null) {
- clusterProperties.putAll(properties);
- }
+ Map<String, DocCollection> collStateByName = new
LinkedHashMap<>(collectionsNl.size());
+ for (Entry<String, Map<String, Object>> entry : collectionsNl) {
+ collStateByName.put(
+ entry.getKey(), getDocCollectionFromObjects(entry.getKey(),
entry.getValue()));
}
- return cs;
- }
- private SimpleOrderedMap<?> submitClusterStateRequest(
- SolrClient client, String collection, ClusterStateRequestType
requestType)
- throws SolrServerException, IOException {
-
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.set("action", "CLUSTERSTATUS");
+ return new ClusterState(this.liveNodes, collStateByName);
+ }
- if (requestType == ClusterStateRequestType.FETCH_COLLECTION && collection
!= null) {
- params.set("collection", collection);
- } else if (requestType == ClusterStateRequestType.FETCH_LIVE_NODES) {
- params.set("liveNodes", "true");
- } else if (requestType == ClusterStateRequestType.FETCH_CLUSTER_PROP) {
- params.set("clusterProperties", "true");
- } else if (requestType == ClusterStateRequestType.FETCH_NODE_ROLES) {
- params.set("roles", "true");
- }
+ @SuppressWarnings("unchecked")
+ private DocCollection getDocCollectionFromObjects(
+ String collectionName, Map<String, Object> collStateMap) {
+ int zNodeVersion = (int) collStateMap.get("znodeVersion");
- params.set("includeAll", "false");
- params.set("prs", "true");
- QueryRequest request = new QueryRequest(params);
- request.setPath("/admin/collections");
- return (SimpleOrderedMap<?>) client.request(request).get("cluster");
- }
+ Long creationTimeMillis = (Long) collStateMap.get("creationTimeMillis");
+ Instant creationTime =
+ creationTimeMillis == null ? Instant.EPOCH :
Instant.ofEpochMilli(creationTimeMillis);
- @SuppressWarnings({"rawtypes", "unchecked"})
- private DocCollection fillPrs(
- int znodeVersion, Map.Entry<String, Object> e, Instant creationTime, Map
m) {
DocCollection.PrsSupplier prsSupplier = null;
- if (m.containsKey("PRS")) {
- Map prs = (Map) m.remove("PRS");
+ if (collStateMap.containsKey("PRS")) {
+ Map<String, Object> prs = (Map<String, Object>)
collStateMap.remove("PRS");
prsSupplier =
() ->
new PerReplicaStates(
(String) prs.get("path"),
(Integer) prs.get("cversion"),
(List<String>) prs.get("states"));
}
-
return ClusterState.collectionFromObjects(
- e.getKey(), m, znodeVersion, creationTime, prsSupplier);
+ collectionName, collStateMap, zNodeVersion, creationTime, prsSupplier);
+ }
+
+ @SuppressWarnings("unchecked")
+ private DocCollection fetchCollectionState(SolrClient client, String
collection)
+ throws SolrServerException, IOException, NotACollectionException {
+
+ SimpleOrderedMap<?> cluster =
+ submitClusterStateRequest(client, collection,
ClusterStateRequestType.FETCH_COLLECTION);
+
+ var collStateMap = (Map<String, Object>)
cluster.findRecursive("collections", collection);
+ if (collStateMap == null) {
+ throw new NotACollectionException(); // probably an alias
+ }
+ return getDocCollectionFromObjects(collection, collStateMap);
+ }
+
+ private SimpleOrderedMap<?> submitClusterStateRequest(
+ SolrClient client, String collection, ClusterStateRequestType
requestType)
+ throws SolrServerException, IOException {
+
+ ModifiableSolrParams params = new ModifiableSolrParams();
+ params.set("action", "CLUSTERSTATUS");
+
+ params.set("includeAll", false); // will flip flor CLUSTER_STATE
+ switch (requestType) {
Review Comment:
switch is more idiomatic
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -117,4 +149,39 @@ private Instant getCreationTimeFromClusterStatus(String
collectionName)
Map<String, Object> collection = (Map<String, Object>)
collections.get(collectionName);
return Instant.ofEpochMilli((long) collection.get("creationTimeMillis"));
}
+
+ @Test
+ public void testClusterStateProvider() throws SolrServerException,
IOException {
Review Comment:
In the process of writing this test, I discovered some keys like "health" in
the base ZkNodeProps that prevented equivalency with the ZK one. So I enhanced
HttpCSP to remove these keys.
--
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
