[
https://issues.apache.org/jira/browse/BLUR-277?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13793653#comment-13793653
]
Aaron McCurry commented on BLUR-277:
------------------------------------
If at all possible it is best to avoid exceptions as logical execution paths,
especially with ZooKeeper. While this code will work, the exception is logged
in the ZooKeeper server logs as well. Overall this makes the logs noisy and
hard to read.
Also since the registering of a controller is a one time thing (the PERSISTENT
node) and because it's only done at process startup, I think is would be better
to check and create if missing. In most cases since controllers should have
unique names (unless there is a config problem), by checking and then creating
an exception should never be thrown.
Also if there is a strange race condition between controller servers where they
are both starting at the same time and they are named the same thing (bad
config). Both could pass the looping exists check and both go to register the
EPHEMERAL nodes at the same time. One wold create the EPHEMERAL node and the
other would throw an exception. A NodeExistsException exception and then that
rogue process would continue on because it ate the exception and not blow up
like it should. While I realize this is an edge case, but if it can happen it
will likely happen to someone and take a long time to debug the problem.
This is how I would rework the method:
private void registerMyself() {
String version = BlurUtil.getVersion();
String controllerPath = ZookeeperPathConstants.getControllersPath() + "/" +
_nodeName;
try {
if (_zookeeper.exists(controllerPath, false) == null) {
// Don't set the version in the registered controllers, only in the
online
_zookeeper.create(controllerPath, null, Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
}
} catch (KeeperException e) {
throw new RuntimeException(e);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
try {
String onlineControllerPath =
ZookeeperPathConstants.getOnlineControllersPath() + "/" + _nodeName;
while (_zookeeper.exists(onlineControllerPath, false) != null) {
LOG.info("Node [{0}] already registered, waiting for path [{1}] to be
released", _nodeName,
onlineControllerPath);
Thread.sleep(3000);
}
_zookeeper.create(onlineControllerPath, version.getBytes(),
Ids.OPEN_ACL_UNSAFE, CreateMode.EPHEMERAL);
} catch (KeeperException e) {
throw new RuntimeException(e);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
}
Thanks!
Aaron
> Create an API to fetch the list of total Controller Servers in the Controller
> Cluster.
> --------------------------------------------------------------------------------------
>
> Key: BLUR-277
> URL: https://issues.apache.org/jira/browse/BLUR-277
> Project: Apache Blur
> Issue Type: Sub-task
> Components: Blur
> Affects Versions: 0.3.0
> Reporter: Vikrant Navalgund
> Fix For: 0.3.0
>
> Attachments: BLUR-259-Subtask_1-MASTER.patch
>
> Original Estimate: 6h
> Remaining Estimate: 6h
>
> Right now we have an API to fetch the list of online controller servers. The
> method says getControllerServerList() which in turn gets only the online
> list.
> Suppose the Blur Shell needs a list of both the online and the offline
> controller list we have no way of getting it today. This API gets the total
> controller list and with the existing method to get the online controller
> list, the clients can do a Set difference and get the offline list.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
