You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/09/01 16:31:05 UTC

[GitHub] [lucene-solr] megancarey opened a new pull request #1592: SOLR-14579 First pass at dismantling Utils

megancarey opened a new pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592


   <!--
   _(If you are a project committer then you may remove some/all of the following template.)_
   
   Before creating a pull request, please file an issue in the ASF Jira system for Lucene or Solr:
   
   * https://issues.apache.org/jira/projects/LUCENE
   * https://issues.apache.org/jira/projects/SOLR
   
   You will need to create an account in Jira in order to create an issue.
   
   The title of the PR should reference the Jira issue number in the form:
   
   * LUCENE-####: <short description of problem or changes>
   * SOLR-####: <short description of problem or changes>
   
   LUCENE and SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
   
   Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
   
   
   # Description
   
   Eliminating warnings due to static Functions in the Utils class within Solr. 
   
   # Solution
   
   Removed a few Function variables from the Utils class which were adding little value and causing warnings throughout the code. Removed a handful of unused variables, etc. to reduce warnings as well.
   
   # Tests
   
   I ran `ant test` from the solr directory, as all of the changes in this PR are limited to Solr.
   
   # Checklist
   
   Please review the following and check all that apply:
   
   - [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
   - [x] I have created a Jira issue and added the issue ID to my pull request title.
   - [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
   - [x] I have developed this patch against the `master` branch.
   - [x] I have run `ant precommit` and the appropriate test suite.
   - [ ] I have added tests for my changes.
   - [ ] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
   


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

[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#discussion_r443887161



##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
##########
@@ -351,10 +340,10 @@ public void simSetClusterState(ClusterState initialState) throws Exception {
         // DocCollection will be created later
         collectionsStatesRef.put(dc.getName(), new CachedCollectionRef(dc.getName(), dc.getZNodeVersion()));
         collProperties.computeIfAbsent(dc.getName(), name -> new ConcurrentHashMap<>()).putAll(dc.getProperties());
-        opDelays.computeIfAbsent(dc.getName(), Utils.NEW_HASHMAP_FUN).putAll(defaultOpDelays);
+        opDelays.computeIfAbsent(dc.getName(), o -> new HashMap<String,Long>()).putAll(defaultOpDelays);

Review comment:
       `putIfAbsent()` is wore than `computeIfAbsent()` 
   
   the former actually creates an object even before the method is called. The latter does it only after if the key has no value




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

[GitHub] [lucene-solr] dsmiley commented on pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#issuecomment-647082412


   +1
   Can you see who added these and get their attention here for their opinion?.


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

[GitHub] [lucene-solr] ErickErickson commented on pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
ErickErickson commented on pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#issuecomment-684888633


   See the JIRA, wound up only commenting the static lambdas in Utils.java


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

[GitHub] [lucene-solr] noblepaul commented on pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
noblepaul commented on pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#issuecomment-647378856


   -1
   
   don't commit this


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

[GitHub] [lucene-solr] ErickErickson closed pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
ErickErickson closed pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592


   


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

[GitHub] [lucene-solr] megancarey commented on a change in pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
megancarey commented on a change in pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#discussion_r443788954



##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
##########
@@ -351,10 +340,10 @@ public void simSetClusterState(ClusterState initialState) throws Exception {
         // DocCollection will be created later
         collectionsStatesRef.put(dc.getName(), new CachedCollectionRef(dc.getName(), dc.getZNodeVersion()));
         collProperties.computeIfAbsent(dc.getName(), name -> new ConcurrentHashMap<>()).putAll(dc.getProperties());
-        opDelays.computeIfAbsent(dc.getName(), Utils.NEW_HASHMAP_FUN).putAll(defaultOpDelays);
+        opDelays.computeIfAbsent(dc.getName(), o -> new HashMap<String,Long>()).putAll(defaultOpDelays);

Review comment:
       Should we be using Map.putIfAbsent instead of compute? The value mapped to the key doesn't actually use the value, so I don't believe we need to do computeIfAbsent, and if lambda functions are the issue, putIfAbsent will remove that altogether.
   
   I'll try this out and run tests to see if that's an acceptable option, and report back in a bit.




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

[GitHub] [lucene-solr] megancarey commented on a change in pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
megancarey commented on a change in pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#discussion_r444544150



##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/AutoScalingHandler.java
##########
@@ -542,7 +542,7 @@ private AutoScalingConfig handleSetTrigger(SolrQueryRequest req, SolrQueryRespon
     String eventTypeStr = op.getStr(EVENT);
 
     if (op.hasError()) return currentConfig;
-    TriggerEventType eventType = TriggerEventType.valueOf(eventTypeStr.trim().toUpperCase(Locale.ROOT));
+    TriggerEventType.valueOf(eventTypeStr.trim().toUpperCase(Locale.ROOT));

Review comment:
       If the eventTypeStr is an invalid event type, it will throw an exception that there are no matches in TriggerEventType enum. I'll add a comment so the reason for this line is more obvious.




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

[GitHub] [lucene-solr] madrob commented on a change in pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#discussion_r444325409



##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
##########
@@ -191,7 +190,7 @@ public DocCollection getColl() throws InterruptedException, IOException {
                 }
                 Map<String, Object> props;
                 synchronized (ri) {
-                  props = new HashMap<>(ri.getVariables());
+                  props = new HashMap<String,Object>(ri.getVariables());

Review comment:
       I don't think we need this?

##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/AutoScalingHandler.java
##########
@@ -542,7 +542,7 @@ private AutoScalingConfig handleSetTrigger(SolrQueryRequest req, SolrQueryRespon
     String eventTypeStr = op.getStr(EVENT);
 
     if (op.hasError()) return currentConfig;
-    TriggerEventType eventType = TriggerEventType.valueOf(eventTypeStr.trim().toUpperCase(Locale.ROOT));
+    TriggerEventType.valueOf(eventTypeStr.trim().toUpperCase(Locale.ROOT));

Review comment:
       I think you can remove the whole line.

##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
##########
@@ -2293,15 +2274,14 @@ public void simSetShardValue(String collection, String shard, String key, Object
     }
   }
 
-  @SuppressWarnings({"unchecked"})
   public void simSetReplicaValues(String node, Map<String, Map<String, List<ReplicaInfo>>> source, boolean overwrite) {
     List<ReplicaInfo> infos = nodeReplicaMap.get(node);
     if (infos == null) {
       throw new RuntimeException("Node not present: " + node);
     }
     // core_node_name is not unique across collections
-    Map<String, Map<String, ReplicaInfo>> infoMap = new HashMap<>();
-    infos.forEach(ri -> infoMap.computeIfAbsent(ri.getCollection(), Utils.NEW_HASHMAP_FUN).put(ri.getName(), ri));
+    Map<String, Map<String, ReplicaInfo>> infoMap = new HashMap<String, Map<String, ReplicaInfo>>();

Review comment:
       `<>` is fine here.




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

[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#discussion_r443887161



##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
##########
@@ -351,10 +340,10 @@ public void simSetClusterState(ClusterState initialState) throws Exception {
         // DocCollection will be created later
         collectionsStatesRef.put(dc.getName(), new CachedCollectionRef(dc.getName(), dc.getZNodeVersion()));
         collProperties.computeIfAbsent(dc.getName(), name -> new ConcurrentHashMap<>()).putAll(dc.getProperties());
-        opDelays.computeIfAbsent(dc.getName(), Utils.NEW_HASHMAP_FUN).putAll(defaultOpDelays);
+        opDelays.computeIfAbsent(dc.getName(), o -> new HashMap<String,Long>()).putAll(defaultOpDelays);

Review comment:
       `putIfAbsent()` is wore than `computeIfAbsent()` 
   
   the former actually create an object even before the method is called. The latter does it only after if the key has no value




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

[GitHub] [lucene-solr] msokolov commented on a change in pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
msokolov commented on a change in pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#discussion_r443535745



##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
##########
@@ -351,10 +340,10 @@ public void simSetClusterState(ClusterState initialState) throws Exception {
         // DocCollection will be created later
         collectionsStatesRef.put(dc.getName(), new CachedCollectionRef(dc.getName(), dc.getZNodeVersion()));
         collProperties.computeIfAbsent(dc.getName(), name -> new ConcurrentHashMap<>()).putAll(dc.getProperties());
-        opDelays.computeIfAbsent(dc.getName(), Utils.NEW_HASHMAP_FUN).putAll(defaultOpDelays);
+        opDelays.computeIfAbsent(dc.getName(), o -> new HashMap<String,Long>()).putAll(defaultOpDelays);

Review comment:
       Is this some kind of tight inner loop where we need to be concerned about squeezing out every last gram of garbage? I'm not super familiar with this code, but it rather looks like a one-time initialization that will run once for each collection, so really it's a handful of objects we're talking about I think. Seems like a case of premature optimization, not really worth a warning from the compiler, IMO 




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

[GitHub] [lucene-solr] noblepaul commented on pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
noblepaul commented on pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#issuecomment-647379273


   `o -> new HashMap<String,Long>()`
   
   this creates a new lambda class. 
   OTOH `Utils.NEW_HASHMAP_FUN` is created once and reused everywhere.
   
   


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

[GitHub] [lucene-solr] ErickErickson commented on pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
ErickErickson commented on pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#issuecomment-687632455


   Fixed up as per Uwe's criticisms.


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

[GitHub] [lucene-solr] uschindler commented on pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
uschindler commented on pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#issuecomment-684982042


   This pull request is fine, please apply it. The comments by @noblepaul are not true, there's no instances or classes generated on every call. See https://issues.apache.org/jira/browse/SOLR-14579?focusedCommentId=17188582&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17188582


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

[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#discussion_r443408369



##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
##########
@@ -351,10 +340,10 @@ public void simSetClusterState(ClusterState initialState) throws Exception {
         // DocCollection will be created later
         collectionsStatesRef.put(dc.getName(), new CachedCollectionRef(dc.getName(), dc.getZNodeVersion()));
         collProperties.computeIfAbsent(dc.getName(), name -> new ConcurrentHashMap<>()).putAll(dc.getProperties());
-        opDelays.computeIfAbsent(dc.getName(), Utils.NEW_HASHMAP_FUN).putAll(defaultOpDelays);
+        opDelays.computeIfAbsent(dc.getName(), o -> new HashMap<String,Long>()).putAll(defaultOpDelays);

Review comment:
       This was added for a purpose.
   `o -> new HashMap<String,Long>()` 
   
   this creates a new lambda class. 
   OTOH `Utils.NEW_HASHMAP_FUN` is created once and reused everywhere
   
   
   




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

[GitHub] [lucene-solr] megancarey commented on a change in pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
megancarey commented on a change in pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#discussion_r443788954



##########
File path: solr/core/src/java/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
##########
@@ -351,10 +340,10 @@ public void simSetClusterState(ClusterState initialState) throws Exception {
         // DocCollection will be created later
         collectionsStatesRef.put(dc.getName(), new CachedCollectionRef(dc.getName(), dc.getZNodeVersion()));
         collProperties.computeIfAbsent(dc.getName(), name -> new ConcurrentHashMap<>()).putAll(dc.getProperties());
-        opDelays.computeIfAbsent(dc.getName(), Utils.NEW_HASHMAP_FUN).putAll(defaultOpDelays);
+        opDelays.computeIfAbsent(dc.getName(), o -> new HashMap<String,Long>()).putAll(defaultOpDelays);

Review comment:
       Should we be using `map.putIfAbsent` instead of compute? The value mapped to the key doesn't actually use the key, so I don't believe we need to use `computeIfAbsent`/lambda functions at all.
   
   I'll try this out and run tests to see if that's an acceptable option, and report back in a bit.




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

[GitHub] [lucene-solr] megancarey commented on pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
megancarey commented on pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592#issuecomment-647226554


   cc @noblepaul - It looks like you authored the Utils class. Can you please take a look at this change when you get a chance?


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

[GitHub] [lucene-solr] ErickErickson closed pull request #1592: SOLR-14579 First pass at dismantling Utils

Posted by GitBox <gi...@apache.org>.
ErickErickson closed pull request #1592:
URL: https://github.com/apache/lucene-solr/pull/1592


   


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