Patrick Hunt commented on ZOOKEEPER-78:

comments on the c code:

the api methods are named with prefix zoo_mutex_*
seems to me it would be better to name them zoo_recipe_lock_* as prefix
this specifies that it's a recipe, in particular that it's implementing the 
"lock" recipe
We would expect (and should document in src/recipe/readme) that c api methods
should be named in this fashion, ie zoo_recipe_<recipename>_*
otw the c global namespace is going to cause problems down the line
also easier for users to identify the recipe to which a particular method call 

it's good to commit the configure scripts (see hadoop c code for which files)
this was lesson learned for cbinding & zkfuse, makes it easier for users to 
ensure binary flag set in tar package for executable scripts

would be nice to have a readme to orient the new user in src/c
include how to build (easier if configure script is here)

out of the box the patch fails to run-check
  Running Zookeeper_locktest::testlocksh: ./tests/zkServer.sh: Permission denied
  sh: ./tests/zkServer.sh: Permission denied
   : assertion : assertion
be sure to set the exec flag when committing zkServer to svn.

in tests might want to use localhost rather than (if ipv6 only host? 
or does ipv6 handle that?)

sleep(30) in test is unfortunate - it artificially inflates the test time. 
having short test time is a win
  any chance you can actively poll rather than just sleeping? something 
reusable by other recipes would be grt

I don't see any logging in zoo_lock.c, I think we should log at least the error 
conditions, also things like
loop retry at debug level. also operation is pretty complex, some logging in 
there would help.

also some similar comments as java - for example getting client_id in 
operation, this may be invalid
if the session is not established?

the api says:
 * \return the return code.
for many of the methods. what does this mean?

> added a high level protocol/feature - for easy Leader Election or exclusive 
> Write Lock creation
> -----------------------------------------------------------------------------------------------
>                 Key: ZOOKEEPER-78
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-78
>             Project: Zookeeper
>          Issue Type: New Feature
>          Components: java client
>    Affects Versions: 3.0.0
>            Reporter: james strachan
>            Assignee: Mahadev konar
>             Fix For: 3.2.0
>         Attachments: patch_with_including_Benjamin's_fix.patch, 
> using_zookeeper_facade.patch, ZOOKEEPER-78.patch, ZOOKEEPER-78.patch, 
> ZOOKEEPER-78.patch, ZOOKEEPER-78.patch, ZOOKEEPER-78.patch, 
> ZOOKEEPER-78.patch, ZOOKEEPER-78.patch
> Here's a patch which adds a little WriteLock helper class for performing 
> leader elections or creating exclusive locks in some directory znode. Note 
> its an early cut; am sure we can improve it over time. The aim is to avoid 
> folks having to use the low level ZK stuff but provide a simpler high level 
> abstraction.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

Reply via email to