[
https://issues.apache.org/jira/browse/ZOOKEEPER-2139?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15244343#comment-15244343
]
Rakesh R commented on ZOOKEEPER-2139:
-------------------------------------
Sorry for pitch in late and adding some comments. Since I was involved at the
beginning of this jira discussion I'd like to take a look at the final patch.
Thank you [~arshad.mohammad] for the good work. I found some time in reviewing
the patch, overall its pretty good. I've few comments, please take a look at it.
# Typo: CLIENT_MAX_PACKET_LENTH_DEFAULT, please correct spelling to "LENGTH"
# The default value in the following needs to be corrected, it should be
{{ENABLE_CLIENT_SASL_DEFAULT}}, right?. Also, can we call
{{ZKClientConfig.isSaslClientEnabled()}} in this method {{#isEnabled()}} to
avoid code duplication?
{code}
public static boolean isEnabled() {
return Boolean.valueOf(System.getProperty(
ZKClientConfig.ENABLE_CLIENT_SASL_KEY,
ZKClientConfig.ENABLE_CLIENT_SASL_KEY));
}
{code}
# I'd prefer to keep the default value in ZKClientConfig. Please move this
default value to this property specific class.
{code}
ClientCnxnSocket.java
+ /**
+ * 4 MB
+ */
+ private static int CLIENT_MAX_PACKET_LENTH_DEFAULT = 4096 * 1024;
{code}
# It would be good to specify the version which we are introducing
ZKClientConfig and ZKConfig, @since 3.5.2 at the class level javadoc. Also, I
think we need to add the version to {{config}} param in the constructor similar
to the existing API doc style like, {{@param canBeReadOnly}} has version
details {{(added in 3.4)}}.
{code}
* @param conf
* (added in 3.5.2) passing this conf object gives each client
the flexibility of
* configuring properties differently compared to other instances
{code}
{code}
+ * @param clientConfig
+ * (added in 3.5.2) passing this conf object gives each client
the flexibility of
+ * configuring properties differently compared to other
instances
{code}
# Please remove the extra bracket from the following function.
{code}
ZKConfig.java
public void addConfiguration(File configFile) throws ConfigException {
{
{code}
# It is good practice to add timeout parameter in tests like {{@Test(timeout =
10000)}}, please add it.
# In general, I could see there are many occurances of System.getProperty() in
the code. I think we could unify all these calls by using
ZKClientConfig#getProperty("") and ZKConfig#getProperty(""). Since the current
patch is quite big, can raise separate improvement task and work on after this
patch is committed.
Example cases:
{code}
System.getProperty(ZKClientConfig.LOGIN_CONTEXT_NAME_KEY, "Client")
System.getProperty(ZKConfig.SSL_AUTHPROVIDER, "x509")
{code}
> Support multiple ZooKeeper client, with different configurations, in a single
> JVM
> ---------------------------------------------------------------------------------
>
> Key: ZOOKEEPER-2139
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2139
> Project: ZooKeeper
> Issue Type: Improvement
> Components: java client
> Affects Versions: 3.5.0
> Reporter: Surendra Singh Lilhore
> Assignee: Arshad Mohammad
> Priority: Blocker
> Fix For: 3.5.2, 3.6.0
>
> Attachments: ZOOKEEPER-2139-05.patch, ZOOKEEPER-2139-06.patch,
> ZOOKEEPER-2139-07.patch, ZOOKEEPER-2139-08.patch, ZOOKEEPER-2139-09.patch,
> ZOOKEEPER-2139-10.patch, ZOOKEEPER-2139-11.patch, ZOOKEEPER-2139.patch,
> ZOOKEEPER-2139.patch, ZOOKEEPER-2139_1.patch, ZOOKEEPER-2139_2.patch
>
>
> I have two ZK client in one JVM, one is secure client and second is normal
> client (For non secure cluster).
> "zookeeper.sasl.client" system property is "true" by default, because of this
> my second client connection is failing.
> We should pass all client configurations in client constructor like HDFS
> client.
> For example :
> {code}
> public ZooKeeper(String connectString, int sessionTimeout, Watcher watcher,
> Configuration conf) throws IOException
> {
> ......
> ......
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
