Mahadev konar commented on ZOOKEEPER-38:

the patch looks good --- here are some of my comments --- 

also I am not finished reviewing it :). its a long patch --- 

0) overall there are too many interfaces in the patch. though i uinderstand 
that interfaces make things pluggable but I think we should restrict only wehn 
we need it and create interfaces only when we seriously think that it would be 
good to have it pluggable.

1) DataTereeBuilder is an interface and has only one subclass --- i think that 
we should not make this pluggable and just have a datatree class instead of the 
interfaces and implementing classes.
2) FileDBINFO implements DBINFO but  FILEDBINFO Is the only subclass of DBINFO. 
I think here again we should just keep it simple and not make this an interface 
and pluggable.
3) Snapshotting plicy is again pluggable -- we should also not make this 
pluggable and let it work our default implementation.
4) we have arguments passing around as url though there are not true url's in 
java -- but just strings. if we really want to make this pluggable and use 
url's then we should use URL class in java and use its methods for checking 
what kind of provider we want it to return. like create a URL(string) and 
url.getProtocol.equals("file") then we retrun our default one.
5) i dont see any use case of more persistence layers as -D expressions? what 
if we had something like this 

-Dpersisten.provider=classname and then just use reflections to create the 
provider class else we just return the default provider. I think it makes the 
code simpler.

i am not done reviewing the patch entirely. :) 

> headers (version+) in log/snap files
> ------------------------------------
>                 Key: ZOOKEEPER-38
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-38
>             Project: Zookeeper
>          Issue Type: New Feature
>          Components: server
>            Reporter: Patrick Hunt
>            Assignee: Andrew Kornev
>             Fix For: 3.0.0
>         Attachments: ZOOKEEPER-38.patch, ZOOKEEPER-38.patch
> Moved from SourceForge to Apache.
> http://sourceforge.net/tracker/index.php?func=detail&aid=1961767&group_id=209147&atid=1008547

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