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 2021/06/26 05:40:49 UTC
[GitHub] [solr] madrob opened a new pull request #195: SOLR-15385 RawTypes Part IV, solr-core
madrob opened a new pull request #195:
URL: https://github.com/apache/solr/pull/195
https://issues.apache.org/jira/browse/SOLR-15385
This one is slightly bigger than the others, but I think the median change is also lower complexity. There were a lot of thornier issues that I decided to skip over completely for now in solr-core.
--
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] cpoerschke commented on a change in pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #195:
URL: https://github.com/apache/solr/pull/195#discussion_r659208737
##########
File path: solr/core/src/java/org/apache/solr/response/CSVResponseWriter.java
##########
@@ -318,29 +318,27 @@ public void close() throws IOException {
}
//NOTE: a document cannot currently contain another document
- @SuppressWarnings({"rawtypes"})
- List tmpList;
+ List<Object> tmpList;
Review comment:
unrelated to the changes in this pull request: could this be private and/or have a comment w.r.t. why it's not just a local variable in `writeSolrDocument` would be helpful
##########
File path: solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java
##########
@@ -442,23 +442,23 @@ public void modifyRequest(ResponseBuilder rb, SearchComponent who, ShardRequest
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
public void handleResponses(ResponseBuilder rb, ShardRequest sreq) {
if (!rb.doExpand) {
return;
}
if ((sreq.purpose & ShardRequest.PURPOSE_GET_FIELDS) != 0) {
SolrQueryRequest req = rb.req;
- NamedList expanded = (NamedList) req.getContext().get("expanded");
+ @SuppressWarnings("unchecked")
+ NamedList<Object> expanded = (NamedList<Object>) req.getContext().get("expanded");
if (expanded == null) {
- expanded = new SimpleOrderedMap();
+ expanded = new SimpleOrderedMap<>();
req.getContext().put("expanded", expanded);
}
for (ShardResponse srsp : sreq.responses) {
- NamedList response = srsp.getSolrResponse().getResponse();
- NamedList ex = (NamedList) response.get("expanded");
+ NamedList<Object> response = srsp.getSolrResponse().getResponse();
+ NamedList<?> ex = (NamedList<?>) response.get("expanded");
Review comment:
`NamedList<?>` here but `NamedList<Object>` above, both for `expanded`, is unexpected at a glance, not yet looked into possible reasons for it.
--
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] cpoerschke commented on pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #195:
URL: https://github.com/apache/solr/pull/195#issuecomment-869039643
> This one is slightly bigger than the others, but I think the median change is also lower complexity. There were a lot of thornier issues that I decided to skip over completely for now in solr-core.
Wow, I admire your stamina and patience with this cleanup effort! Thank you.
The sheer "Files changed 352" number made me wonder about a script to somehow split the review and tool assisted reviewing. And so I hacked up a script to help me, specifically looking for files that had _only_ removals and/or relocations of `@SuppressWarnings` and/or additions of `<Object>` or `<?>` or `<>` i.e. easily reviewed low complexity changes.
On the assumption that no one else has yet started to review here I'd like to add two commits to remove 254+5 files from the scope of this pull request (i.e. defer them to Part V) leaving 93 files here to review on the github UI whilst the tests are running locally.
--
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 merged pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #195:
URL: https://github.com/apache/solr/pull/195
--
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 pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #195:
URL: https://github.com/apache/solr/pull/195#issuecomment-869894976
@cpoerschke so this is the "easy" changes left in here, right? Do you want me to open a new PR for the remaining 254+5 files or did you mean to add them back here once you were done with the first batch?
--
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 change in pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #195:
URL: https://github.com/apache/solr/pull/195#discussion_r660007488
##########
File path: solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java
##########
@@ -442,23 +442,23 @@ public void modifyRequest(ResponseBuilder rb, SearchComponent who, ShardRequest
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
public void handleResponses(ResponseBuilder rb, ShardRequest sreq) {
if (!rb.doExpand) {
return;
}
if ((sreq.purpose & ShardRequest.PURPOSE_GET_FIELDS) != 0) {
SolrQueryRequest req = rb.req;
- NamedList expanded = (NamedList) req.getContext().get("expanded");
+ @SuppressWarnings("unchecked")
+ NamedList<Object> expanded = (NamedList<Object>) req.getContext().get("expanded");
if (expanded == null) {
- expanded = new SimpleOrderedMap();
+ expanded = new SimpleOrderedMap<>();
req.getContext().put("expanded", expanded);
}
for (ShardResponse srsp : sreq.responses) {
- NamedList response = srsp.getSolrResponse().getResponse();
- NamedList ex = (NamedList) response.get("expanded");
+ NamedList<Object> response = srsp.getSolrResponse().getResponse();
+ NamedList<?> ex = (NamedList<?>) response.get("expanded");
Review comment:
`getResponse()` returns `<Object>` so I went with that, `expanded` has items added to it (L465) so it can't be `<?>`. Meanwhile, we're just reading values out of `ex` so I was lazy and made it wildcard. I suppose we could do `NamedList<SolrDocumentList>` here because we cast it later but I was going for quantity over quality at some point 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.
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 change in pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #195:
URL: https://github.com/apache/solr/pull/195#discussion_r660007488
##########
File path: solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java
##########
@@ -442,23 +442,23 @@ public void modifyRequest(ResponseBuilder rb, SearchComponent who, ShardRequest
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
public void handleResponses(ResponseBuilder rb, ShardRequest sreq) {
if (!rb.doExpand) {
return;
}
if ((sreq.purpose & ShardRequest.PURPOSE_GET_FIELDS) != 0) {
SolrQueryRequest req = rb.req;
- NamedList expanded = (NamedList) req.getContext().get("expanded");
+ @SuppressWarnings("unchecked")
+ NamedList<Object> expanded = (NamedList<Object>) req.getContext().get("expanded");
if (expanded == null) {
- expanded = new SimpleOrderedMap();
+ expanded = new SimpleOrderedMap<>();
req.getContext().put("expanded", expanded);
}
for (ShardResponse srsp : sreq.responses) {
- NamedList response = srsp.getSolrResponse().getResponse();
- NamedList ex = (NamedList) response.get("expanded");
+ NamedList<Object> response = srsp.getSolrResponse().getResponse();
+ NamedList<?> ex = (NamedList<?>) response.get("expanded");
Review comment:
`getResponse()` returns `<Object>` so I went with that, `expanded` has items added to it (L465) so it can't be `<?>`. Meanwhile, we're just reading values out of `ex` so I was lazy and made it wildcard. I suppose we could do `NamedList<SolrDocumentList>` here because we cast it later but I was going for quantity over quality at some point 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.
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] cpoerschke commented on pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #195:
URL: https://github.com/apache/solr/pull/195#issuecomment-869944074
Merging of this first batch and then a new PR for rest would be my preference.
--
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 change in pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #195:
URL: https://github.com/apache/solr/pull/195#discussion_r659123757
##########
File path: solr/core/src/java/org/apache/solr/cloud/ZkController.java
##########
@@ -2627,8 +2626,7 @@ private static void ensureRegisteredSearcher(SolrCore core) throws InterruptedEx
}
registeredSearcher.decref();
} else {
- @SuppressWarnings({"rawtypes"})
- Future[] waitSearcher = new Future[1];
+ AtomicReference<Future<Void>> waitSearcher = new AtomicReference<>();
Review comment:
We can do this, or we can do Array.newInstance and cast like I've done in other places, no strong opinions although I do lean towards the Reference solution because it feels like the intent is clearer to me.
--
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] cpoerschke commented on pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #195:
URL: https://github.com/apache/solr/pull/195#issuecomment-869944074
Merging of this first batch and then a new PR for rest would be my preference.
--
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 merged pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #195:
URL: https://github.com/apache/solr/pull/195
--
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 pull request #195: SOLR-15385 RawTypes Part IV, solr-core
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #195:
URL: https://github.com/apache/solr/pull/195#issuecomment-869894976
@cpoerschke so this is the "easy" changes left in here, right? Do you want me to open a new PR for the remaining 254+5 files or did you mean to add them back here once you were done with the first batch?
--
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