You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "bruno-roustant (via GitHub)" <gi...@apache.org> on 2025/03/24 08:27:48 UTC
[PR] SOLR-17716: Handle interrupted exception in SolrCores.waitAddPendingCoreOps. [solr]
bruno-roustant opened a new pull request, #3283:
URL: https://github.com/apache/solr/pull/3283
https://issues.apache.org/jira/browse/SOLR-17716
SolrCores.waitAddPendingCoreOps is the main locking mechanism for operations on cores like creation/loading/removal.
In the current code, if an InterruptedException is caught while waiting for the lock, the method returns null. However returning null means basically that the waiting is complete and the caller got the lock. So the caller code proceeds normally with its critical operation on the core.
We should handle differently an InterruptedException to prevent the caller from taking the lock while another pending op is on the core. The proposal is to throw a SolrException.
--
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-17716: Handle interrupted exception in SolrCores.waitAddPendingCoreOps. [solr]
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #3283:
URL: https://github.com/apache/solr/pull/3283#discussion_r2010360147
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2323,16 +2323,14 @@ public SolrCore getCore(String name, UUID id) {
// TestConfigSetsAPI and TestLazyCores
if (desc == null || zkSys.getZkController() != null) return null;
- // This will put an entry in pending core ops if the core isn't loaded. Here's where moving the
- // waitAddPendingCoreOps to createFromDescriptor would introduce a race condition.
- core = solrCores.waitAddPendingCoreOps(name);
-
- if (isShutDown) {
- // We're quitting, so stop. This needs to be after the wait above since we may come off the
- // wait as a consequence of shutting down.
- return null;
- }
try {
+ // This will put an entry in pending core ops if the core isn't loaded. Here's where moving
+ // the waitAddPendingCoreOps to createFromDescriptor would introduce a race condition.
+ core = solrCores.waitAddPendingCoreOps(name);
Review Comment:
This is not safe to move this call inside the `try` block.
If an exception is raised for whatever reason, it means we did not have the lock. In this case, we should **not** execute the finally block that releases the lock.
--
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-17716: Handle interrupted exception in SolrCores.waitAddPendingCoreOps. [solr]
Posted by "psalagnac (via GitHub)" <gi...@apache.org>.
psalagnac commented on code in PR #3283:
URL: https://github.com/apache/solr/pull/3283#discussion_r2010386539
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2323,16 +2323,14 @@ public SolrCore getCore(String name, UUID id) {
// TestConfigSetsAPI and TestLazyCores
if (desc == null || zkSys.getZkController() != null) return null;
- // This will put an entry in pending core ops if the core isn't loaded. Here's where moving the
- // waitAddPendingCoreOps to createFromDescriptor would introduce a race condition.
- core = solrCores.waitAddPendingCoreOps(name);
-
- if (isShutDown) {
- // We're quitting, so stop. This needs to be after the wait above since we may come off the
- // wait as a consequence of shutting down.
- return null;
- }
try {
+ // This will put an entry in pending core ops if the core isn't loaded. Here's where moving
+ // the waitAddPendingCoreOps to createFromDescriptor would introduce a race condition.
+ core = solrCores.waitAddPendingCoreOps(name);
Review Comment:
I think it is fine to throw an exception as long as we don't remove the lock in case of exception. I'm good with your change in `SolrCores` (the other file).
--
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-17716: Handle interrupted exception in SolrCores.waitAddPendingCoreOps. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant merged PR #3283:
URL: https://github.com/apache/solr/pull/3283
--
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-17716: Handle interrupted exception in SolrCores.waitAddPendingCoreOps. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on PR #3283:
URL: https://github.com/apache/solr/pull/3283#issuecomment-2750377083
Thanks Pierre for your review.
--
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-17716: Handle interrupted exception in SolrCores.waitAddPendingCoreOps. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #3283:
URL: https://github.com/apache/solr/pull/3283#discussion_r2010426760
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2323,16 +2323,14 @@ public SolrCore getCore(String name, UUID id) {
// TestConfigSetsAPI and TestLazyCores
if (desc == null || zkSys.getZkController() != null) return null;
- // This will put an entry in pending core ops if the core isn't loaded. Here's where moving the
- // waitAddPendingCoreOps to createFromDescriptor would introduce a race condition.
- core = solrCores.waitAddPendingCoreOps(name);
-
- if (isShutDown) {
- // We're quitting, so stop. This needs to be after the wait above since we may come off the
- // wait as a consequence of shutting down.
- return null;
- }
try {
+ // This will put an entry in pending core ops if the core isn't loaded. Here's where moving
+ // the waitAddPendingCoreOps to createFromDescriptor would introduce a race condition.
+ core = solrCores.waitAddPendingCoreOps(name);
Review Comment:
Fixing the 5 calls is straightforward, so let's keep the exception.
--
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-17716: Handle interrupted exception in SolrCores.waitAddPendingCoreOps. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #3283:
URL: https://github.com/apache/solr/pull/3283#discussion_r2010382686
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2323,16 +2323,14 @@ public SolrCore getCore(String name, UUID id) {
// TestConfigSetsAPI and TestLazyCores
if (desc == null || zkSys.getZkController() != null) return null;
- // This will put an entry in pending core ops if the core isn't loaded. Here's where moving the
- // waitAddPendingCoreOps to createFromDescriptor would introduce a race condition.
- core = solrCores.waitAddPendingCoreOps(name);
-
- if (isShutDown) {
- // We're quitting, so stop. This needs to be after the wait above since we may come off the
- // wait as a consequence of shutting down.
- return null;
- }
try {
+ // This will put an entry in pending core ops if the core isn't loaded. Here's where moving
+ // the waitAddPendingCoreOps to createFromDescriptor would introduce a race condition.
+ core = solrCores.waitAddPendingCoreOps(name);
Review Comment:
You're right. I wanted to have a consistent way to try-finally between all the calls to waitAddPendingCoreOps.
That means that changing waitAddPendingCoreOps to throw an exception is either not appropriate, or needs more care with the other callers. And btw, the other callers may have currently an issue if an exception is thrown inside waitAddPendingCoreOps.
--
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