SOLR-18011: Allow locked Admin APIs to call other locked Admin APIs #3916
Conversation
github-actions
bot
added
test-framework
client:solrj
module:s3-repository
tests
cat:cloud
cat:api
labels
gerlowskija
left a comment
gerlowskija
left a comment
There was a problem hiding this comment.
No big changes jump out to me here, just a few questions and comments. Would be happy to approve as soon as you're able to clarify those and I can understand this just a bit better.
| @@ -0,0 +1,9 @@ | |||
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | |||
| title: Allow locked Admin APIs to call other locked AdminAPIs without deadlocking | |||
There was a problem hiding this comment.
[0] If these deadlocks are a real thing that a user might've seen, then I think we should reword the changelog to highlight the benefit they get out of this change. Maybe something like:
Resolve locking-related deadlock in INSTALLSHARD and similar Admin API calls
There was a problem hiding this comment.
So currently no Admin APIs call other Admin APIs, they will run the Cmd themselves perhaps, but they won't submit it to the overseer or anything. Or they will call the coreAPIs themselves that the other collection API would do. But we currently get around this deadlock by just not doing it. I think we should be able to re-use code a bit better and have 1 entry point for a lot of these APIs, so this is a necessary part of that.
| } | ||
| } | ||
|
|
||
| public String getCallingLockIds() { |
There was a problem hiding this comment.
[0] I understand that a comma-delimited-string is our serialization format for when this is header, but it'd probably be nicer to have this method and others expose a List<String> or something similar. Saves callers from needing to understand the serialization format and parse it out themselves...
There was a problem hiding this comment.
Fair enough, I went back and forth on this. I can easily change it back though.
There was a problem hiding this comment.
Maybe I'll just add a method to do the deserialization here, it's nice to keep it as a string throughout the process if we can.
There was a problem hiding this comment.
Yeah, I've added this and it works quite well.
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionApiLockFactory.java
Outdated
Show resolved
Hide resolved
| params.remove(CommonParams.WT); // use default (currently javabin) | ||
| QueryRequest req = createQueryRequest(sreq, params, shard); | ||
| req.setMethod(SolrRequest.METHOD.POST); | ||
| if (sreq.headers != null) { |
There was a problem hiding this comment.
[Q] More thinking aloud than raising an objection. In general I've seen bugs before where headers are copied over blindly because certain things like auth headers, content-length, etc. may not be relevant for the sub-request.
But, in this case the ShardRequest is something that we've built by hand and that doesn't have all the headers from the original user request. So it should be safe to do a blind-copy here.
Do I have that right?
There was a problem hiding this comment.
Yes, headers are added on an individual basis, and will probably only have this header for a while.
solr/solrj/src/java/org/apache/solr/common/params/CollectionParams.java
Outdated
Show resolved
Hide resolved
|
Ultimately I think there is 2 things left to do here:
|
2 participants
https://issues.apache.org/jira/browse/SOLR-18011
The Admin APIs will need to take in information in the request about the lock acquired for the parent Admin API call, so that it doesn't try to acquire that lock as well.