dsmiley commented on code in PR #2599:
URL: https://github.com/apache/solr/pull/2599#discussion_r1695854596
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java:
##########
@@ -107,9 +111,15 @@ 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) {
+ Map<String, Object> properties = getClusterProperties();
+ if (properties == null) {
+ return defaultValue;
+ }
@SuppressWarnings({"unchecked"})
- T value = (T) getClusterProperties().get(key);
- if (value == null) return defaultValue;
+ T value = (T) properties.get(key);
Review Comment:
call getOrDefault
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -173,6 +167,33 @@ private ClusterState fetchClusterState(
return cs;
}
+ private SimpleOrderedMap<?> submitClusterStateRequest(
+ SolrClient client,
+ String collection,
+ boolean fetchLiveNodes,
+ boolean fetchClusterProp,
+ boolean fetchNodeRoles)
Review Comment:
Methods with tons of booleans is hard to read at the caller. Would it be
sufficiently concise to heave each caller request what they want? I could
imagine doing so in only a 2-3 lines each.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -88,6 +88,9 @@ public class ZkStateReader implements SolrCloseable {
public static final String CORE_NAME_PROP = "core";
public static final String COLLECTION_PROP = "collection";
+ public static final String INCLUDE_ALL = "includeAll";
+ public static final String LIVENODES_PROP = "liveNodes";
+ public static final String CLUSTER_PROP = "clusterProperties";
Review Comment:
No; please don't add constants to ZkStateReader that are really HTTP
parameter level constants. Instead, see CollectionAdminParams if you must...
but for this PR, these constants are really specific to a particular request,
so I suggest just leaving them as constants in ClusterStatus.java
I'm aware some older constants here are used for multiple purposes but
that's a bad practice. The constants here _should_ only be used for objects in
ZK.
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -45,7 +45,11 @@
public class ClusterStatus {
private final ZkStateReader zkStateReader;
private final ZkNodeProps message;
- private final String collection; // maybe null
+ private final String collectionParam; // maybe null
+ private final boolean liveNodesParam;
+ private final boolean clusterPropertiesParam;
+ private final boolean rolesParam;
+ private final boolean includeAll;
Review Comment:
Instead see `org.apache.solr.handler.admin.ColStatus#getColStatus` for
better naming, like "withSegments" etc.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -228,12 +249,9 @@ > getCacheTimeout()) {
}
@SuppressWarnings({"rawtypes", "unchecked"})
- private static Set<String> fetchLiveNodes(SolrClient client) throws
Exception {
- ModifiableSolrParams params = new ModifiableSolrParams();
- params.set("action", "CLUSTERSTATUS");
- QueryRequest request = new QueryRequest(params);
- request.setPath("/admin/collections");
- NamedList cluster = (SimpleOrderedMap)
client.request(request).get("cluster");
+ private Set<String> fetchLiveNodes(SolrClient client) throws Exception {
+
+ SimpleOrderedMap<?> cluster = submitClusterStateRequest(client, null,
true, false, false);
Review Comment:
oh yes, null, true false false. Uh..... what does that refer to? ;-).
See my other comment.
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -92,11 +96,57 @@ public static Health combine(Collection<Health> states) {
public ClusterStatus(ZkStateReader zkStateReader, ZkNodeProps props) {
this.zkStateReader = zkStateReader;
this.message = props;
- collection = props.getStr(ZkStateReader.COLLECTION_PROP);
+ collectionParam = props.getStr(ZkStateReader.COLLECTION_PROP);
+ liveNodesParam = props.getBool(ZkStateReader.LIVENODES_PROP, false);
+ clusterPropertiesParam = props.getBool(ZkStateReader.CLUSTER_PROP, false);
+ rolesParam = props.getBool(ZkStateReader.ROLES_PROP, false);
+ includeAll = props.getBool(ZkStateReader.INCLUDE_ALL, true);
}
public void getClusterStatus(NamedList<Object> results)
throws KeeperException, InterruptedException {
+
+ List<String> liveNodes = null;
+ NamedList<Object> clusterStatus = new SimpleOrderedMap<>();
+ if (includeAll || collectionParam != null || liveNodesParam) {
+ liveNodes =
+
zkStateReader.getZkClient().getChildren(ZkStateReader.LIVE_NODES_ZKNODE, null,
true);
+ // add live_nodes
+ clusterStatus.add("live_nodes", liveNodes);
+ }
+
+ if (includeAll || collectionParam != null)
+ fetchClusterStatusForCollOrAlias(clusterStatus, liveNodes);
+
+ if (includeAll || clusterPropertiesParam) {
+ // read cluster properties
+ Map<String, Object> clusterProps = zkStateReader.getClusterProperties();
+ if (clusterProps != null && !clusterProps.isEmpty()) {
+ clusterStatus.add("properties", clusterProps);
+ }
+ }
+
+ // add the roles map
+ if (includeAll || rolesParam) {
Review Comment:
instead of constantly checking includeAll _as well as_ the one param for the
category, consider defaulting the category boolean to be based on includeAll.
The way you coded it, it's impossible to include all and exclude one category.
I'm looking at the beginning of ColStatus.getColStatus for inspiration and
seeing what I suggest would be really trivial; just change a bunch of "false"
references to "includeAll"
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ClusterStateProvider.java:
##########
@@ -107,9 +111,15 @@ 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) {
+ Map<String, Object> properties = getClusterProperties();
Review Comment:
The change could introduce a regression to a caller, albeit rather minor.
Why not have it return Collections.empty() so the callers can skip the null
check?
--
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]
