dsmiley commented on code in PR #2853:
URL: https://github.com/apache/solr/pull/2853#discussion_r1873806832


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -182,6 +157,37 @@ private ClusterState fetchClusterState(
     return cs;
   }
 
+  private DocCollection getDocCollectionFromObjects(Map.Entry<String, Object> 
e, int zNodeVersion) {

Review Comment:
   This is a weird method, taking a Map.Entry.  I suggest never doing that.   
It'd be clearer if it took the name & parsed object/Map.
   Naming the parsed object/Map collection variable isn't clear but if you 
can't think of something (better than 'm'!), then I propose `collStateMap`



##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -182,6 +157,37 @@ private ClusterState fetchClusterState(
     return cs;
   }
 
+  private DocCollection getDocCollectionFromObjects(Map.Entry<String, Object> 
e, int zNodeVersion) {
+    @SuppressWarnings("rawtypes")
+    Map m = (Map) e.getValue();
+    Long creationTimeMillisFromClusterStatus = (Long) 
m.get("creationTimeMillis");
+    Instant creationTime =
+        creationTimeMillisFromClusterStatus == null
+            ? Instant.EPOCH
+            : Instant.ofEpochMilli(creationTimeMillisFromClusterStatus);
+    return fillPrs(zNodeVersion, e, creationTime, m);

Review Comment:
   ah, you chose Map.Entry because you want to pass it to `fillPrs`, which 
already exists and takes this.  I'd update that method as well instead.



##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -182,6 +157,37 @@ private ClusterState fetchClusterState(
     return cs;
   }
 
+  private DocCollection getDocCollectionFromObjects(Map.Entry<String, Object> 
e, int zNodeVersion) {
+    @SuppressWarnings("rawtypes")
+    Map m = (Map) e.getValue();
+    Long creationTimeMillisFromClusterStatus = (Long) 
m.get("creationTimeMillis");

Review Comment:
   wow that's a long var name.  I think the `FromClusterStatus` suffix isn't 
needed; we aren't disambiguating multiple sources of creation time.



-- 
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

Reply via email to