dsmiley commented on code in PR #5161:
URL: https://github.com/apache/solr/pull/5161#discussion_r4355866431
##########
solr/core/src/java/org/apache/solr/cloud/ZkDistributedLockFactory.java:
##########
@@ -37,16 +39,15 @@ abstract class ZkDistributedLockFactory {
protected DistributedLock doCreateLock(
boolean isWriteLock, String lockPath, String lockIdToMirror) {
try {
- if (StrUtils.isBlank(lockIdToMirror)) {
- // TODO optimize by first attempting to create the ZkDistributedLock
without calling
- // makeLockPath() and only call it if the lock creation fails. This
will be less costly on
- // high contention (and slightly more on low contention)
- makeLockPath(lockPath);
+ for (int attempt = 1; attempt < MAX_CREATE_ATTEMPTS; attempt++) {
+ try {
+ return newLock(isWriteLock, lockPath, lockIdToMirror);
+ } catch (KeeperException.NoNodeException e) {
+ // Lock dirs are containers, so ZK drops them when empty. Recreate
and try again.
+ makeLockPath(lockPath);
+ }
}
-
- return isWriteLock
- ? new ZkDistributedLock.Write(zkClient, lockPath, lockIdToMirror)
- : new ZkDistributedLock.Read(zkClient, lockPath, lockIdToMirror);
+ return newLock(isWriteLock, lockPath, lockIdToMirror);
Review Comment:
I think this would look clearer as a loop over MAX_CREATE_ATTEMPTS (not one
less than that). The only difference is that the catch needs to see if we're
at the last attempt to then throw instead of makeLockPath.
--
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]
