You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "risdenk (via GitHub)" <gi...@apache.org> on 2023/03/28 19:16:32 UTC
[GitHub] [solr] risdenk opened a new pull request, #1501: More guava cleanup
risdenk opened a new pull request, #1501:
URL: https://github.com/apache/solr/pull/1501
https://issues.apache.org/jira/browse/SOLR-XXXXX
--
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 #1501: More guava cleanup
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1501:
URL: https://github.com/apache/solr/pull/1501#discussion_r1151106048
##########
solr/core/src/java/org/apache/solr/logging/jul/JulWatcher.java:
##########
@@ -157,7 +158,9 @@ public SolrDocument toSolrDocument(LogRecord event) {
doc.setField("message", event.getMessage().toString());
Throwable t = event.getThrown();
if (t != null) {
- doc.setField("trace", Throwables.getStackTraceAsString(t));
+ StringWriter trace = new StringWriter();
+ t.printStackTrace(new PrintWriter(trace));
Review Comment:
<picture><img alt="0% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/0/display.svg"></picture>
<b>*[INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE](https://find-sec-bugs.github.io/bugs.htm#INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE):</b>* Possible information exposure through an error message
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=462817520&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=462817520&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=462817520&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=462817520&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=462817520&lift_comment_rating=5)â]
##########
solr/core/src/java/org/apache/solr/logging/log4j2/Log4j2Watcher.java:
##########
@@ -272,7 +273,11 @@ public SolrDocument toSolrDocument(LogEvent event) {
Message message = event.getMessage();
doc.setField("message", message.getFormattedMessage());
Throwable t = message.getThrowable();
- if (t != null) doc.setField("trace", Throwables.getStackTraceAsString(t));
+ if (t != null) {
+ StringWriter trace = new StringWriter();
+ t.printStackTrace(new PrintWriter(trace));
Review Comment:
<picture><img alt="0% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/0/display.svg"></picture>
<b>*[INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE](https://find-sec-bugs.github.io/bugs.htm#INFORMATION_EXPOSURE_THROUGH_AN_ERROR_MESSAGE):</b>* Possible information exposure through an error message
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=462817587&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=462817587&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=462817587&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=462817587&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=462817587&lift_comment_rating=5)â]
##########
solr/core/src/java/org/apache/solr/response/transform/ChildDocTransformer.java:
##########
@@ -258,14 +256,14 @@ public void transform(SolrDocument rootDoc, int rootDocId) {
}
private static void addChildrenToParent(
- SolrDocument parent, Multimap<String, SolrDocument> children) {
+ SolrDocument parent, Map<String, List<SolrDocument>> children) {
for (String childLabel : children.keySet()) {
addChildrenToParent(parent, children.get(childLabel), childLabel);
Review Comment:
<picture><img alt="10% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/10/display.svg"></picture>
<b>*INEFFICIENT_KEYSET_ITERATOR:</b>* Accessing a value using a key that was retrieved from a `keySet` iterator. It is more efficient to use an iterator on the `entrySet` of the map, avoiding the extra `HashMap.get(key)` lookup.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=462817966&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=462817966&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=462817966&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=462817966&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=462817966&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] HoustonPutman commented on pull request #1501: SOLR-16713: Replace Guava usages with pure Java (part 2)
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1501:
URL: https://github.com/apache/solr/pull/1501#issuecomment-1492444801
> so added a simple helper to StrUtils to do that conversion.
Ahh ok I get that, but why do all of the readers need to be wrapped by `BufferedReader` first?
--
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 #1501: SOLR-16713: Replace Guava usages with pure Java (part 2)
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1501:
URL: https://github.com/apache/solr/pull/1501#issuecomment-1492432576
> Why do we want to get rid of `IOUtils.toString()`
This is a specific version of IOUtils.toString() specifically one that deals with Java Reader. Basically limits usage of commons-io. Guava and commons-io both had methods converting a Java Reader to a String so added a simple helper to StrUtils to do that conversion.
--
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 #1501: SOLR-16713: Replace Guava usages with pure Java (part 2)
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1501:
URL: https://github.com/apache/solr/pull/1501#discussion_r1155113182
##########
solr/solrj/src/java/org/apache/solr/common/util/StrUtils.java:
##########
@@ -363,4 +366,16 @@ public static String formatString(String pattern, Object... args) {
public static boolean isNullOrEmpty(String string) {
return string == null || string.isEmpty();
}
+
+ public static String stringFromReader(Reader inReader) throws IOException {
+ try (Reader reader = new BufferedReader(inReader)) {
+ char[] arr = new char[8 * 1024];
+ StringBuilder buffer = new StringBuilder();
Review Comment:
thanks looks a lot nicer - fixed in b8e3fd33d327d0fd0c57ea226a4ea90764c3298f
--
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] dsmiley commented on a diff in pull request #1501: SOLR-16713: Replace Guava usages with pure Java (part 2)
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1501:
URL: https://github.com/apache/solr/pull/1501#discussion_r1155050465
##########
solr/solrj/src/java/org/apache/solr/common/util/StrUtils.java:
##########
@@ -363,4 +366,16 @@ public static String formatString(String pattern, Object... args) {
public static boolean isNullOrEmpty(String string) {
return string == null || string.isEmpty();
}
+
+ public static String stringFromReader(Reader inReader) throws IOException {
+ try (Reader reader = new BufferedReader(inReader)) {
+ char[] arr = new char[8 * 1024];
+ StringBuilder buffer = new StringBuilder();
Review Comment:
triple buffering?!
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactory.java:
##########
@@ -123,20 +123,23 @@ public List<PlacementPlan> computePlacements(
// replicas on nodes with less cores first. We only need totalReplicasPerShard nodes given
// that's the number of replicas to place. We assign based on the passed
// nodeEntriesToAssign list so the right nodes get replicas.
- ArrayList<Map.Entry<Integer, Node>> nodeEntriesToAssign =
+ List<Map.Entry<Integer, Node>> nodeEntriesToAssign =
new ArrayList<>(totalReplicasPerShard);
- Iterator<Map.Entry<Integer, Node>> treeIterator = nodesByCores.entries().iterator();
+ Iterator<Map.Entry<Integer, Node>> treeIterator =
+ nodesByCores.entrySet().stream()
+ .flatMap(e -> e.getValue().stream().map(n -> Map.entry(e.getKey(), n)))
+ .iterator();
for (int i = 0; i < totalReplicasPerShard; i++) {
Review Comment:
Since you are adding the use of streams above, you might as well remove the need for an iterator and for loop as well. use `Stream.limit(totalReplicasPerShard).collect(Collectors.toList())` and then the whole concoction becomes rather satisfying IMO.
##########
solr/solrj/src/java/org/apache/solr/common/util/StrUtils.java:
##########
@@ -363,4 +366,16 @@ public static String formatString(String pattern, Object... args) {
public static boolean isNullOrEmpty(String string) {
return string == null || string.isEmpty();
}
+
+ public static String stringFromReader(Reader inReader) throws IOException {
+ try (Reader reader = new BufferedReader(inReader)) {
+ char[] arr = new char[8 * 1024];
+ StringBuilder buffer = new StringBuilder();
Review Comment:
instead I recommend inReader.transferTo(StringWriter)
--
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 #1501: More guava cleanup
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1501:
URL: https://github.com/apache/solr/pull/1501#discussion_r1152023909
##########
solr/core/src/java/org/apache/solr/logging/jul/JulWatcher.java:
##########
@@ -157,7 +158,9 @@ public SolrDocument toSolrDocument(LogRecord event) {
doc.setField("message", event.getMessage().toString());
Throwable t = event.getThrown();
if (t != null) {
- doc.setField("trace", Throwables.getStackTraceAsString(t));
+ StringWriter trace = new StringWriter();
+ t.printStackTrace(new PrintWriter(trace));
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
##########
solr/core/src/java/org/apache/solr/logging/log4j2/Log4j2Watcher.java:
##########
@@ -272,7 +273,11 @@ public SolrDocument toSolrDocument(LogEvent event) {
Message message = event.getMessage();
doc.setField("message", message.getFormattedMessage());
Throwable t = message.getThrowable();
- if (t != null) doc.setField("trace", Throwables.getStackTraceAsString(t));
+ if (t != null) {
+ StringWriter trace = new StringWriter();
+ t.printStackTrace(new PrintWriter(trace));
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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 #1501: More guava cleanup
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1501:
URL: https://github.com/apache/solr/pull/1501#discussion_r1152023763
##########
solr/core/src/java/org/apache/solr/logging/jul/JulWatcher.java:
##########
@@ -157,7 +158,9 @@ public SolrDocument toSolrDocument(LogRecord event) {
doc.setField("message", event.getMessage().toString());
Throwable t = event.getThrown();
if (t != null) {
- doc.setField("trace", Throwables.getStackTraceAsString(t));
+ StringWriter trace = new StringWriter();
+ t.printStackTrace(new PrintWriter(trace));
Review Comment:
@sonatype-lift ignore
##########
solr/core/src/java/org/apache/solr/logging/log4j2/Log4j2Watcher.java:
##########
@@ -272,7 +273,11 @@ public SolrDocument toSolrDocument(LogEvent event) {
Message message = event.getMessage();
doc.setField("message", message.getFormattedMessage());
Throwable t = message.getThrowable();
- if (t != null) doc.setField("trace", Throwables.getStackTraceAsString(t));
+ if (t != null) {
+ StringWriter trace = new StringWriter();
+ t.printStackTrace(new PrintWriter(trace));
Review Comment:
@sonatype-lift ignore
--
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 merged pull request #1501: SOLR-16713: Replace Guava usages with pure Java (part 2)
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk merged PR #1501:
URL: https://github.com/apache/solr/pull/1501
--
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] HoustonPutman commented on pull request #1501: SOLR-16713: Replace Guava usages with pure Java (part 2)
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1501:
URL: https://github.com/apache/solr/pull/1501#issuecomment-1492423646
Why do we want to get rid of `IOUtils.toString()`
--
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 #1501: SOLR-16713: Replace Guava usages with pure Java (part 2)
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1501:
URL: https://github.com/apache/solr/pull/1501#issuecomment-1492460113
> Ahh ok I get that, but why do all of the readers need to be wrapped by BufferedReader first?
yea so they don't that is left over from a previous iteration where I was using BufferedReader.lines() I can fix that.
--
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 #1501: SOLR-16713: Replace Guava usages with pure Java (part 2)
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1501:
URL: https://github.com/apache/solr/pull/1501#discussion_r1155113686
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/MinimizeCoresPlacementFactory.java:
##########
@@ -123,20 +123,23 @@ public List<PlacementPlan> computePlacements(
// replicas on nodes with less cores first. We only need totalReplicasPerShard nodes given
// that's the number of replicas to place. We assign based on the passed
// nodeEntriesToAssign list so the right nodes get replicas.
- ArrayList<Map.Entry<Integer, Node>> nodeEntriesToAssign =
+ List<Map.Entry<Integer, Node>> nodeEntriesToAssign =
new ArrayList<>(totalReplicasPerShard);
- Iterator<Map.Entry<Integer, Node>> treeIterator = nodesByCores.entries().iterator();
+ Iterator<Map.Entry<Integer, Node>> treeIterator =
+ nodesByCores.entrySet().stream()
+ .flatMap(e -> e.getValue().stream().map(n -> Map.entry(e.getKey(), n)))
+ .iterator();
for (int i = 0; i < totalReplicasPerShard; i++) {
Review Comment:
Yes much cleaner - fixed in 983a8306b78c1fa0237eb5661c2a8147a58a6a5d
--
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