Skip to content

Commit 3b1566f

Browse files
cshannoncshannon
authored
Replace boolean with enum for table locks in 3.1 (apache#4297)
Instead of using a boolean value for obtaining table locks, use an enum which makes the code more readable and makes it easier to search and catch errors with the lock type. This closes apache#4276
1 parent c853f64 commit 3b1566f

28 files changed

+146
-112
lines changed

‎core/src/main/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLock.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@
4040
*/
4141
public class DistributedReadWriteLock implements java.util.concurrent.locks.ReadWriteLock {
4242

43-
public static enum LockType {
43+
public enum LockType {
4444
READ, WRITE,
4545
}
4646

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/ChangeTableState.java

+7-6
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.apache.accumulo.core.data.NamespaceId;
2525
import org.apache.accumulo.core.data.TableId;
2626
import org.apache.accumulo.core.fate.Repo;
27+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
2728
import org.apache.accumulo.core.manager.state.tables.TableState;
2829
import org.apache.accumulo.manager.Manager;
2930
import org.slf4j.LoggerFactory;
@@ -52,8 +53,8 @@ public ChangeTableState(NamespaceId namespaceId, TableId tableId, TableOperation
5253
public long isReady(long tid, Manager env) throws Exception {
5354
// reserve the table so that this op does not run concurrently with create, clone, or delete
5455
// table
55-
return Utils.reserveNamespace(env, namespaceId, tid, false, true, top)
56-
+ Utils.reserveTable(env, tableId, tid, true, true, top);
56+
return Utils.reserveNamespace(env, namespaceId, tid, LockType.READ, true, top)
57+
+ Utils.reserveTable(env, tableId, tid, LockType.WRITE, true, top);
5758
}
5859

5960
@Override
@@ -64,16 +65,16 @@ public Repo<Manager> call(long tid, Manager env) {
6465
}
6566

6667
env.getTableManager().transitionTableState(tableId, ts, expectedCurrStates);
67-
Utils.unreserveNamespace(env, namespaceId, tid, false);
68-
Utils.unreserveTable(env, tableId, tid, true);
68+
Utils.unreserveNamespace(env, namespaceId, tid, LockType.READ);
69+
Utils.unreserveTable(env, tableId, tid, LockType.WRITE);
6970
LoggerFactory.getLogger(ChangeTableState.class).debug("Changed table state {} {}", tableId, ts);
7071
env.getEventCoordinator().event("Set table state of %s to %s", tableId, ts);
7172
return null;
7273
}
7374

7475
@Override
7576
public void undo(long tid, Manager env) {
76-
Utils.unreserveNamespace(env, namespaceId, tid, false);
77-
Utils.unreserveTable(env, tableId, tid, true);
77+
Utils.unreserveNamespace(env, namespaceId, tid, LockType.READ);
78+
Utils.unreserveTable(env, tableId, tid, LockType.WRITE);
7879
}
7980
}

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java

+25-24
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@ public static <T extends AbstractId<T>> T getNextId(String name, ServerContext c
9090
static final Lock tableNameLock = new ReentrantLock();
9191
static final Lock idLock = new ReentrantLock();
9292

93-
public static long reserveTable(Manager env, TableId tableId, long tid, boolean writeLock,
93+
public static long reserveTable(Manager env, TableId tableId, long tid, LockType lockType,
9494
boolean tableMustExist, TableOperation op) throws Exception {
95-
if (getLock(env.getContext(), tableId, tid, writeLock).tryLock()) {
95+
if (getLock(env.getContext(), tableId, tid, lockType).tryLock()) {
9696
if (tableMustExist) {
9797
ZooReaderWriter zk = env.getContext().getZooReaderWriter();
9898
if (!zk.exists(env.getContext().getZooKeeperRoot() + Constants.ZTABLES + "/" + tableId)) {
@@ -101,29 +101,27 @@ public static long reserveTable(Manager env, TableId tableId, long tid, boolean
101101
}
102102
}
103103
log.info("table {} {} locked for {} operation: {}", tableId, FateTxId.formatTid(tid),
104-
(writeLock ? "write" : "read"), op);
104+
lockType, op);
105105
return 0;
106106
} else {
107107
return 100;
108108
}
109109
}
110110

111-
public static void unreserveTable(Manager env, TableId tableId, long tid, boolean writeLock) {
112-
getLock(env.getContext(), tableId, tid, writeLock).unlock();
113-
log.info("table {} {} unlocked for {}", tableId, FateTxId.formatTid(tid),
114-
(writeLock ? "write" : "read"));
111+
public static void unreserveTable(Manager env, TableId tableId, long tid, LockType lockType) {
112+
getLock(env.getContext(), tableId, tid, lockType).unlock();
113+
log.info("table {} {} unlocked for {}", tableId, FateTxId.formatTid(tid), lockType);
115114
}
116115

117116
public static void unreserveNamespace(Manager env, NamespaceId namespaceId, long id,
118-
boolean writeLock) {
119-
getLock(env.getContext(), namespaceId, id, writeLock).unlock();
120-
log.info("namespace {} {} unlocked for {}", namespaceId, FateTxId.formatTid(id),
121-
(writeLock ? "write" : "read"));
117+
LockType lockType) {
118+
getLock(env.getContext(), namespaceId, id, lockType).unlock();
119+
log.info("namespace {} {} unlocked for {}", namespaceId, FateTxId.formatTid(id), lockType);
122120
}
123121

124122
public static long reserveNamespace(Manager env, NamespaceId namespaceId, long id,
125-
boolean writeLock, boolean mustExist, TableOperation op) throws Exception {
126-
if (getLock(env.getContext(), namespaceId, id, writeLock).tryLock()) {
123+
LockType lockType, boolean mustExist, TableOperation op) throws Exception {
124+
if (getLock(env.getContext(), namespaceId, id, lockType).tryLock()) {
127125
if (mustExist) {
128126
ZooReaderWriter zk = env.getContext().getZooReaderWriter();
129127
if (!zk.exists(
@@ -133,7 +131,7 @@ public static long reserveNamespace(Manager env, NamespaceId namespaceId, long i
133131
}
134132
}
135133
log.info("namespace {} {} locked for {} operation: {}", namespaceId, FateTxId.formatTid(id),
136-
(writeLock ? "write" : "read"), op);
134+
lockType, op);
137135
return 0;
138136
} else {
139137
return 100;
@@ -163,27 +161,30 @@ public static void unreserveHdfsDirectory(Manager env, String directory, long ti
163161
}
164162

165163
private static Lock getLock(ServerContext context, AbstractId<?> id, long tid,
166-
boolean writeLock) {
164+
LockType lockType) {
167165
byte[] lockData = FastFormat.toZeroPaddedHex(tid);
168166
var fLockPath =
169167
FateLock.path(context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS + "/" + id.canonical());
170168
FateLock qlock = new FateLock(context.getZooReaderWriter(), fLockPath);
171169
DistributedLock lock = DistributedReadWriteLock.recoverLock(qlock, lockData);
172170
if (lock != null) {
173-
174171
// Validate the recovered lock type
175-
boolean isWriteLock = lock.getType() == LockType.WRITE;
176-
if (writeLock != isWriteLock) {
172+
if (lock.getType() != lockType) {
177173
throw new IllegalStateException("Unexpected lock type " + lock.getType()
178174
+ " recovered for transaction " + FateTxId.formatTid(tid) + " on object " + id
179-
+ ". Expected " + (writeLock ? LockType.WRITE : LockType.READ) + " lock instead.");
175+
+ ". Expected " + lockType + " lock instead.");
180176
}
181177
} else {
182178
DistributedReadWriteLock locker = new DistributedReadWriteLock(qlock, lockData);
183-
if (writeLock) {
184-
lock = locker.writeLock();
185-
} else {
186-
lock = locker.readLock();
179+
switch (lockType) {
180+
case WRITE:
181+
lock = locker.writeLock();
182+
break;
183+
case READ:
184+
lock = locker.readLock();
185+
break;
186+
default:
187+
throw new IllegalStateException("Unexpected LockType: " + lockType);
187188
}
188189
}
189190
return lock;
@@ -198,7 +199,7 @@ public static Lock getTableNameLock() {
198199
}
199200

200201
public static Lock getReadLock(Manager env, AbstractId<?> id, long tid) {
201-
return Utils.getLock(env.getContext(), id, tid, false);
202+
return Utils.getLock(env.getContext(), id, tid, LockType.READ);
202203
}
203204

204205
public static void checkNamespaceDoesNotExist(ServerContext context, String namespace,

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneTable.java

+6-5
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import org.apache.accumulo.core.data.NamespaceId;
2626
import org.apache.accumulo.core.data.TableId;
2727
import org.apache.accumulo.core.fate.Repo;
28+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
2829
import org.apache.accumulo.manager.Manager;
2930
import org.apache.accumulo.manager.tableOps.ManagerRepo;
3031
import org.apache.accumulo.manager.tableOps.Utils;
@@ -48,9 +49,9 @@ public CloneTable(String user, NamespaceId namespaceId, TableId srcTableId, Stri
4849

4950
@Override
5051
public long isReady(long tid, Manager environment) throws Exception {
51-
long val = Utils.reserveNamespace(environment, cloneInfo.srcNamespaceId, tid, false, true,
52-
TableOperation.CLONE);
53-
val += Utils.reserveTable(environment, cloneInfo.srcTableId, tid, false, true,
52+
long val = Utils.reserveNamespace(environment, cloneInfo.srcNamespaceId, tid, LockType.READ,
53+
true, TableOperation.CLONE);
54+
val += Utils.reserveTable(environment, cloneInfo.srcTableId, tid, LockType.READ, true,
5455
TableOperation.CLONE);
5556
return val;
5657
}
@@ -71,8 +72,8 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {
7172

7273
@Override
7374
public void undo(long tid, Manager environment) {
74-
Utils.unreserveNamespace(environment, cloneInfo.srcNamespaceId, tid, false);
75-
Utils.unreserveTable(environment, cloneInfo.srcTableId, tid, false);
75+
Utils.unreserveNamespace(environment, cloneInfo.srcNamespaceId, tid, LockType.READ);
76+
Utils.unreserveTable(environment, cloneInfo.srcTableId, tid, LockType.READ);
7677
}
7778

7879
}

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/CloneZookeeper.java

+6-5
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.apache.accumulo.core.clientImpl.Namespaces;
2424
import org.apache.accumulo.core.clientImpl.thrift.TableOperation;
2525
import org.apache.accumulo.core.fate.Repo;
26+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
2627
import org.apache.accumulo.core.util.tables.TableNameUtil;
2728
import org.apache.accumulo.manager.Manager;
2829
import org.apache.accumulo.manager.tableOps.ManagerRepo;
@@ -45,11 +46,11 @@ public CloneZookeeper(CloneInfo cloneInfo, ClientContext context)
4546
public long isReady(long tid, Manager environment) throws Exception {
4647
long val = 0;
4748
if (!cloneInfo.srcNamespaceId.equals(cloneInfo.namespaceId)) {
48-
val += Utils.reserveNamespace(environment, cloneInfo.namespaceId, tid, false, true,
49+
val += Utils.reserveNamespace(environment, cloneInfo.namespaceId, tid, LockType.READ, true,
4950
TableOperation.CLONE);
5051
}
51-
val +=
52-
Utils.reserveTable(environment, cloneInfo.tableId, tid, true, false, TableOperation.CLONE);
52+
val += Utils.reserveTable(environment, cloneInfo.tableId, tid, LockType.WRITE, false,
53+
TableOperation.CLONE);
5354
return val;
5455
}
5556

@@ -77,9 +78,9 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {
7778
public void undo(long tid, Manager environment) throws Exception {
7879
environment.getTableManager().removeTable(cloneInfo.tableId);
7980
if (!cloneInfo.srcNamespaceId.equals(cloneInfo.namespaceId)) {
80-
Utils.unreserveNamespace(environment, cloneInfo.namespaceId, tid, false);
81+
Utils.unreserveNamespace(environment, cloneInfo.namespaceId, tid, LockType.READ);
8182
}
82-
Utils.unreserveTable(environment, cloneInfo.tableId, tid, true);
83+
Utils.unreserveTable(environment, cloneInfo.tableId, tid, LockType.WRITE);
8384
environment.getContext().clearTableListCache();
8485
}
8586

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/clone/FinishCloneTable.java

+5-4
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.EnumSet;
2222

2323
import org.apache.accumulo.core.fate.Repo;
24+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
2425
import org.apache.accumulo.core.manager.state.tables.TableState;
2526
import org.apache.accumulo.manager.Manager;
2627
import org.apache.accumulo.manager.tableOps.ManagerRepo;
@@ -58,12 +59,12 @@ public Repo<Manager> call(long tid, Manager environment) {
5859
expectedCurrStates);
5960
}
6061

61-
Utils.unreserveNamespace(environment, cloneInfo.srcNamespaceId, tid, false);
62+
Utils.unreserveNamespace(environment, cloneInfo.srcNamespaceId, tid, LockType.READ);
6263
if (!cloneInfo.srcNamespaceId.equals(cloneInfo.namespaceId)) {
63-
Utils.unreserveNamespace(environment, cloneInfo.namespaceId, tid, false);
64+
Utils.unreserveNamespace(environment, cloneInfo.namespaceId, tid, LockType.READ);
6465
}
65-
Utils.unreserveTable(environment, cloneInfo.srcTableId, tid, false);
66-
Utils.unreserveTable(environment, cloneInfo.tableId, tid, true);
66+
Utils.unreserveTable(environment, cloneInfo.srcTableId, tid, LockType.READ);
67+
Utils.unreserveTable(environment, cloneInfo.tableId, tid, LockType.WRITE);
6768

6869
environment.getEventCoordinator().event("Cloned table %s from %s", cloneInfo.tableName,
6970
cloneInfo.srcTableId);

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactRange.java

+6-4
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.apache.accumulo.core.data.NamespaceId;
3434
import org.apache.accumulo.core.data.TableId;
3535
import org.apache.accumulo.core.fate.Repo;
36+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
3637
import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
3738
import org.apache.accumulo.core.util.FastFormat;
3839
import org.apache.accumulo.core.util.TextUtil;
@@ -89,8 +90,9 @@ public CompactRange(NamespaceId namespaceId, TableId tableId, CompactionConfig c
8990

9091
@Override
9192
public long isReady(long tid, Manager env) throws Exception {
92-
return Utils.reserveNamespace(env, namespaceId, tid, false, true, TableOperation.COMPACT)
93-
+ Utils.reserveTable(env, tableId, tid, false, true, TableOperation.COMPACT);
93+
return Utils.reserveNamespace(env, namespaceId, tid, LockType.READ, true,
94+
TableOperation.COMPACT)
95+
+ Utils.reserveTable(env, tableId, tid, LockType.READ, true, TableOperation.COMPACT);
9496
}
9597

9698
@Override
@@ -179,8 +181,8 @@ public void undo(long tid, Manager env) throws Exception {
179181
try {
180182
removeIterators(env, tid, tableId);
181183
} finally {
182-
Utils.unreserveNamespace(env, namespaceId, tid, false);
183-
Utils.unreserveTable(env, tableId, tid, false);
184+
Utils.unreserveNamespace(env, namespaceId, tid, LockType.READ);
185+
Utils.unreserveTable(env, tableId, tid, LockType.READ);
184186
}
185187
}
186188

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/cancel/CancelCompactions.java

+6-4
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.apache.accumulo.core.data.TableId;
2727
import org.apache.accumulo.core.fate.FateTxId;
2828
import org.apache.accumulo.core.fate.Repo;
29+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
2930
import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
3031
import org.apache.accumulo.manager.Manager;
3132
import org.apache.accumulo.manager.tableOps.ManagerRepo;
@@ -48,8 +49,9 @@ public CancelCompactions(NamespaceId namespaceId, TableId tableId) {
4849

4950
@Override
5051
public long isReady(long tid, Manager env) throws Exception {
51-
return Utils.reserveNamespace(env, namespaceId, tid, false, true, TableOperation.COMPACT_CANCEL)
52-
+ Utils.reserveTable(env, tableId, tid, false, true, TableOperation.COMPACT_CANCEL);
52+
return Utils.reserveNamespace(env, namespaceId, tid, LockType.READ, true,
53+
TableOperation.COMPACT_CANCEL)
54+
+ Utils.reserveTable(env, tableId, tid, LockType.READ, true, TableOperation.COMPACT_CANCEL);
5355
}
5456

5557
@Override
@@ -60,8 +62,8 @@ public Repo<Manager> call(long tid, Manager environment) throws Exception {
6062

6163
@Override
6264
public void undo(long tid, Manager env) {
63-
Utils.unreserveTable(env, tableId, tid, false);
64-
Utils.unreserveNamespace(env, namespaceId, tid, false);
65+
Utils.unreserveTable(env, tableId, tid, LockType.READ);
66+
Utils.unreserveNamespace(env, namespaceId, tid, LockType.READ);
6567
}
6668

6769
public static void mutateZooKeeper(long tid, TableId tableId, Manager environment)

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/cancel/FinishCancelCompaction.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.accumulo.core.data.NamespaceId;
2222
import org.apache.accumulo.core.data.TableId;
2323
import org.apache.accumulo.core.fate.Repo;
24+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
2425
import org.apache.accumulo.manager.Manager;
2526
import org.apache.accumulo.manager.tableOps.ManagerRepo;
2627
import org.apache.accumulo.manager.tableOps.Utils;
@@ -37,8 +38,8 @@ public FinishCancelCompaction(NamespaceId namespaceId, TableId tableId) {
3738

3839
@Override
3940
public Repo<Manager> call(long tid, Manager environment) {
40-
Utils.unreserveTable(environment, tableId, tid, false);
41-
Utils.unreserveNamespace(environment, namespaceId, tid, false);
41+
Utils.unreserveTable(environment, tableId, tid, LockType.READ);
42+
Utils.unreserveNamespace(environment, namespaceId, tid, LockType.READ);
4243
return null;
4344
}
4445

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/CreateTable.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.apache.accumulo.core.data.NamespaceId;
2828
import org.apache.accumulo.core.data.TableId;
2929
import org.apache.accumulo.core.fate.Repo;
30+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
3031
import org.apache.accumulo.manager.Manager;
3132
import org.apache.accumulo.manager.tableOps.ManagerRepo;
3233
import org.apache.accumulo.manager.tableOps.TableInfo;
@@ -60,7 +61,7 @@ public CreateTable(String user, String tableName, TimeType timeType, Map<String,
6061
@Override
6162
public long isReady(long tid, Manager environment) throws Exception {
6263
// reserve the table's namespace to make sure it doesn't change while the table is created
63-
return Utils.reserveNamespace(environment, tableInfo.getNamespaceId(), tid, false, true,
64+
return Utils.reserveNamespace(environment, tableInfo.getNamespaceId(), tid, LockType.READ, true,
6465
TableOperation.CREATE);
6566
}
6667

@@ -95,7 +96,7 @@ public void undo(long tid, Manager env) throws IOException {
9596
} catch (IOException e) {
9697
log.error("Table failed to be created and failed to clean up split files at {}", p, e);
9798
} finally {
98-
Utils.unreserveNamespace(env, tableInfo.getNamespaceId(), tid, false);
99+
Utils.unreserveNamespace(env, tableInfo.getNamespaceId(), tid, LockType.READ);
99100
}
100101
}
101102

‎server/manager/src/main/java/org/apache/accumulo/manager/tableOps/create/FinishCreateTable.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import org.apache.accumulo.core.client.admin.InitialTableState;
2525
import org.apache.accumulo.core.fate.Repo;
26+
import org.apache.accumulo.core.fate.zookeeper.DistributedReadWriteLock.LockType;
2627
import org.apache.accumulo.core.manager.state.tables.TableState;
2728
import org.apache.accumulo.manager.Manager;
2829
import org.apache.accumulo.manager.tableOps.ManagerRepo;
@@ -61,8 +62,8 @@ public Repo<Manager> call(long tid, Manager env) throws Exception {
6162
TableState.ONLINE, expectedCurrStates);
6263
}
6364

64-
Utils.unreserveNamespace(env, tableInfo.getNamespaceId(), tid, false);
65-
Utils.unreserveTable(env, tableInfo.getTableId(), tid, true);
65+
Utils.unreserveNamespace(env, tableInfo.getNamespaceId(), tid, LockType.READ);
66+
Utils.unreserveTable(env, tableInfo.getTableId(), tid, LockType.WRITE);
6667

6768
env.getEventCoordinator().event("Created table %s ", tableInfo.getTableName());
6869

0 commit comments

Comments
 (0)