You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/06/21 13:59:57 UTC
[GitHub] [solr] risdenk opened a new pull request, #912: WIP - Use up to JDK 11 features
risdenk opened a new pull request, #912:
URL: https://github.com/apache/solr/pull/912
https://issues.apache.org/jira/browse/SOLR-XXXXX
# Description
* Use enhanced for loops
* Avoid type declaration when `<>` can be used
* Use `computeIfAbsent`
* use `Objects.requireNonNullElse` to simplify if/else statements
* Simplify lambda statements
* `Collections.sort` -> `list.sort`
* Use string `.contains` instead of checking `indexOf`
* Avoid unnecessary boxing/unboxing
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #912: WIP - Use up to JDK 11 features
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #912:
URL: https://github.com/apache/solr/pull/912#issuecomment-1165760542
@epugh mostly an FYI since you were going through intelli warnings - these are also intellij warnings/findings behind the scenes.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a diff in pull request #912: WIP - Use up to JDK 11 features
Posted by GitBox <gi...@apache.org>.
madrob commented on code in PR #912:
URL: https://github.com/apache/solr/pull/912#discussion_r902952991
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -39,8 +39,7 @@ class SolrCores {
private final CoreContainer container;
- private Set<String> currentlyLoadingCores =
- Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
+ private Set<String> currentlyLoadingCores = Collections.newSetFromMap(new ConcurrentHashMap<>());
Review Comment:
```suggestion
private Set<String> currentlyLoadingCores = ConcurrentHashMap.newKeySet();
```
##########
solr/core/src/java/org/apache/solr/filestore/PackageStoreAPI.java:
##########
@@ -212,12 +212,10 @@ public void upload(SolrQueryRequest req, SolrQueryResponse rsp) {
} catch (IOException e) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
}
- } catch (InterruptedException e) {
- log.error("Unexpected error", e);
} catch (KeeperException.NodeExistsException e) {
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR, "A write is already in process , try later");
- } catch (KeeperException e) {
+ } catch (InterruptedException | KeeperException e) {
Review Comment:
should check for interrupted status, which we weren't doing before either unfortunately.
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -676,10 +675,7 @@ boolean printTree(JSONWriter json, String path) throws IOException {
}
first = false;
}
- } catch (KeeperException e) {
- writeError(500, e.toString());
- return false;
- } catch (InterruptedException e) {
+ } catch (KeeperException | InterruptedException e) {
Review Comment:
check interrupted
##########
solr/core/src/java/org/apache/solr/handler/component/FieldFacetStats.java:
##########
@@ -114,16 +114,11 @@ public boolean facetTermNum(int docID, int statsTermNum) throws IOException {
key = topLevelSortedValues.lookupOrd(term).utf8ToString();
}
while (facetStatsTerms.size() <= statsTermNum) {
- facetStatsTerms.add(new HashMap<String, Integer>());
+ facetStatsTerms.add(new HashMap<>());
}
final Map<String, Integer> statsTermCounts = facetStatsTerms.get(statsTermNum);
- Integer statsTermCount = statsTermCounts.get(key);
- if (statsTermCount == null) {
- statsTermCounts.put(key, 1);
- } else {
- statsTermCounts.put(key, statsTermCount + 1);
- }
+ statsTermCounts.merge(key, 1, Integer::sum);
Review Comment:
I feel like there might be a better data structure to use for this, but probably can be a separate issue.
##########
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java:
##########
@@ -760,10 +756,7 @@ boolean printZnode(JSONWriter json, String path) throws IOException {
}
json.endObject();
- } catch (KeeperException e) {
- writeError(500, e.toString());
- return false;
- } catch (InterruptedException e) {
+ } catch (KeeperException | InterruptedException e) {
Review Comment:
check interrupted
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1324,7 +1324,7 @@ public void initializeMetrics(SolrMetricsContext parentContext, String scope) {
"coreName",
Category.CORE.toString());
parentContext.gauge(() -> startTime, true, "startTime", Category.CORE.toString());
- parentContext.gauge(() -> getOpenCount(), true, "refCount", Category.CORE.toString());
+ parentContext.gauge(this::getOpenCount, true, "refCount", Category.CORE.toString());
Review Comment:
I like the consistency with the lines above and below here, feels easier to understand if they're all the same.
##########
solr/core/src/java/org/apache/solr/cluster/placement/impl/AttributeFetcherImpl.java:
##########
@@ -185,20 +185,18 @@ public AttributeValues fetchAttributes() {
.getReplicaMetricsBuilders()
.computeIfAbsent(
replica.getName(),
- n ->
- new CollectionMetricsBuilder
- .ReplicaMetricsBuilder(n));
+ CollectionMetricsBuilder.ReplicaMetricsBuilder
+ ::new);
Review Comment:
This line break is so awkward.
##########
solr/core/src/java/org/apache/solr/handler/BlobHandler.java:
##########
@@ -207,24 +206,21 @@ public void handleRequestBody(final SolrQueryRequest req, SolrQueryResponse rsp)
if (docs.totalHits.value > 0) {
rsp.add(
ReplicationHandler.FILE_STREAM,
- new SolrCore.RawWriter() {
-
- @Override
- public void write(OutputStream os) throws IOException {
- Document doc = req.getSearcher().doc(docs.scoreDocs[0].doc);
- IndexableField sf = doc.getField("blob");
- FieldType fieldType = req.getSchema().getField("blob").getType();
- ByteBuffer buf = (ByteBuffer) fieldType.toObject(sf);
- if (buf == null) {
- // should never happen unless a user wrote this document directly
- throw new SolrException(
- SolrException.ErrorCode.NOT_FOUND,
- "Invalid document . No field called blob");
- } else {
- os.write(buf.array(), buf.arrayOffset(), buf.limit());
- }
- }
- });
+ (SolrCore.RawWriter)
Review Comment:
This looks like it actually increases the code indent, would revert.
##########
solr/core/src/java/org/apache/solr/handler/admin/SegmentsInfoRequestHandler.java:
##########
@@ -256,18 +256,9 @@ private SimpleOrderedMap<Object> getSegmentInfo(
size = dir.fileLength(f);
} catch (IOException e) {
}
- return new Pair<String, Long>(f, size);
- })
- .sorted(
- (p1, p2) -> {
- if (p1.second() > p2.second()) {
- return -1;
- } else if (p1.second() < p2.second()) {
- return 1;
- } else {
- return 0;
- }
+ return new Pair<>(f, size);
})
+ .sorted((p1, p2) -> p2.second().compareTo(p1.second()))
Review Comment:
Add a comment that this is a reverse sort.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #912: WIP - Use up to JDK 11 features
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #912:
URL: https://github.com/apache/solr/pull/912#issuecomment-1163147648
Thanks @madrob for the quick review. I threw it up to review myself and wasn't expecting comments yet :) I agree with your findings and many probably repeat throughout. I'll clean it up over the next few days.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #912: WIP - Use up to JDK 11 features
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #912:
URL: https://github.com/apache/solr/pull/912#discussion_r902723580
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/eval/RecursiveEvaluator.java:
##########
@@ -180,10 +180,9 @@ public RecursiveEvaluator(
Set<String> namedParameters =
factory.getNamedOperands(expression).stream()
- .map(param -> param.getName())
+ .map(StreamExpressionNamedParameter::getName)
.collect(Collectors.toSet());
- long ignorableCount =
- ignoredNamedParameters.stream().filter(name -> namedParameters.contains(name)).count();
+ long ignorableCount = ignoredNamedParameters.stream().filter(namedParameters::contains).count();
Review Comment:
*[UnusedVariable](https://errorprone.info/bugpattern/UnusedVariable):* The local variable 'ignorableCount' is never read.
Reply with *"**@sonatype-lift help**"* for more info.
Reply with *"**@sonatype-lift ignore**"* to tell LiftBot to leave out the above finding from this PR.
Reply with *"**@sonatype-lift ignoreall**"* to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to **refresh** the page to see its response. [Click here](https://help.sonatype.com/lift/talking-to-lift) to get to know more about LiftBot commands.
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=281932877&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=281932877&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=281932877&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=281932877&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=281932877&lift_comment_rating=5) ]
##########
solr/modules/hadoop-auth/src/java/org/apache/solr/security/hadoop/HadoopAuthFilter.java:
##########
@@ -215,7 +210,7 @@ public List<ACL> getAclForPath(String path) {
}
private List<AuthInfo> createAuthInfo(SolrZkClient zkClient) {
- List<AuthInfo> ret = new LinkedList<AuthInfo>();
+ List<AuthInfo> ret = new LinkedList<>();
Review Comment:
*[JdkObsolete](https://errorprone.info/bugpattern/JdkObsolete):* It is very rare for LinkedList to out-perform ArrayList or ArrayDeque. Avoid it unless you're willing to invest a lot of time into benchmarking. Caveat: LinkedList supports null elements, but ArrayDeque does not.
```suggestion
List<AuthInfo> ret = new ArrayList<>();
```
Reply with *"**@sonatype-lift help**"* for more info.
Reply with *"**@sonatype-lift ignore**"* to tell LiftBot to leave out the above finding from this PR.
Reply with *"**@sonatype-lift ignoreall**"* to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to **refresh** the page to see its response. [Click here](https://help.sonatype.com/lift/talking-to-lift) to get to know more about LiftBot commands.
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=281932891&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=281932891&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=281932891&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=281932891&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=281932891&lift_comment_rating=5) ]
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #912: SOLR-16318: WIP - Use up to JDK 11 features
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #912:
URL: https://github.com/apache/solr/pull/912#issuecomment-1279400882
This is being broken into pieces slowly.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #912: WIP - Use up to JDK 11 features
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #912:
URL: https://github.com/apache/solr/pull/912#discussion_r903793244
##########
solr/core/src/java/org/apache/solr/filestore/PackageStoreAPI.java:
##########
@@ -212,12 +212,10 @@ public void upload(SolrQueryRequest req, SolrQueryResponse rsp) {
} catch (IOException e) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e);
}
- } catch (InterruptedException e) {
- log.error("Unexpected error", e);
} catch (KeeperException.NodeExistsException e) {
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR, "A write is already in process , try later");
- } catch (KeeperException e) {
+ } catch (InterruptedException | KeeperException e) {
Review Comment:
Thanks yea I noticed this as well. You are quick to review :) - I threw it up so I could more easily look at the changes too.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #912: WIP - Use up to JDK 11 features
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #912:
URL: https://github.com/apache/solr/pull/912#issuecomment-1165759795
I am not really up on the new stuff in JDK 11 @risdenk so probably not the best person to review... ;-)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk closed pull request #912: SOLR-16318: WIP - Use up to JDK 11 features
Posted by GitBox <gi...@apache.org>.
risdenk closed pull request #912: SOLR-16318: WIP - Use up to JDK 11 features
URL: https://github.com/apache/solr/pull/912
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org