dsmiley commented on code in PR #2707:
URL: https://github.com/apache/solr/pull/2707#discussion_r1774359207
##########
solr/CHANGES.txt:
##########
@@ -163,6 +163,8 @@ Other Changes
* SOLR-17142: Fix Gradle build sometimes gives spurious "unreferenced license
file" warnings. (Uwe Schindler)
+* SOLR-17448: Audit usage of ExecutorService#submit in Solr codebase (Andrey
Bozhko)
Review Comment:
Could you reword this into its impact instead of its technical detail? Like
maybe "fixed inadvertent suppression of exceptions across the codebase due to
...""
##########
solr/core/src/java/org/apache/solr/schema/ZkIndexSchemaReader.java:
##########
@@ -154,6 +151,26 @@ public void process(WatchedEvent event) {
}
}
+ @Override
+ public void process(WatchedEvent event) {
Review Comment:
minor: please move this method to immediately before doProcess since it
calls it.
##########
solr/core/src/test/org/apache/solr/pkg/TestPackages.java:
##########
@@ -810,10 +813,18 @@ public void testSchemaPlugins() throws Exception {
":fieldType:_packageinfo_:version",
"1.0"));
+ JettySolrRunner jetty =
+ cluster.getJettySolrRunners().stream()
+ .dropWhile(j -> j.getCoreContainer().getAllCoreNames().isEmpty())
+ .findFirst()
+ .orElseThrow();
+
+ IndexSchema schemaBeforePackageUpdate = withOnlyCoreInJetty(jetty,
SolrCore::getLatestSchema);
+
add = new PackagePayload.AddVersion();
add.version = "2.0";
add.pkg = "schemapkg";
- add.files = Arrays.asList(FILE1);
+ add.files = Arrays.asList(FILE1, FILE2);
Review Comment:
Great investigation! It's a nice little story how submit() was hiding
errors!
##########
solr/core/src/java/org/apache/solr/handler/admin/PrepRecoveryOp.java:
##########
@@ -191,11 +195,6 @@ public void execute(CallInfo it) throws Exception {
}
}
- if (coreContainer.isShutDown()) {
Review Comment:
why remove this?
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -1981,29 +1981,18 @@ protected void destroyServers() throws Exception {
ExecutorService customThreadPool =
ExecutorUtil.newMDCAwareCachedThreadPool(new
SolrNamedThreadFactory("closeThreadPool"));
- customThreadPool.submit(() -> IOUtils.closeQuietly(commonCloudSolrClient));
+ customThreadPool.execute(() ->
IOUtils.closeQuietly(commonCloudSolrClient));
- customThreadPool.submit(() -> IOUtils.closeQuietly(controlClient));
+ customThreadPool.execute(() -> IOUtils.closeQuietly(controlClient));
- customThreadPool.submit(
- () ->
- coreClients.parallelStream()
- .forEach(
- c -> {
- IOUtils.closeQuietly(c);
- }));
+ customThreadPool.execute(() ->
coreClients.parallelStream().forEach(IOUtils::closeQuietly));
Review Comment:
shouldn't we avoid parallelStream? Could loop these to submit them to
customThreadPool. Same for below.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
