dsmiley commented on code in PR #2916:
URL: https://github.com/apache/solr/pull/2916#discussion_r1894337155
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -216,28 +216,21 @@ private void fetchClusterStatusForCollOrAlias(
MapWriter collectionPropsWriter =
ew -> {
- Iterator<Map<String, Object>> it =
- collectionStream
- .map(
- (collectionState) ->
- collectionPropsResponse(
- collectionState,
- collectionVsAliases,
- routeKey,
- liveNodes,
- requestedShards))
- .iterator();
- while (it.hasNext()) {
- Map<String, Object> props = it.next();
- props.forEach(
- (key, value) -> {
- try {
- ew.put(key, value);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- });
- }
+ collectionStream.forEach(
+ (collectionState) -> {
+ try {
+ ew.put(
+ collectionState.getName(),
+ buildResponseForCollection(
+ collectionState,
+ collectionVsAliases,
+ routeKey,
+ liveNodes,
+ requestedShards));
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review Comment:
Why does buildResponseForCollection throw IOException? That's suspicious.
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -356,28 +346,28 @@ public static Map<String, Object>
postProcessCollectionJSON(Map<String, Object>
return collection;
}
- private Map<String, Object> collectionPropsResponse(
+ private Map<String, Object> buildResponseForCollection(
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;
+ Set<String> shards = new HashSet<>(requestedShards);
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());
+ shards.add(slice.getName());
}
Review Comment:
something seems wrong to me here. If requestedShards is specified, we
should use that. If routeKey is specified, we should use that (to compute the
shards). Or neither (99% of users won't do either). But we shouldn't do both
if for no other reason that it's confusing.
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -295,22 +288,19 @@ private Map<String, Object> getCollectionStatus(
@SuppressWarnings("unchecked")
protected void crossCheckReplicaStateWithLiveNodes(
List<String> liveNodes, Map<String, Object> collectionProps) {
- for (Map.Entry<String, Object> next : collectionProps.entrySet()) {
- Map<String, Object> collMap = (Map<String, Object>) next.getValue();
- Map<String, Object> shards = (Map<String, Object>) collMap.get("shards");
- for (Object nextShard : shards.values()) {
- Map<String, Object> shardMap = (Map<String, Object>) nextShard;
- Map<String, Object> replicas = (Map<String, Object>)
shardMap.get("replicas");
- for (Object nextReplica : replicas.values()) {
- Map<String, Object> replicaMap = (Map<String, Object>) nextReplica;
- if (Replica.State.getState((String)
replicaMap.get(ZkStateReader.STATE_PROP))
- != Replica.State.DOWN) {
- // not down, so verify the node is live
- String node_name = (String)
replicaMap.get(ZkStateReader.NODE_NAME_PROP);
- if (!liveNodes.contains(node_name)) {
- // node is not live, so this replica is actually down
- replicaMap.put(ZkStateReader.STATE_PROP,
Replica.State.DOWN.toString());
- }
+ Map<String, Object> shards = (Map<String, Object>)
collectionProps.get("shards");
+ for (Object nextShard : shards.values()) {
+ Map<String, Object> shardMap = (Map<String, Object>) nextShard;
+ Map<String, Object> replicas = (Map<String, Object>)
shardMap.get("replicas");
+ for (Object nextReplica : replicas.values()) {
+ Map<String, Object> replicaMap = (Map<String, Object>) nextReplica;
Review Comment:
I almost want to cry just glancing at this.
This is the poster-child for why Java introduced "var". And there may be
other approaches to improve it but that's the simplest.
(Yeah you didn't write it; I know)
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -183,10 +181,12 @@ private void fetchClusterStatusForCollOrAlias(
String routeKey = solrParams.get(ShardParams._ROUTE_);
String shard = solrParams.get(ZkStateReader.SHARD_ID_PROP);
- Set<String> requestedShards = new HashSet<>();
+ Set<String> requestedShards;
if (shard != null) {
String[] paramShards = shard.split(",");
- requestedShards.addAll(Arrays.asList(paramShards));
+ requestedShards = Set.of(paramShards);
+ } else {
+ requestedShards = Set.of();
}
Review Comment:
I bet this whole thing could be one line using ternary operator and no line
wrap.
Also, if "shard" isn't specified, I think it's clearer to have
requestedShards as null to indicate there was no request for any shards.
##########
solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java:
##########
@@ -216,28 +216,21 @@ private void fetchClusterStatusForCollOrAlias(
MapWriter collectionPropsWriter =
ew -> {
- Iterator<Map<String, Object>> it =
- collectionStream
- .map(
- (collectionState) ->
- collectionPropsResponse(
- collectionState,
- collectionVsAliases,
- routeKey,
- liveNodes,
- requestedShards))
- .iterator();
- while (it.hasNext()) {
- Map<String, Object> props = it.next();
- props.forEach(
- (key, value) -> {
- try {
- ew.put(key, value);
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- });
- }
+ collectionStream.forEach(
+ (collectionState) -> {
Review Comment:
friggin beautiful now
--
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]
