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 2022/12/07 14:42:44 UTC
[GitHub] [solr] heythm opened a new pull request, #1221: SOLR-16577: Always log core load issues
heythm opened a new pull request, #1221:
URL: https://github.com/apache/solr/pull/1221
https://issues.apache.org/jira/browse/SOLR-XXXXX
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Please provide a short description of the changes you're making with this pull request.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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-unsubscribe@solr.apache.org
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
Re: [PR] SOLR-16577: Always log core load issues [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #1221:
URL: https://github.com/apache/solr/pull/1221
--
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-unsubscribe@solr.apache.org
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] risdenk commented on pull request #1221: SOLR-16577: Always log core load issues
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1221:
URL: https://github.com/apache/solr/pull/1221#issuecomment-1412555567
@heythm hope all is well - curious of the comments will be addressed? I think this is a beneficial change
--
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-unsubscribe@solr.apache.org
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 diff in pull request #1221: SOLR-16577: Always log core load issues
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1221:
URL: https://github.com/apache/solr/pull/1221#discussion_r1043577392
##########
solr/CHANGES.txt:
##########
@@ -160,6 +160,8 @@ Other Changes
SOLR-16575: splitshard should honour createNodeSet (noble)
+* SOLR-16577: Always log core load issues (Haythem Khiri)
Review Comment:
```suggestion
* SOLR-16577: Encore core load failures are always logged. (Haythem Khiri, David Smiley)
```
##########
solr/CHANGES.txt:
##########
@@ -160,6 +160,8 @@ Other Changes
SOLR-16575: splitshard should honour createNodeSet (noble)
+* SOLR-16577: Always log core load issues (Haythem Khiri)
Review Comment:
I'd also put this under Improvements.
--
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-unsubscribe@solr.apache.org
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 #1221: SOLR-16577: Always log core load issues
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1221:
URL: https://github.com/apache/solr/pull/1221#issuecomment-1360080469
We should also probably set & clear the MDC so that if we log an issue, we have more context (e.g. *which* core).
--
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-unsubscribe@solr.apache.org
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 #1221: SOLR-16577: Always log core load issues
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1221:
URL: https://github.com/apache/solr/pull/1221#issuecomment-1343014214
FYI everyone, I already code reviewed this.
BTW a nice improvement here is that we remove needless complexity. These Futures were pointless. Perhaps there was once a point to them but I see no point now.
--
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-unsubscribe@solr.apache.org
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
Re: [PR] SOLR-16577: Always log core load issues [solr]
Posted by "heythm (via GitHub)" <gi...@apache.org>.
heythm commented on code in PR #1221:
URL: https://github.com/apache/solr/pull/1221#discussion_r1417144757
##########
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:
yes, hence no need to throw an exception at a lower level; in task body
--
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-unsubscribe@solr.apache.org
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] risdenk commented on a diff in pull request #1221: SOLR-16577: Always log core load issues
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
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 #1221: SOLR-16577: Always log core load issues
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1221:
URL: https://github.com/apache/solr/pull/1221#issuecomment-1376427109
While working on another issue & troubleshooting, I would have benefited from this improvement. Some core initialization errors were getting swallowed.
--
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-unsubscribe@solr.apache.org
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
Re: [PR] SOLR-16577: Always log core load issues [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1221:
URL: https://github.com/apache/solr/pull/1221#issuecomment-1847186357
@risdenk I think your concerns have been addressed?
--
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-unsubscribe@solr.apache.org
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
Re: [PR] SOLR-16577: Always log core load issues [solr]
Posted by "heythm (via GitHub)" <gi...@apache.org>.
heythm commented on code in PR #1221:
URL: https://github.com/apache/solr/pull/1221#discussion_r1417168401
##########
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:
`coreContainerWorkExecutor` is for async core loadings
##########
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:
`coreContainerWorkExecutor` is for async cores loadings
--
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-unsubscribe@solr.apache.org
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
Re: [PR] SOLR-16577: Always log core load issues [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1221:
URL: https://github.com/apache/solr/pull/1221#discussion_r1416431432
##########
solr/CHANGES.txt:
##########
@@ -160,7 +160,7 @@ Other Changes
SOLR-16575: splitshard should honour createNodeSet (noble)
-* SOLR-16577: Always log core load issues (Haythem Khiri)
+* SOLR-16577: Encore core load failures are always logged. (Haythem Khiri, David Smiley)
Review Comment:
```suggestion
* SOLR-16577: Ensure core load failures are always logged. (Haythem Khiri, David Smiley)
```
--
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-unsubscribe@solr.apache.org
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
Re: [PR] SOLR-16577: Always log core load issues [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1221:
URL: https://github.com/apache/solr/pull/1221#issuecomment-1841829670
@heythm , for a very old PR, first get it up to date with main (merge main into this) *then* update CHANGES.txt. Doing CHANGES.txt too early is no fun merging. Also remember there is a bunch of feedback here that is still unanswered.
--
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-unsubscribe@solr.apache.org
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
Re: [PR] SOLR-16577: Always log core load issues [solr]
Posted by "heythm (via GitHub)" <gi...@apache.org>.
heythm commented on PR #1221:
URL: https://github.com/apache/solr/pull/1221#issuecomment-1843222328
Sorry for the force-push, I did it to amend a commit message. It wasn't a good idea.
@dsmiley knows about 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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
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