huaxiangsun commented on a change in pull request #2454:
URL: https://github.com/apache/hbase/pull/2454#discussion_r498993561
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
##########
@@ -1953,20 +1952,27 @@ public SetNormalizerRunningResponse
setNormalizerRunning(RpcController controlle
rpcPreCheck("setNormalizerRunning");
// Sets normalizer on/off flag in ZK.
- boolean prevValue = master.getRegionNormalizerTracker().isNormalizerOn();
- boolean newValue = request.getOn();
- try {
- master.getRegionNormalizerTracker().setNormalizerOn(newValue);
- } catch (KeeperException ke) {
- LOG.warn("Error flipping normalizer switch", ke);
- }
+ // TODO: this method is totally broken in terms of atomicity of actions
and values read.
+ // 1. The contract has this RPC returning the previous value. There isn't
a ZKUtil method
+ // that lets us retrieve the previous value as part of setting a new
value, so we simply
+ // perform a read before issuing the update. Thus we have a data race
opportunity, between
+ // when the `prevValue` is read and whatever is actually overwritten.
+ // 2. Down in `setNormalizerOn`, the call to `createAndWatch` inside of
the catch clause can
+ // itself fail in the event that the znode already exists. Thus,
another data race, between
+ // when the initial `setData` call is notified of the absence of the
target znode and the
+ // subsequent `createAndWatch`, with another client creating said node.
+ // That said, there's supposed to be only one active master and thus
there's supposed to be
+ // only one process with the authority to modify the value.
Review comment:
Nice comments!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
