dsmiley commented on code in PR #2916:
URL: https://github.com/apache/solr/pull/2916#discussion_r1893056849
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -342,76 +356,47 @@ public static Map<String, Object>
postProcessCollectionJSON(Map<String, Object>
return collection;
}
- private class SolrCollectionProperiesIterator implements
Iterator<NamedList<Object>> {
-
- final Iterator<DocCollection> it;
- Map<String, List<String>> collectionVsAliases;
- String routeKey;
- List<String> liveNodes;
- String shard;
-
- public SolrCollectionProperiesIterator(
- Iterator<DocCollection> it,
- Map<String, List<String>> collectionVsAliases,
- String routeKey,
- List<String> liveNodes,
- String shard) {
- this.it = it;
- this.collectionVsAliases = collectionVsAliases;
- this.routeKey = routeKey;
- this.liveNodes = liveNodes;
- this.shard = shard;
- }
-
- @Override
- public boolean hasNext() {
- return it.hasNext();
- }
-
- @Override
- public NamedList<Object> next() {
- NamedList<Object> collectionProps = new SimpleOrderedMap<>();
- DocCollection clusterStateCollection = it.next();
- Map<String, Object> collectionStatus;
- String name = clusterStateCollection.getName();
-
- Set<String> requestedShards = new HashSet<>();
- if (routeKey != null) {
- DocRouter router = clusterStateCollection.getRouter();
- Collection<Slice> slices = router.getSearchSlices(routeKey, null,
clusterStateCollection);
- for (Slice slice : slices) {
- requestedShards.add(slice.getName());
- }
- }
- if (shard != null) {
- String[] paramShards = shard.split(",");
- requestedShards.addAll(Arrays.asList(paramShards));
+ private Map<String, Object> collectionPropsResponse(
+ DocCollection clusterStateCollection,
+ Map<String, List<String>> collectionVsAliases,
+ String routeKey,
+ List<String> liveNodes,
+ Set<String> requestedShards) {
+ Map<String, Object> collectionProps = new HashMap<>();
+ Map<String, Object> collectionStatus;
+ String name = clusterStateCollection.getName();
+
+ if (routeKey != null) {
+ DocRouter router = clusterStateCollection.getRouter();
+ Collection<Slice> slices = router.getSearchSlices(routeKey, null,
clusterStateCollection);
+ for (Slice slice : slices) {
+ requestedShards.add(slice.getName());
}
+ }
- byte[] bytes = Utils.toJSON(clusterStateCollection);
- @SuppressWarnings("unchecked")
- Map<String, Object> docCollection = (Map<String, Object>)
Utils.fromJSON(bytes);
- collectionStatus = getCollectionStatus(docCollection, name,
requestedShards);
+ byte[] bytes = Utils.toJSON(clusterStateCollection);
+ @SuppressWarnings("unchecked")
+ Map<String, Object> docCollection = (Map<String, Object>)
Utils.fromJSON(bytes);
+ collectionStatus = getCollectionStatus(docCollection, name,
requestedShards);
- collectionStatus.put("znodeVersion",
clusterStateCollection.getZNodeVersion());
- collectionStatus.put(
- "creationTimeMillis",
clusterStateCollection.getCreationTime().toEpochMilli());
+ collectionStatus.put("znodeVersion",
clusterStateCollection.getZNodeVersion());
+ collectionStatus.put(
+ "creationTimeMillis",
clusterStateCollection.getCreationTime().toEpochMilli());
- if (collectionVsAliases.containsKey(name) &&
!collectionVsAliases.get(name).isEmpty()) {
- collectionStatus.put("aliases", collectionVsAliases.get(name));
- }
- String configName = clusterStateCollection.getConfigName();
- collectionStatus.put("configName", configName);
- if (solrParams.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
- PerReplicaStates prs = clusterStateCollection.getPerReplicaStates();
- collectionStatus.put("PRS", prs);
- }
- collectionProps.add(name, collectionStatus);
-
- // now we need to walk the collectionProps tree to cross-check replica
state with live
- // nodes
- crossCheckReplicaStateWithLiveNodes(liveNodes, collectionProps);
- return collectionProps;
+ if (collectionVsAliases.containsKey(name) &&
!collectionVsAliases.get(name).isEmpty()) {
+ collectionStatus.put("aliases", collectionVsAliases.get(name));
+ }
+ String configName = clusterStateCollection.getConfigName();
+ collectionStatus.put("configName", configName);
+ if (solrParams.getBool("prs", false) &&
clusterStateCollection.isPerReplicaState()) {
+ PerReplicaStates prs = clusterStateCollection.getPerReplicaStates();
+ collectionStatus.put("PRS", prs);
}
+ collectionProps.put(name, collectionStatus);
Review Comment:
it seems that we're returning an extra map here? Shouldn't we be returning
collectionStatus (that which holds the keys-value pairs describing this
collection), not collectionProps, which holds lists of collections (or did but
now has just one)
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -342,76 +356,47 @@ public static Map<String, Object>
postProcessCollectionJSON(Map<String, Object>
return collection;
}
- private class SolrCollectionProperiesIterator implements
Iterator<NamedList<Object>> {
-
- final Iterator<DocCollection> it;
- Map<String, List<String>> collectionVsAliases;
- String routeKey;
- List<String> liveNodes;
- String shard;
-
- public SolrCollectionProperiesIterator(
- Iterator<DocCollection> it,
- Map<String, List<String>> collectionVsAliases,
- String routeKey,
- List<String> liveNodes,
- String shard) {
- this.it = it;
- this.collectionVsAliases = collectionVsAliases;
- this.routeKey = routeKey;
- this.liveNodes = liveNodes;
- this.shard = shard;
- }
-
- @Override
- public boolean hasNext() {
- return it.hasNext();
- }
-
- @Override
- public NamedList<Object> next() {
- NamedList<Object> collectionProps = new SimpleOrderedMap<>();
- DocCollection clusterStateCollection = it.next();
- Map<String, Object> collectionStatus;
- String name = clusterStateCollection.getName();
-
- Set<String> requestedShards = new HashSet<>();
- if (routeKey != null) {
- DocRouter router = clusterStateCollection.getRouter();
- Collection<Slice> slices = router.getSearchSlices(routeKey, null,
clusterStateCollection);
- for (Slice slice : slices) {
- requestedShards.add(slice.getName());
- }
- }
- if (shard != null) {
- String[] paramShards = shard.split(",");
- requestedShards.addAll(Arrays.asList(paramShards));
+ private Map<String, Object> collectionPropsResponse(
+ DocCollection clusterStateCollection,
+ Map<String, List<String>> collectionVsAliases,
+ String routeKey,
+ List<String> liveNodes,
+ Set<String> requestedShards) {
+ Map<String, Object> collectionProps = new HashMap<>();
+ Map<String, Object> collectionStatus;
+ String name = clusterStateCollection.getName();
+
+ if (routeKey != null) {
+ DocRouter router = clusterStateCollection.getRouter();
+ Collection<Slice> slices = router.getSearchSlices(routeKey, null,
clusterStateCollection);
+ for (Slice slice : slices) {
+ requestedShards.add(slice.getName());
Review Comment:
wow, so you're manipulating the contents of a parameter (requestedShards) to
this method? Is that needed?
If this is a bug, it'd be more easily caught if we embrace more immutability
-- if requestedShards was created via Set.of instead of new HashSet.
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -183,6 +183,12 @@ private void fetchClusterStatusForCollOrAlias(
String routeKey = solrParams.get(ShardParams._ROUTE_);
String shard = solrParams.get(ZkStateReader.SHARD_ID_PROP);
+ Set<String> requestedShards = new HashSet<>();
+ if (shard != null) {
+ String[] paramShards = shard.split(",");
+ requestedShards.addAll(Arrays.asList(paramShards));
+ }
Review Comment:
I'd prefer Set.of(...) for immutability but it's ok
##########
solr/solrj/src/java/org/apache/solr/common/util/NamedList.java:
##########
@@ -363,6 +363,8 @@ public Object findRecursive(String... args) {
} else {
if (value instanceof NamedList) {
currentList = (NamedList<?>) value;
+ } else if (value instanceof Map) {
+ currentList = new NamedList<>((Map<String, Object>) value);
Review Comment:
woah; definitely don't want to copy a map into a new NamedList just because
findRecursive is called!
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -342,76 +356,47 @@ public static Map<String, Object>
postProcessCollectionJSON(Map<String, Object>
return collection;
}
- private class SolrCollectionProperiesIterator implements
Iterator<NamedList<Object>> {
-
- final Iterator<DocCollection> it;
- Map<String, List<String>> collectionVsAliases;
- String routeKey;
- List<String> liveNodes;
- String shard;
-
- public SolrCollectionProperiesIterator(
- Iterator<DocCollection> it,
- Map<String, List<String>> collectionVsAliases,
- String routeKey,
- List<String> liveNodes,
- String shard) {
- this.it = it;
- this.collectionVsAliases = collectionVsAliases;
- this.routeKey = routeKey;
- this.liveNodes = liveNodes;
- this.shard = shard;
- }
-
- @Override
- public boolean hasNext() {
- return it.hasNext();
- }
-
- @Override
- public NamedList<Object> next() {
- NamedList<Object> collectionProps = new SimpleOrderedMap<>();
- DocCollection clusterStateCollection = it.next();
- Map<String, Object> collectionStatus;
- String name = clusterStateCollection.getName();
-
- Set<String> requestedShards = new HashSet<>();
- if (routeKey != null) {
- DocRouter router = clusterStateCollection.getRouter();
- Collection<Slice> slices = router.getSearchSlices(routeKey, null,
clusterStateCollection);
- for (Slice slice : slices) {
- requestedShards.add(slice.getName());
- }
- }
- if (shard != null) {
- String[] paramShards = shard.split(",");
- requestedShards.addAll(Arrays.asList(paramShards));
+ private Map<String, Object> collectionPropsResponse(
Review Comment:
confusing name. I suggest buildResponseForCollection.
BTW "collectionProps" is not a name anyone would choose today; this is older
code.
--
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]
