sarkaramrit1993 commented on code in PR #5161:
URL: https://github.com/apache/solr/pull/5161#discussion_r4374562743
##########
solr/core/src/java/org/apache/solr/cloud/ZkDistributedLockFactory.java:
##########
@@ -55,19 +58,57 @@ protected DistributedLock doCreateLock(
}
}
+ private DistributedLock newLock(boolean isWriteLock, String lockPath, String
lockIdToMirror)
+ throws KeeperException, InterruptedException {
+ return isWriteLock
+ ? new ZkDistributedLock.Write(zkClient, lockPath, lockIdToMirror)
+ : new ZkDistributedLock.Read(zkClient, lockPath, lockIdToMirror);
+ }
+
protected StringBuilder getPathPrefix() {
StringBuilder path = new StringBuilder(100);
path.append(rootPath).append(DistributedCollectionConfigSetCommandRunner.ZK_PATH_SEPARATOR);
return path;
}
- private void makeLockPath(String lockNodePath) throws KeeperException,
InterruptedException {
+ private void makeLockPath(String lockPath) throws KeeperException,
InterruptedException {
+ if (!zkClient.exists(rootPath)) {
+ zkClient.makePath(rootPath, false);
+ }
+ StringBuilder path = new StringBuilder(rootPath);
+ for (String node :
+ lockPath
+ .substring(rootPath.length() + 1)
+
.split(DistributedCollectionConfigSetCommandRunner.ZK_PATH_SEPARATOR)) {
+
path.append(DistributedCollectionConfigSetCommandRunner.ZK_PATH_SEPARATOR).append(node);
+ try {
+ zkClient.create(path.toString(), null, CreateMode.CONTAINER);
+ } catch (KeeperException.NodeExistsException nee) {
+ // Some other thread (on this or another JVM) beat us to create the
node, that's ok.
+ } catch (KeeperException.NoNodeException nne) {
+ // A parent got removed while we were here. The caller retries.
+ return;
+ }
+ }
+ }
+
+ /**
+ * Deletes the empty lock dirs under {@code path}, deepest first. Lock nodes
and dirs still
+ * holding them are left alone.
+ */
+ protected void deleteEmptyDirs(String path) throws KeeperException,
InterruptedException {
+ Stat stat = zkClient.exists(path, null);
+ if (stat == null || stat.getEphemeralOwner() != 0) {
Review Comment:
By the time this runs, the DELETE has released its own lock, so any
ephemeral left under the dir is the R_/W_ node of an op that's still in flight.
Deleting it breaks that op: its owner fails with ["Missing lock
node"](https://github.com/apache/solr/blob/da72ab13dd933b2cbd06969d96d299d0e354f36a/solr/core/src/java/org/apache/solr/cloud/ZkDistributedLock.java#L262)
on the next check, and anything waiting behind it [stops
waiting](https://github.com/apache/solr/blob/da72ab13dd933b2cbd06969d96d299d0e354f36a/solr/core/src/java/org/apache/solr/cloud/ZkDistributedLock.java#L184-L190)
and gets the lock while the first op is still running. So we stop at live
locks and remove everything else. Nodes from dead sessions are already gone by
then. Curator does the same: `LockInternals.clean()` only deletes the lock dir
once it's empty.
##########
solr/core/src/java/org/apache/solr/cloud/ZkDistributedLockFactory.java:
##########
@@ -55,19 +58,57 @@ protected DistributedLock doCreateLock(
}
}
+ private DistributedLock newLock(boolean isWriteLock, String lockPath, String
lockIdToMirror)
+ throws KeeperException, InterruptedException {
+ return isWriteLock
Review Comment:
Agreed, inlined it in da72ab13dd9. It only had the one caller.
--
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]
