hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikas Saha (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-8163) Improve ActiveStandbyElector to provide hooks for fencing old active
Date Thu, 22 Mar 2012 23:02:22 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-8163?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13236151#comment-13236151

Bikas Saha commented on HADOOP-8163:

Thanks for clarifying! I was afraid I would have to look out for code changes that might have
been covered by the "cleaned up" tests. The new test code looks cleaner. Thanks! :) I still
have to get my head around the changes to the real ZK test :P

- consider renaming zkMostRecentFilePath to something like zkMostRecentActiveFilePath. 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.

- 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?

- 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.

- 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.

- 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.

- in zkDoWithRetries there is a NUM_RETRIES field that could be used instead of 3.

- why are we exposing "public synchronized ZooKeeper getZKClient()"? What operations does
the HA service need to perform that needs direct access to the zkClient? I am stressing on
this because the Elector is trying to provide an interface for leader election and management
protocol. Hence, any necessary activity should be available through the interface. The client
of the Elector should not manipulate/read/interact directly with ZK. If it needs ZK for some
other activity then it could create another ZK client. Making this a public method adds it
to the public API of the elector and I am wary of doing it.

- the following code seems to have issues
    LOG.info("Old node exists: " + StringUtils.byteToHexString(data));


    try {
      deleteWithRetries(zkMostRecentFilePath, stat.getVersion());
    } catch (KeeperException e) {
      LOG.error("Hit error " + e + " trying to delete info znode " +
          "after fencing!", e);
      throw e;
say appClient.fenceOldActive(data) is a long running operation (and likely to be so). 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.

- 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. We could add service side logic that checks against this but IMO since
the fencing strategy via permanent znodes is an artifact of ActiveStandbyElector, the elector
should itself be guarding against this.

- 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 :P

> Improve ActiveStandbyElector to provide hooks for fencing old active
> --------------------------------------------------------------------
>                 Key: HADOOP-8163
>                 URL: https://issues.apache.org/jira/browse/HADOOP-8163
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: ha
>    Affects Versions: 0.23.3, 0.24.0
>            Reporter: Todd Lipcon
>            Assignee: Todd Lipcon
>         Attachments: hadoop-8163.txt, hadoop-8163.txt, hadoop-8163.txt
> When a new node becomes active in an HA setup, it may sometimes have to take fencing
actions against the node that was formerly active. This JIRA extends the ActiveStandbyElector
which adds an extra non-ephemeral node into the ZK directory, which acts as a second copy
of the active node's information. Then, if the active loses its ZK session, the next active
to be elected may easily locate the unfenced node to take the appropriate actions.

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


View raw message