[GitHub] [lucene-solr] murblanc commented on a change in pull request #1542: SOLR-14347: fix cached session update to not depend on Zookeeper state

[GitBox <gi...@apache.org>]

murblanc commented on a change in pull request #1542:
URL: https://github.com/apache/lucene-solr/pull/1542#discussion_r438038559



##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java
##########
@@ -644,15 +643,13 @@ public Session cloneToNewSession(SolrCloudManager cloudManager) {
       List<Clause> expandedClauses = new ArrayList<>(this.expandedClauses);
 
       List<Row> matrix = new ArrayList<>(nodes.size());
+      Map<String, Row> copyNodes = new HashMap<>();
+      for (Row oldRow: this.matrix) {

Review comment:
       Tradeoff with this one time matrix walk and copy is that we copy all nodes, even those that might not be needed (because not present in parameter `nodes`). I do like the approach though it's cleaner than the code I wrote.
   
   Maybe keep this initial loop but just store Row references, then do the copy below when newRow is needed? (there's already an if bloc when it's null, so copy action can easily be placed in else bloc).




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org