ilePath. zkMostRecentFilePath is open to being misunderstood. Same for MOST_RECENT_FILENAME.
actually MostRecent seems to be a misnomer to me. I think it actually is LockOwnerInfo/LeaderInfo. zkLockOwnerInfoPath/tryDeleteLeaderInfo etc.
It's not always the lock owner, though. Basically, we go through the following states:
||Node A acquires active lock
||..and writes its own info
||A loses its ZK lease
||Node B acquires active lock
||Node B fences node A
||Node B writes its info
So, in steps 3 and 7, calling it "LeaderInfo" or "LockOwnerInfo" makes sense. But in steps 4 and 5, it's the "PreviousLeaderInfo".
Perhaps just renaming to "LeaderBreadcrumb" or something makes more sense, since it's basically a bread crumb left around by the previous leader so that future leaders know its info.
why is ensureBaseNode() needed? In it we are creating a new set of znodes with the given zkAcls which may or may not be the correct thing. eg. if the admin simply forgot to create the appropriate znode path before starting the service it might be ok to fail. Instead of trying to create the path ourselves with permissions that may or may not be appropriate for the entire path. I would be wary of doing this. What is the use case?
The use case is a "ZKFailoverController -formatZK" command line tool that I'm building into the ZKFC code. The thinking is that most administrators won't want to go into the ZK CLI to manually create the parent znode while installing HDFS. Instead, they'd rather just issue this simple command. In the case that they want to have varying permissions across the path, or some more complicated ACL, then they'll have to use the ZK CLI, but for the common case I think this will make deployment much simpler.
consider renaming baseNodeExists() to parentNodeExists() or renaming the parentZnodeName parameter in the constructor to baseNode for consistency. Perhaps this could be called in the constructor to check that the znode exists and be done with config issues. No need for ensureBaseNode() above.
Renamed to parentZNodeExists and ensureParentZNode
this must be my newbie java skills but I find something like - prefixPath.append("/").append(pathParts[index]) or znodeWorkingDir.subString(0, znodeWorkingDir.nextIndexOf('/')) - more readable than prefixPath = Joiner.on("/").join(Arrays.asList(pathParts).subList(0, i)). It might also be more efficient but thats not relevant for this situation.
public synchronized void quitElection(boolean needFence) - Dont we want to delete the permanent znode for standby's too? Why check if state is active. It anyways calls a tryDelete* method that should be harmless.
If the node is standby, then the permanent znode refers to the current lockholder. So deleting it would incorrectly signify that whoever is active doesn't need to be fenced if it crashes.
tryDeleteMostRecentNode() - From my understanding of "tryFunction" - this function should be not really be asserting that some state holds. If it should assert then we should remove "try" from the name.
The difference here is this: the assert() guards against programmer error. It is a mistake to call this function when you aren't active (see above comment). But if there is a ZK error (like the session got lost) it's OK to fail to delete it, since it just means that the node will get fenced.
in zkDoWithRetries there is a NUM_RETRIES field that could be used instead of 3.
why are we exposing "public synchronized ZooKeeper getZKClient()"?
the following code seems to have issues... <snip>... While that is happening, the state of the world changes and this elector is not longer the lock owner. When appClient.fenceOldActive(data) will complete then the code will go ahead and delete the lockOwnerZnode at zkMostRecentFilePath. This node could be from the new leader who had successfully fenced and become active. The version number parameter might accidentally save us but would likely be 0 all the time.
This scenario is impossible for the following reason: If the state of the world changed and this node was no longer active, the only possible reason for that is that the node lost its ZK session lease. If that's the case, then it won't be able to issue any further commands from that client (see my conversation with Hari above)
what happens if the leader lost the lock, tried to delete its znode, failed to do so, exited anyways, then became the next owner and found the existing mostrecent znode. I think it will try to fence itself
I'm currently handling this situation at the ZKFailoverController level. Are you suggesting we determine "self" by comparing the actual bytewise data of the znode, and skip the fence call if it's the same as this elector instance's appData? Seems reasonable, just want to clarify.
It would be great to see the functional test with RealZK enhanced to cover some of the permanent znode use cases. I remember fixing some issues in my own code that were exposed to the reality of the real ZK
Added a small change to the test to make sure the fencing code got exercised. I also plan to add some more integration-level tests using real ZK and the ZKFailoverController with that patch.