gerlowskija commented on code in PR #3916:
URL: https://github.com/apache/solr/pull/3916#discussion_r2742123458
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/CollectionApiLockFactory.java:
##########
@@ -118,10 +126,18 @@ DistributedMultiLock createCollectionApiLock(
};
List<DistributedLock> locks = new ArrayList<>(iterationOrder.length);
for (CollectionParams.LockLevel level : iterationOrder) {
+ // We do not want to re-lock from the parent lock level
+ // TODO: Fix
Review Comment:
[Q] Is this a TODO you're aiming to fix prior to merging, or a longer-term
improvement idea?
##########
changelog/unreleased/solr-18011-locking-update.yml:
##########
@@ -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
Review 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
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/AdminCmdContext.java:
##########
@@ -42,6 +56,36 @@ public String getAsyncId() {
return asyncId;
}
+ public void setLockId(String lockId) {
+ this.lockId = lockId;
+ regenerateSubRequestCallingLockIds();
+ }
+
+ public String getLockId() {
+ return lockId;
+ }
+
+ public void setCallingLockIds(String callingLockIds) {
+ this.callingLockIds = callingLockIds;
+ regenerateSubRequestCallingLockIds();
+ }
+
+ private void regenerateSubRequestCallingLockIds() {
+ if (StrUtils.isNotBlank(callingLockIds) && StrUtils.isNotBlank(lockId)) {
+ subRequestCallingLockIds += "," + lockId;
+ } else if (StrUtils.isNotBlank(callingLockIds)) {
+ subRequestCallingLockIds = callingLockIds;
+ } else if (StrUtils.isNotBlank(lockId)) {
+ subRequestCallingLockIds = lockId;
+ } else {
+ subRequestCallingLockIds = null;
+ }
+ }
+
+ public String getCallingLockIds() {
Review 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...
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java:
##########
@@ -188,6 +188,9 @@ private LBSolrClient.Req prepareLBRequest(
params.remove(CommonParams.WT); // use default (currently javabin)
QueryRequest req = createQueryRequest(sreq, params, shard);
req.setMethod(SolrRequest.METHOD.POST);
+ if (sreq.headers != null) {
Review 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?
##########
solr/solrj/src/java/org/apache/solr/common/params/CollectionParams.java:
##########
@@ -48,7 +48,8 @@ enum LockLevel {
REPLICA(3, null),
SHARD(2, REPLICA),
COLLECTION(1, SHARD),
- CLUSTER(0, COLLECTION);
+ CLUSTER(0, COLLECTION),
+ BASE(-1, CLUSTER);
Review Comment:
[Q] I can't find this "BASE" enum constant used anywhere else in this PR -
what am I missing?
--
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]
