keith-turner commented on code in PR #4125:
URL: https://github.com/apache/accumulo/pull/4125#discussion_r1441080539
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -166,8 +168,17 @@ private static Lock getLock(ServerContext context,
AbstractId<?> id, long tid,
var fLockPath =
FateLock.path(context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS +
"/" + id.canonical());
FateLock qlock = new FateLock(context.getZooReaderWriter(), fLockPath);
- Lock lock = DistributedReadWriteLock.recoverLock(qlock, lockData);
- if (lock == null) {
+ DistributedLock lock = DistributedReadWriteLock.recoverLock(qlock,
lockData);
+ if (lock != null) {
+
+ // Validate the recovered lock type
+ boolean isWriteLock = lock.getType() == LockType.WRITE;
+ if (writeLock != isWriteLock) {
+ throw new IllegalStateException("Unexpected lock type " +
lock.getType()
+ + " recovered for transaction " + tid + " on object " + id + ".
Expected "
Review Comment:
Since we don't have a fate id type, its good to use the following method to
format fate ids. This makes it possible to easily find all messages related to
the same fate operation. Otherwise some could be in decimal and others in hex.
May need to import FateTxId, so this change may not compile.
```suggestion
+ " recovered for transaction " + FateTxId.formatTid(tid) + " on
object " + id + ". Expected "
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -166,8 +168,17 @@ private static Lock getLock(ServerContext context,
AbstractId<?> id, long tid,
var fLockPath =
FateLock.path(context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS +
"/" + id.canonical());
FateLock qlock = new FateLock(context.getZooReaderWriter(), fLockPath);
- Lock lock = DistributedReadWriteLock.recoverLock(qlock, lockData);
- if (lock == null) {
+ DistributedLock lock = DistributedReadWriteLock.recoverLock(qlock,
lockData);
+ if (lock != null) {
+
+ // Validate the recovered lock type
+ boolean isWriteLock = lock.getType() == LockType.WRITE;
Review Comment:
This change does not need to be made in this PR. Looking at this code,
wondering if it would clean things up to pass LockType into this method instead
of `boolean writeLock`. It would make the comparison in this if stmt nicer,
but not sure if its an overall win. If its seems useful, could be a follow on
PR.
--
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]
