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/04/29 19:54:09 UTC
[GitHub] [solr] madrob opened a new pull request #107: Address many Rawtypes warnings
madrob opened a new pull request #107:
URL: https://github.com/apache/solr/pull/107
I tried to split this into multiple commits after the fact, no guarantees that they will compile individually but they are meant to be reasonably logical groupings for 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.
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 #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-850660464
There was a merge conflict with some of Noble’s work on the XPath stuff so I wanted to wait for a merge of that, but I suppose there’s no major reason to hold up. I’ll take care of this on Tuesday
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-853062851
"Redundant suppression". AFAICT you can't restrict to particular suppressions, and be aware it's decisions may disagree with Gradle and thus we must adhere to Gradle. Many rawtypes suppressions were recommended to be removed yet they couldn't be because -- https://www.oreilly.com/library/view/learning-java-4th/9781449372477/ch08s10.html
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624363811
##########
File path: solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java
##########
@@ -253,7 +252,9 @@ public void writeResults(ResultContext ctx, JavaBinCodec codec) throws IOExcepti
createJavaBinCodec(callback, resolver).setWritableDocFields(resolver).marshal(rsp.getValues(), out);
try (InputStream in = out.toInputStream()) {
- return (NamedList<Object>) new JavaBinCodec(resolver).unmarshal(in);
+ @SuppressWarnings({"unchecked"})
+ NamedList<Object> resolved = (NamedList<Object>) new JavaBinCodec(resolver).unmarshal(in);
Review comment:
does a one-liner no longer work as it was?
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/ZkTestServer.java
##########
@@ -644,8 +644,10 @@ public void shutdown() throws IOException, InterruptedException {
while (true) {
try {
- zooThread.join();
- ObjectReleaseTracker.release(zooThread);
+ if (zooThread != null) {
Review comment:
belongs in another issue?
##########
File path: solr/core/src/java/org/apache/solr/handler/StreamHandler.java
##########
@@ -141,15 +141,21 @@ public static void addExpressiblePlugins(StreamFactory streamFactory, SolrCore c
}
public static class ExpressibleHolder extends PackagePluginHolder<Class<? extends Expressible>> {
+ private Class<? extends Expressible> clazz;
+
public ExpressibleHolder(PluginInfo info, SolrCore core, SolrConfig.SolrPluginInfo pluginMeta) {
super(info, core, pluginMeta);
}
- // WARNING: This is called from the super constructor, do not make assumptions about object state.
- // This will probably cause a really bad bug someday, I hope you are reading this under better circumstances.
+ @Override
Review comment:
The commit "potential class loading issue" is not related to the PR
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] fdalsotto commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
fdalsotto commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624238957
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -101,14 +101,14 @@ public EndPoint getEndPoint() {
* then absence of Api-s is silently ignored.
* @return list of discovered Api-s
*/
- public static List<Api> getApis(Class<? extends Object> theClass , Object obj, boolean allowEmpty) {
+ public static List<Api> getApis(Class<?> theClass , Object obj, boolean allowEmpty) {
Review comment:
would an interface and Class <Interface> not have better typing than Class<?>?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] fdalsotto commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
fdalsotto commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624238957
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -101,14 +101,14 @@ public EndPoint getEndPoint() {
* then absence of Api-s is silently ignored.
* @return list of discovered Api-s
*/
- public static List<Api> getApis(Class<? extends Object> theClass , Object obj, boolean allowEmpty) {
+ public static List<Api> getApis(Class<?> theClass , Object obj, boolean allowEmpty) {
Review comment:
would an interface and Class<Interface> not have better typing than Class<?>?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-830508062
> Several commits here are not about Rawtypes warnings and thus should be separated.
unchecked casts go hand in hand with raw types so I’m not splitting those out. The zoo thread bull check can be left out, sure. Is there more?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-852715572
I used IntelliJ's "inspections" to find cases where we no longer need to suppress certain warnings. There were false positives from precommit that I reverted back.
ClusterStateProvider.getClusterProperty was a curious case, where the generics there were not helpful and forced the need to suppress on implementing classes even if they don't override that method. So I removed generics there.
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624398704
##########
File path: solr/test-framework/src/java/org/apache/solr/cloud/ZkTestServer.java
##########
@@ -644,8 +644,10 @@ public void shutdown() throws IOException, InterruptedException {
while (true) {
try {
- zooThread.join();
- ObjectReleaseTracker.release(zooThread);
+ if (zooThread != null) {
Review comment:
This came up while I was trying to debug a failing test, but sure
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-830510083
> unchecked casts go hand in hand with raw types so I’m not splitting those out.
100% agree; I didn't mean to suggest otherwise!
An example: TestDistribPackageStore.assertResponseValues felt out of place. Maybe there are others. I was expecting this PR just to be about types/casts to remove some warnings. Thanks for 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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] fdalsotto commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
fdalsotto commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624238957
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -101,14 +101,14 @@ public EndPoint getEndPoint() {
* then absence of Api-s is silently ignored.
* @return list of discovered Api-s
*/
- public static List<Api> getApis(Class<? extends Object> theClass , Object obj, boolean allowEmpty) {
+ public static List<Api> getApis(Class<?> theClass , Object obj, boolean allowEmpty) {
Review comment:
would an interface and Class< Interface > not have better typing than Class<?>?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-830511061
That’s a bug I found because I changed an unchecked makeMap to map.of somewhere, so iteration order changed and I discovered that we were only asserting the first value in the map.
Sometimes when you remove warnings you find real bugs! If this wasn’t the case, I wouldn’t spend time on 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.
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 #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #107:
URL: https://github.com/apache/solr/pull/107
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-850658268
It'll be nice to see this changes merged... did you forget about this one @madrob?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-852982397
What is the IntelliJ inspection called? I tried to find it but couldn’t. :(
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob merged pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #107:
URL: https://github.com/apache/solr/pull/107
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob edited a comment on pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob edited a comment on pull request #107:
URL: https://github.com/apache/solr/pull/107#issuecomment-830508062
> Several commits here are not about Rawtypes warnings and thus should be separated.
unchecked casts go hand in hand with raw types so I’m not splitting those out. The zoo thread null check can be left out, sure. Is there more?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624398859
##########
File path: solr/core/src/java/org/apache/solr/handler/StreamHandler.java
##########
@@ -141,15 +141,21 @@ public static void addExpressiblePlugins(StreamFactory streamFactory, SolrCore c
}
public static class ExpressibleHolder extends PackagePluginHolder<Class<? extends Expressible>> {
+ private Class<? extends Expressible> clazz;
+
public ExpressibleHolder(PluginInfo info, SolrCore core, SolrConfig.SolrPluginInfo pluginMeta) {
super(info, core, pluginMeta);
}
- // WARNING: This is called from the super constructor, do not make assumptions about object state.
- // This will probably cause a really bad bug someday, I hope you are reading this under better circumstances.
+ @Override
Review comment:
It is, because it was a bug I introduced in this issue.
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624398459
##########
File path: solr/core/src/java/org/apache/solr/api/AnnotatedApi.java
##########
@@ -101,14 +101,14 @@ public EndPoint getEndPoint() {
* then absence of Api-s is silently ignored.
* @return list of discovered Api-s
*/
- public static List<Api> getApis(Class<? extends Object> theClass , Object obj, boolean allowEmpty) {
+ public static List<Api> getApis(Class<?> theClass , Object obj, boolean allowEmpty) {
Review comment:
Probably, but that’s a bigger design decision
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r643418718
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -622,15 +622,22 @@ public HttpCachingConfig getHttpCachingConfig() {
public static class HttpCachingConfig implements MapSerializable {
+ /**
+ * config xpath prefix for getting HTTP Caching options
+ */
+ private static final String CACHE_PRE
Review comment:
https://github.com/apache/solr/pull/107/files#diff-473fbcdab103c08461ad1b3c3bb1c6d56f1bcd16d6ce341d80855db2cb20a427R591
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624398602
##########
File path: solr/core/src/java/org/apache/solr/client/solrj/embedded/EmbeddedSolrServer.java
##########
@@ -253,7 +252,9 @@ public void writeResults(ResultContext ctx, JavaBinCodec codec) throws IOExcepti
createJavaBinCodec(callback, resolver).setWritableDocFields(resolver).marshal(rsp.getValues(), out);
try (InputStream in = out.toInputStream()) {
- return (NamedList<Object>) new JavaBinCodec(resolver).unmarshal(in);
+ @SuppressWarnings({"unchecked"})
+ NamedList<Object> resolved = (NamedList<Object>) new JavaBinCodec(resolver).unmarshal(in);
Review comment:
Can’t suppress the warning on a return statement.
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #107: Address many Rawtypes warnings
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #107:
URL: https://github.com/apache/solr/pull/107#discussion_r624417230
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -622,15 +622,22 @@ public HttpCachingConfig getHttpCachingConfig() {
public static class HttpCachingConfig implements MapSerializable {
+ /**
+ * config xpath prefix for getting HTTP Caching options
+ */
+ private static final String CACHE_PRE
Review comment:
I can't tell where this is used
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org