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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org