dsmiley commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1704691188
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -89,14 +93,61 @@ public static Health combine(Collection<Health> states) {
}
}
- public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
+ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
this.zkStateReader = zkStateReader;
- this.message = props;
- collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+ this.solrParams = params;
+ collection = params.get(ZkStateReader.COLLECTION_PROP);
}
public void getClusterStatus(NamedList<Object> results)
throws KeeperException, InterruptedException {
+ NamedList<Object> clusterStatus = new SimpleOrderedMap<>();
+
+ boolean includeAll = solrParams.getBool(INCLUDE_ALL, true);
+ boolean withLiveNodes = solrParams.getBool(LIVENODES_PROP, includeAll);
+ boolean withClusterProperties = solrParams.getBool(CLUSTER_PROP,
includeAll);
+ boolean withRoles = solrParams.getBool(ZkStateReader.ROLES_PROP,
includeAll);
+ boolean withCollection = includeAll || (collection != null);
+
+ List<String> liveNodes = Collections.emptyList();
Review Comment:
shouldn't the default be null since it won't necessarily be fetched?
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -89,14 +93,61 @@ public static Health combine(Collection<Health> states) {
}
}
- public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
+ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
this.zkStateReader = zkStateReader;
- this.message = props;
- collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+ this.solrParams = params;
+ collection = params.get(ZkStateReader.COLLECTION_PROP);
}
public void getClusterStatus(NamedList<Object> results)
throws KeeperException, InterruptedException {
+ NamedList<Object> clusterStatus = new SimpleOrderedMap<>();
+
+ boolean includeAll = solrParams.getBool(INCLUDE_ALL, true);
+ boolean withLiveNodes = solrParams.getBool(LIVENODES_PROP, includeAll);
+ boolean withClusterProperties = solrParams.getBool(CLUSTER_PROP,
includeAll);
+ boolean withRoles = solrParams.getBool(ZkStateReader.ROLES_PROP,
includeAll);
+ boolean withCollection = includeAll || (collection != null);
+
+ List<String> liveNodes = Collections.emptyList();
+ if (withLiveNodes || collection != null) {
+ liveNodes =
+
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
+ // add live_nodes
+ if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
+ }
+ if (withCollection) {
+ fetchClusterStatusForCollOrAlias(clusterStatus, liveNodes);
+ }
+
+ Map<String, Object> clusterProps = Collections.emptyMap();
+ if (withClusterProperties) {
+ // read cluster properties
+ clusterProps = zkStateReader.getClusterProperties();
+ if (clusterProps == null || clusterProps.isEmpty()) {
+ clusterProps = Collections.emptyMap();
+ }
+ clusterStatus.add("properties", clusterProps);
+ }
+
+ // add the roles map
+ Map<?, ?> roles = Collections.emptyMap();
Review Comment:
Why define outside the "if"?
##########
solr/solr-ref-guide/modules/deployment-guide/pages/cluster-node-management.adoc:
##########
@@ -99,6 +99,41 @@ Multiple shard names can be specified as a comma-separated
list.
+
This can be used if you need the details of the shard where a particular
document belongs to and you don't know which shard it falls under.
+`liveNodes`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: will default to the default value of includeAll parameter
specified below
+|===
++
+If set to true, returns the status of live nodes in the cluster.
+
+`clusterProperties`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: will default to the default value of includeAll parameter
specified below
+|===
++
+If set to true, returns the properties of the cluster.
+
+`roles`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: will default to the default value of includeAll parameter
specified below
+|===
++
+If set to true, returns the roles within the cluster.
+
+`includeAll`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: false
Review Comment:
includeAll must default to "true" to be backwards compatible, and this is
what you used in the code. And you didn't document it.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java:
##########
@@ -108,8 +112,7 @@ default DocCollection getCollection(String name) throws
IOException {
/** Obtain a cluster property, or the default value if it doesn't exist. */
default <T> T getClusterProperty(String key, T defaultValue) {
@SuppressWarnings({"unchecked"})
- T value = (T) getClusterProperties().get(key);
- if (value == null) return defaultValue;
+ T value = (T) getClusterProperties().getOrDefault(key, defaultValue);
return value;
Review Comment:
Wow, you're right; even the code for getOrDefault suggests it's necessary.
Okay.
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -89,14 +93,61 @@ public static Health combine(Collection<Health> states) {
}
}
- public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
+ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
this.zkStateReader = zkStateReader;
- this.message = props;
- collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+ this.solrParams = params;
+ collection = params.get(ZkStateReader.COLLECTION_PROP);
}
public void getClusterStatus(NamedList<Object> results)
throws KeeperException, InterruptedException {
+ NamedList<Object> clusterStatus = new SimpleOrderedMap<>();
+
+ boolean includeAll = solrParams.getBool(INCLUDE_ALL, true);
+ boolean withLiveNodes = solrParams.getBool(LIVENODES_PROP, includeAll);
+ boolean withClusterProperties = solrParams.getBool(CLUSTER_PROP,
includeAll);
+ boolean withRoles = solrParams.getBool(ZkStateReader.ROLES_PROP,
includeAll);
+ boolean withCollection = includeAll || (collection != null);
+
+ List<String> liveNodes = Collections.emptyList();
+ if (withLiveNodes || collection != null) {
+ liveNodes =
+
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
+ // add live_nodes
+ if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
+ }
+ if (withCollection) {
+ fetchClusterStatusForCollOrAlias(clusterStatus, liveNodes);
+ }
+
+ Map<String, Object> clusterProps = Collections.emptyMap();
Review Comment:
Why define outside the "if"?
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -89,14 +93,61 @@ public static Health combine(Collection<Health> states) {
}
}
- public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
+ public ClusterStatus(ZkStateReader zkStateReader, SolrParams params) {
this.zkStateReader = zkStateReader;
- this.message = props;
- collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+ this.solrParams = params;
+ collection = params.get(ZkStateReader.COLLECTION_PROP);
}
public void getClusterStatus(NamedList<Object> results)
throws KeeperException, InterruptedException {
+ NamedList<Object> clusterStatus = new SimpleOrderedMap<>();
+
+ boolean includeAll = solrParams.getBool(INCLUDE_ALL, true);
+ boolean withLiveNodes = solrParams.getBool(LIVENODES_PROP, includeAll);
+ boolean withClusterProperties = solrParams.getBool(CLUSTER_PROP,
includeAll);
+ boolean withRoles = solrParams.getBool(ZkStateReader.ROLES_PROP,
includeAll);
+ boolean withCollection = includeAll || (collection != null);
+
+ List<String> liveNodes = Collections.emptyList();
+ if (withLiveNodes || collection != null) {
+ liveNodes =
+
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
+ // add live_nodes
+ if (withLiveNodes) clusterStatus.add("live_nodes", liveNodes);
+ }
+ if (withCollection) {
+ fetchClusterStatusForCollOrAlias(clusterStatus, liveNodes);
Review Comment:
I would add a line above `assert liveNodes != null`
--
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]
