SOLR-16427: Evaluate and fix errorprone rules - part 2

[risdenk]

trafficstars

https://issues.apache.org/jira/browse/SOLR-16427

Builds on https://github.com/apache/solr/pull/1037

Enables the following errorprone rules:

• AlmostJavadoc - Fixed a few javadoc issues
• BadInstanceof - this is most of the changes
• DoubleBraceInitialization - Most of these moved to List.of, Map.of, or Set.of
• InvalidInlineTag - Fixed a few javadoc issues
• InvalidParam - Fixed a few javadoc issues
• JavaLangClash - Package was used twice in Solr code - renamed to SolrPackage and PackagePayload. Also renamed PackageLoader to SolrPackageLoader. Renamed Error class in SolrCmdDistributor to SolrError to avoid clashing with Java.
• JdkObsolete - There are some suppressions for this for LinkedList usages. Most went to ArrayList or ArrayDeque
• ProtectedMembersInFinalClass - fixed the visibility of these methods
• UnsynchronizedOverridesSynchronized found one issue in StandardDirectoryFactory where removeDirectory missed synchronized

[epugh]

I've looked at about half the changes... It all seems good... I'm going to try and run the tests before I head to bed on this branch...

[epugh]

Your most recent fix JUST fixed the test error I was getting...

./gradlew :solr:core:test --tests "org.apache.solr.handler.component.DistributedFacetPivotLargeTest" passes now for me!

[epugh]

THis is failing...

./gradlew :solr:core:test --tests "org.apache.solr.core.TestCoreContainer"

[risdenk]

./gradlew :solr:core:test --tests "org.apache.solr.core.TestCoreContainer"

@epugh I'm not seeing that test fail. I ran it ~10 times and no failures.

[dsmiley]

Thanks for this. It'd help reviewing if you did the work in separate commits. That way I could look at the commits and maybe review some commits of interest to me. For example inlining deprecated methods on SolrException could have been its own commit. This in particular feels different in nature from the other changes that it should be in some other PR, actually.

[risdenk]

It'd help reviewing if you did the work in separate commits. That way I could look at the commits and maybe review some commits of interest to me. For example inlining deprecated methods on SolrException could have been its own commit. This in particular feels different in nature from the other changes that it should be in some other PR, actually.

yea will admit got a bit overzealous fixing things in some of these files. I'll see if I can clean it up.

[risdenk]

I'm going to come back to this after https://github.com/apache/solr/pull/953 is merged. I'm expecting some conflicts based on changes I shouldn't have made in this PR that are made in https://github.com/apache/solr/pull/953

[risdenk]

yea I figured it would conflict after https://github.com/apache/solr/pull/953. gives me a chance to separate out the commits as @dsmiley rightly pointed out.

[risdenk]

@dsmiley / @epugh I force pushed this branch to reapply these changes on top of https://github.com/apache/solr/pull/953 and break up each commit into one error-prone rule. Hopefully that makes it easier to review.

[risdenk]

I'm not 100% sure I got all the changes from my previous branch - I'm running all the tests now and will potentially force push again to fix any minor things I missed.

[risdenk]

solr git:(SOLR-16427-part-2) ./gradlew check -Pvalidation.errorprone=true
...
BUILD SUCCESSFUL in 21m 31s
586 actionable tasks: 215 executed, 371 up-to-date

[risdenk]

I didn't make any changes to the underlying code - just updated all the commit messages. I don't plan on squashing these so its easier to revert any individual commits if necessary.

[dsmiley]

+1 thanks Kevin

[risdenk]

part 3 continues here: https://github.com/apache/solr/pull/1116