[
https://issues.apache.org/jira/browse/ZOOKEEPER-2391?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15219201#comment-15219201
]
Edward Ribeiro commented on ZOOKEEPER-2391:
-------------------------------------------
Hi [~kazuakibanzai], nice work. A couple of points regarding your patches:
1. It would be cool if you could follow the same code syntax rules of ZK
codebase. For example:
{code}
if(this.minSessionTimeout > maxSessionTimeout){
{code}
can be modified by inserting a space between the {{if}} and the {{(}} and
before the curly braces, as below:
{code}
if (this.minSessionTimeout > maxSessionTimeout) {
{code}
See the rest of the code in the same files you are changing for clues. ;)
2. Could you document the changes in the
{{src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml}} file? There are
sections devoted to maxSessionTimeout and minSessionTimeout there. When
compiled, those xml files generate the ZK pdf docs, so it would be cool if you
add a bit of info to reflect the code changes too (for example, what happens if
< -1, == 1, etc).
3. There's a small typo in the javadoc comment above {{public void
setMaxSessionTimeout(int max)}}:
{{max minSessionTimeout}} should be {{max maxSessionTimeout}} (copy-and-paste
artifact, I guess ;) )
4. Also, I'd suggest to beef up the javadoc comment. An example, but keep it in
my that this is only a rough *suggestion*, please adjust if you feel like doing
so:
{code}
/**
* @param max session timeout in milliseconds, if max value is -1 the
default value is set.
* @throws IllegalArgumentException if max timeout < -1 or min timeout >
max timeout
*/
{code}
> setMin/MaxSessionTimeout of ZookeeperServer are implemented in quite a weak
> way
> -------------------------------------------------------------------------------
>
> Key: ZOOKEEPER-2391
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2391
> Project: ZooKeeper
> Issue Type: Bug
> Components: server
> Reporter: Kazuaki Banzai
> Assignee: Kazuaki Banzai
> Priority: Minor
> Attachments: ZOOKEEPER-2391-3.4.patch, ZOOKEEPER-2391.patch
>
>
> setMin/MaxSessionTimeout of ZookeeperServer are implemented in quite a weak
> way.
> * -1 restores the default, but this is not documented.
> * values < -1 are permitted but make no sense.
> * min > max is permitted but makes not sense.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
