risdenk commented on code in PR #1221:
URL: https://github.com/apache/solr/pull/1221#discussion_r1043921139
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -973,29 +969,31 @@ public void load() {
solrCores.markCoreAsLoading(cd);
}
if (cd.isLoadOnStartup()) {
- futures.add(
- coreLoadExecutor.submit(
- () -> {
- SolrCore core;
- try {
- if (zkSys.getZkController() != null) {
-
zkSys.getZkController().throwErrorIfReplicaReplaced(cd);
- }
- solrCores.waitAddPendingCoreOps(cd.getName());
- core = createFromDescriptor(cd, false, false);
- } finally {
- solrCores.removeFromPendingOps(cd.getName());
- if (asyncSolrCoreLoad) {
- solrCores.markCoreAsNotLoading(cd);
- }
- }
- try {
- zkSys.registerInZk(core, true, false);
- } catch (RuntimeException e) {
- SolrException.log(log, "Error registering SolrCore", e);
- }
- return core;
- }));
+ coreLoadExecutor.submit(
+ () -> {
+ SolrCore core;
+ try {
+ if (zkSys.getZkController() != null) {
+ zkSys.getZkController().throwErrorIfReplicaReplaced(cd);
+ }
+ solrCores.waitAddPendingCoreOps(cd.getName());
+ core = createFromDescriptor(cd, false, false);
+ } catch (Exception e) {
+ log.error("SolrCore failed to load on startup", e);
+ return null;
+ } finally {
+ solrCores.removeFromPendingOps(cd.getName());
+ if (asyncSolrCoreLoad) {
+ solrCores.markCoreAsNotLoading(cd);
+ }
+ }
+ try {
+ zkSys.registerInZk(core, true, false);
+ } catch (RuntimeException e) {
+ log.error("Error registering SolrCore", e);
+ }
+ return core;
Review Comment:
Do we need to return anything? Since this isn't stored in a future?
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -973,29 +969,31 @@ public void load() {
solrCores.markCoreAsLoading(cd);
}
if (cd.isLoadOnStartup()) {
- futures.add(
- coreLoadExecutor.submit(
- () -> {
- SolrCore core;
- try {
- if (zkSys.getZkController() != null) {
-
zkSys.getZkController().throwErrorIfReplicaReplaced(cd);
- }
- solrCores.waitAddPendingCoreOps(cd.getName());
- core = createFromDescriptor(cd, false, false);
- } finally {
- solrCores.removeFromPendingOps(cd.getName());
- if (asyncSolrCoreLoad) {
- solrCores.markCoreAsNotLoading(cd);
- }
- }
- try {
- zkSys.registerInZk(core, true, false);
- } catch (RuntimeException e) {
- SolrException.log(log, "Error registering SolrCore", e);
- }
- return core;
- }));
+ coreLoadExecutor.submit(
+ () -> {
+ SolrCore core;
+ try {
+ if (zkSys.getZkController() != null) {
+ zkSys.getZkController().throwErrorIfReplicaReplaced(cd);
+ }
+ solrCores.waitAddPendingCoreOps(cd.getName());
+ core = createFromDescriptor(cd, false, false);
+ } catch (Exception e) {
+ log.error("SolrCore failed to load on startup", e);
+ return null;
Review Comment:
What does returning null do here?
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -1004,24 +1002,9 @@ public void load() {
backgroundCloser.start();
} finally {
- if (asyncSolrCoreLoad && futures != null) {
-
+ if (asyncSolrCoreLoad) {
coreContainerWorkExecutor.submit(
- () -> {
- try {
- for (Future<SolrCore> future : futures) {
- try {
- future.get();
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- } catch (ExecutionException e) {
- log.error("Error waiting for SolrCore to be loaded on
startup", e);
Review Comment:
At least the old code was just logging on failure. I guess this was ok to
not throw anything?
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -973,29 +969,31 @@ public void load() {
solrCores.markCoreAsLoading(cd);
}
if (cd.isLoadOnStartup()) {
- futures.add(
- coreLoadExecutor.submit(
- () -> {
- SolrCore core;
- try {
- if (zkSys.getZkController() != null) {
-
zkSys.getZkController().throwErrorIfReplicaReplaced(cd);
- }
- solrCores.waitAddPendingCoreOps(cd.getName());
- core = createFromDescriptor(cd, false, false);
- } finally {
- solrCores.removeFromPendingOps(cd.getName());
- if (asyncSolrCoreLoad) {
- solrCores.markCoreAsNotLoading(cd);
- }
- }
- try {
- zkSys.registerInZk(core, true, false);
- } catch (RuntimeException e) {
- SolrException.log(log, "Error registering SolrCore", e);
- }
- return core;
- }));
+ coreLoadExecutor.submit(
Review Comment:
Do we only need to do this in an executor if `asyncSolrCoreLoad`? Or if this
is async should it use `coreContainerWorkExecutor`?
I'm a little surprised by looking at this diff there are two executors -
`coreLoadExecutor` and `coreContainerWorkExecutor`?
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -973,29 +969,31 @@ public void load() {
solrCores.markCoreAsLoading(cd);
}
if (cd.isLoadOnStartup()) {
- futures.add(
- coreLoadExecutor.submit(
- () -> {
- SolrCore core;
- try {
- if (zkSys.getZkController() != null) {
-
zkSys.getZkController().throwErrorIfReplicaReplaced(cd);
- }
- solrCores.waitAddPendingCoreOps(cd.getName());
- core = createFromDescriptor(cd, false, false);
- } finally {
- solrCores.removeFromPendingOps(cd.getName());
- if (asyncSolrCoreLoad) {
- solrCores.markCoreAsNotLoading(cd);
- }
- }
- try {
- zkSys.registerInZk(core, true, false);
- } catch (RuntimeException e) {
- SolrException.log(log, "Error registering SolrCore", e);
- }
- return core;
- }));
+ coreLoadExecutor.submit(
+ () -> {
+ SolrCore core;
+ try {
+ if (zkSys.getZkController() != null) {
+ zkSys.getZkController().throwErrorIfReplicaReplaced(cd);
+ }
+ solrCores.waitAddPendingCoreOps(cd.getName());
+ core = createFromDescriptor(cd, false, false);
+ } catch (Exception e) {
+ log.error("SolrCore failed to load on startup", e);
+ return null;
+ } finally {
+ solrCores.removeFromPendingOps(cd.getName());
+ if (asyncSolrCoreLoad) {
+ solrCores.markCoreAsNotLoading(cd);
+ }
+ }
+ try {
+ zkSys.registerInZk(core, true, false);
+ } catch (RuntimeException e) {
+ log.error("Error registering SolrCore", e);
Review Comment:
Shouldn't this throw or at least do something besides just log?
--
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]
