[
https://issues.apache.org/jira/browse/ZOOKEEPER-2139?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14513559#comment-14513559
]
Rakesh R commented on ZOOKEEPER-2139:
-------------------------------------
Thanks [~surendrasingh] for the patch. Nice improvement!
Please go through the following comments. Also, please raise a [review
ticket|https://reviews.apache.org] for this, that would be easy for the
reviewers.
# Please do extends like {{ClientConfiguration extends AbstractConfiguration}}.
This way we can avoid the usage of separate {{properties}} map in our code base.
# Also, we can add system properties as follows in the ClientConfiguration
constructor instead of setting system properties explicitly.
{code}
// add system properties
addConfiguration(new SystemConfiguration());
{code}
# In ZooKeeper.java, please rename method to #getConfig()
# Please use term {{ZooKeeper}} with proper case. I could see some places
{{Zookeeper}} with 'k' small letter
# {{@param conf}} is missing in the following cases, please include the same.
{code}
public ZooKeeper(String connectString, int sessionTimeout, Watcher watcher,
boolean canBeReadOnly, HostProvider aHostProvider,
ClientConfiguration conf) throws IOException {
public ZooKeeper(String connectString, int sessionTimeout, Watcher watcher,
boolean canBeReadOnly, ClientConfiguration conf) throws IOException
{
{code}
# Please set {{@Test (timeout=)}} in tests
# Its good practise to add messages while asserting
{{Assert.assertEquals("Message reflects the case", expVal, actualVal)}}
{code}
+
Assert.assertEquals(conf.getProperty(ClientConfiguration.ZK_SASL_CLIENT_USERNAME),
"zk");
{code}
# Create the {{test.conf}} file relative to the tests, can use something like
below. Also, safe to call flush
{code}
File tmpDir = ClientBase.createTmpDir();
File confFile = new File(tmpDir, "test.conf");
FileWriter fwriter = new FileWriter(confFile);
// ....
writer.flush();
writer.close();
{code}
# Since {{ZooKeeper#getSaslClient()}} is exposed, could be an issue if anybody
calls {{isEnabled}} API in their client application code. Probably others can
give suggestions on this part. I suggest retain the following code and we can
refer the constants to {{ClientConfiguration.ZK_SASL_CLIENT_USERNAME}} etc.
Then we can mark these are deprecated, what do you say?
# Please mention the overriding precedence in the {{ClientConfiguration}}
javadoc like, System property -> user sets value -> file etc.
> Zookeeper client configuration should not be java system property
> -----------------------------------------------------------------
>
> 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: surendra singh lilhore
> Fix For: 3.5.2, 3.6.0
>
> Attachments: 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)
