[GitHub] [zookeeper] dineshappavoo commented on a change in pull request #1025: [ZOOKEEPER-3471] Fix potential lock unavailable due to dangling ephemeral nodes left during local session upgrading

2019-08-12 Thread GitBox
dineshappavoo commented on a change in pull request #1025: [ZOOKEEPER-3471] Fix 
potential lock unavailable due to dangling ephemeral nodes left during local 
session upgrading
URL: https://github.com/apache/zookeeper/pull/1025#discussion_r313177914
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
 ##
 @@ -992,6 +990,7 @@ public void submitRequestNow(Request si) {
 touch(si.cnxn);
 boolean validpacket = Request.isValid(si.type);
 if (validpacket) {
+setLocalSessionFlag(si);
 
 Review comment:
   Thanks @lvfangmin . I think I misunderstood. It was good to clarify and I 
checked little deeper after that. Yes, now I understood all the requests go 
through `RequestThrottler`. 
   
   And the change looks good to me.  Thanks for fixing it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520634570
 
 
   With read comments above, I'd like to propose group rules in different 
concerns into different issue/pull request. Initially on the mailing list we 
reach a consensus on enabling checkstyle configuration per package/module 
instead of per rules but as described above different type of rules need 
different attention. Basically, it would be
   
   A basic checkstyle rules, that is, the same as current 
`checkstyle-strict.xml` excludes *Names rules, JavaDoc rules and LineBreak. 
Details as below.
   
   - A new JIRA issue to track a revisit of *Names rules as well as variable 
visibility. The leverage is between backward compatibility to strict 
visibility. For example `public static final` should be ALL_CAPS ideally but 
for backward compatibility we should retain; while a series of `(default) 
static final` is occasionally package-private but semantically should be 
`private static final`.
   
   - JavaDoc is not just about checkstyle. As filed in ZOOKEEPER-3469 we need a 
dedicated pass for reviewing changes in JavaDoc regard the changes in 
documentation scope as well.
   
   - LineBreak is suppressed as well. There is not a general ideal how to break 
a line and it requires a discussion on code style in Flink community[1]. I 
would prefer suppress it at least for now or even we reach a consensus that we 
don't need it.
   
   - Fair enough, log format deserves a JIRA issue although it might out of the 
scope here.
   
   And consequently, the basic checkstyle configuration focuses on whitespaces, 
imports, operators on a new line, redundant or out-of-order modifiers, TODO 
comments, and if/else also with curly.
   
   It's ok to me that this pull request as a prototype on bringing checkstyle 
to zookeeper-server and a total rebase require one or two days doesn't inflict 
too much. It is more important to see where our community decide to go.
   
   [1] 
https://lists.apache.org/thread.html/609112aa8d0a0d3260bfca4f34ca405f39e2f44bf6bcba010068266e@%3Cdev.flink.apache.org%3E


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520634570
 
 
   With read comments above, I'd like to propose group rules in different 
concerns into different issue/pull request. Initially on the mailing list we 
reach a consensus on enabling checkstyle configuration per package/module 
instead of per rules but as described above different type of rules need 
different attention. Basically, it would be
   
   A basic checkstyle rules, that is, the same as current 
`checkstyle-strict.xml` excludes *Names rules, JavaDoc rules and LineBreak. 
Details as below.
   
   - A new JIRA issue to track a revisit of *Names rules as well as variable 
visibility. The leverage is between backward compatibility to strict 
visibility. For example `public static` should be ALL_CAPS ideally but for 
backward compatibility we should retain; while a series of `(default) static` 
is occasionally package-private but semantically should be `private static`.
   
   - JavaDoc is not just about checkstyle. As filed in ZOOKEEPER-3469 we need a 
dedicated pass for reviewing changes in JavaDoc regard the changes in 
documentation scope as well.
   
   - LineBreak is suppressed as well. There is not a general ideal how to break 
a line and it requires a discussion on code style in Flink community[1]. I 
would prefer suppress it at least for now or even we reach a consensus that we 
don't need it.
   
   - Fair enough, log format deserves a JIRA issue although it might out of the 
scope here.
   
   And consequently, the basic checkstyle configuration focuses on whitespaces, 
imports, operators on a new line, redundant or out-of-order modifiers, and 
if/else also with curly.
   
   It's ok to me that this pull request as a prototype on bringing checkstyle 
to zookeeper-server and a total rebase require one or two days doesn't inflict 
too much. It is more important to see where our community decide to go.
   
   [1] 
https://lists.apache.org/thread.html/609112aa8d0a0d3260bfca4f34ca405f39e2f44bf6bcba010068266e@%3Cdev.flink.apache.org%3E


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520634570
 
 
   With read comments above, I'd like to propose group rules in different 
concerns into different issue/pull request. Initially on the mailing list we 
reach a consensus on enabling checkstyle configuration per package/module 
instead of per rules but as described above different type of rules need 
different attention. Basically, it would be
   
   A basic checkstyle rules, that is, the same as current 
`checkstyle-strict.xml` excludes *Names rules, JavaDoc rules and LineBreak. 
Details as below.
   
   - A new JIRA issue to track a revisit of *Names rules as well as variable 
visibility. The leverage is between backward compatibility to strict 
visibility. For example `public static` should be ALL_CAPS ideally but for 
backward compatibility we should retain; while a series of `(default) static` 
is occasionally package-private but semantically should be `private static`.
   
   - JavaDoc is not just about checkstyle. As filed in ZOOKEEPER-3469 we need a 
dedicated pass for reviewing changes in JavaDoc regard the changes in 
documentation scope as well.
   
   - LineBreak is suppressed as well. There is not a general ideal how to break 
a line and it requires a discussion on code style in Flink community[1]. I 
would prefer suppress it at least for now or even we reach a consensus that we 
don't need it.
   
   - Fair enough, log format deserves a JIRA issue although it might out of the 
scope here.
   
   And consequently, the basic checkstyle configuration focuses on whitespaces, 
imports, operators on a new line, redundant or out-of-order modifiers, TODO 
comments, and if/else also with curly.
   
   It's ok to me that this pull request as a prototype on bringing checkstyle 
to zookeeper-server and a total rebase require one or two days doesn't inflict 
too much. It is more important to see where our community decide to go.
   
   [1] 
https://lists.apache.org/thread.html/609112aa8d0a0d3260bfca4f34ca405f39e2f44bf6bcba010068266e@%3Cdev.flink.apache.org%3E


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520634570
 
 
   With read comments above, I'd like to propose group rules in different 
concerns into different issue/pull request. Initially on the mailing list we 
reach a consensus on enabling checkstyle configuration per package/module 
instead of per rules but as described above different type of rules need 
different attention. Basically, it would be
   
   A basic checkstyle rules, that is, the same as current 
`checkstyle-strict.xml` excludes *Names rules, JavaDoc rules and LineBreak. 
Details as below.
   
   - A new JIRA issue to track a revisit of *Names rules as well as variable 
visibility. The leverage is between backward compatibility to strict 
visibility. For example `public static` should be ALL_CAPS ideally but for 
backward compatibility we should retain; while a series of `(default) static` 
is occasionally package-private but semantically should be `private static`.
   
   - JavaDoc is not just about checkstyle. As filed in ZOOKEEPER-3469 we need a 
dedicated pass for reviewing changes in JavaDoc regard the changes in 
documentation scope as well.
   
   - LineBreak is suppressed as well. There is not a general ideal how to break 
a line and it requires a discussion on code style in Flink community[1]. I 
would prefer suppress it at least for now or even we reach a consensus that we 
don't need it.
   
   - Fair enough, log format deserves a JIRA issue although it might out of the 
scope here.
   
   And consequently, the basic checkstyle configuration focuses on whitespaces, 
imports, operators on a new line, and if/else also with curly.
   
   It's ok to me that this pull request as a prototype on bringing checkstyle 
to zookeeper-server and a total rebase require one or two days doesn't inflict 
too much. It is more important to see where our community decide to go.
   
   [1] 
https://lists.apache.org/thread.html/609112aa8d0a0d3260bfca4f34ca405f39e2f44bf6bcba010068266e@%3Cdev.flink.apache.org%3E


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
hanm commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration 
on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520616825
 
 
   @TisonKun sounds good.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313153091
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
 ##
 @@ -202,78 +201,81 @@
 }
 }
 
-static public InitialMessage parse(Long protocolVersion, 
DataInputStream din)
-throws InitialMessageException, IOException {
+public static InitialMessage parse(
+Long protocolVersion,
+DataInputStream din
+) throws InitialMessageException, IOException {
 Long sid;
 
 if (protocolVersion != PROTOCOL_VERSION) {
-throw new InitialMessageException(
-"Got unrecognized protocol version %s", 
protocolVersion);
+throw new InitialMessageException("Got unrecognized protocol 
version %s", protocolVersion);
 }
 
 sid = din.readLong();
 
 int remaining = din.readInt();
 if (remaining <= 0 || remaining > maxBuffer) {
-throw new InitialMessageException(
-"Unreasonable buffer length: %s", remaining);
+throw new InitialMessageException("Unreasonable buffer length: 
%s", remaining);
 }
 
 byte[] b = new byte[remaining];
-int num_read = din.read(b);
+int numRead = din.read(b);
 
 Review comment:
   @enixon I think my main argument on proposing of doing the naming changes as 
a future pull request instead of as part of this pull request is to ensure this 
pull request does not contain any semantic changes, make both this and the 
future pull request easier to review and evaluate. That approach has the down 
side of having to do another (or other sets of) pull requests for enabling more 
check styles rules, but I feel the benefit of staging the work out weight the 
cost of invalidate any PR or causing internal private code rebase multiple 
times, as each future pull request will be well scoped, and the amortized cost 
would likely be low for rebasing etc.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1037: ZOOKEEPER-3492: Add weights to server side connection throttling

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1037: ZOOKEEPER-3492: Add weights 
to server side connection throttling
URL: https://github.com/apache/zookeeper/pull/1037#discussion_r313121704
 
 

 ##
 File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
 ##
 @@ -822,6 +822,27 @@ property, when available, is noted below.
 dropping. This parameter defines the threshold to decrease the dropping
 probability. The default is 0.
 
+* *zookeeper.connection_throttle_weight_enabled* :
+(Java system property only)
+**New in 3.6.0:**
+Whether to consider connection weights when throttling. Only useful when 
connection throttle is enabled, that is, connectionMaxTokens is larger than 0. 
The default is false.
+
+* *zookeeper.connection_throttle_global_session_weight* :
+(Java system property only)
+**New in 3.6.0:**
+The weight of a global session. It is the number of tokens required for a 
global session request to get through the connection throttler. The default is 
3.
 
 Review comment:
   Aside from default value, it might also worth mention only positive integers 
are allowed.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1037: ZOOKEEPER-3492: Add weights to server side connection throttling

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1037: ZOOKEEPER-3492: Add weights 
to server side connection throttling
URL: https://github.com/apache/zookeeper/pull/1037#discussion_r313123296
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/BlueThrottle.java
 ##
 @@ -86,36 +90,106 @@
 Random rng;
 
 public static final String CONNECTION_THROTTLE_TOKENS = 
"zookeeper.connection_throttle_tokens";
-public static final int DEFAULT_CONNECTION_THROTTLE_TOKENS;
+private static final int DEFAULT_CONNECTION_THROTTLE_TOKENS;
 
 public static final String CONNECTION_THROTTLE_FILL_TIME = 
"zookeeper.connection_throttle_fill_time";
-public static final int DEFAULT_CONNECTION_THROTTLE_FILL_TIME;
+private static final int DEFAULT_CONNECTION_THROTTLE_FILL_TIME;
 
 public static final String CONNECTION_THROTTLE_FILL_COUNT = 
"zookeeper.connection_throttle_fill_count";
-public static final int DEFAULT_CONNECTION_THROTTLE_FILL_COUNT;
+private static final int DEFAULT_CONNECTION_THROTTLE_FILL_COUNT;
 
 public static final String CONNECTION_THROTTLE_FREEZE_TIME = 
"zookeeper.connection_throttle_freeze_time";
-public static final int DEFAULT_CONNECTION_THROTTLE_FREEZE_TIME;
+private static final int DEFAULT_CONNECTION_THROTTLE_FREEZE_TIME;
 
 public static final String CONNECTION_THROTTLE_DROP_INCREASE = 
"zookeeper.connection_throttle_drop_increase";
-public static final double DEFAULT_CONNECTION_THROTTLE_DROP_INCREASE;
+private static final double DEFAULT_CONNECTION_THROTTLE_DROP_INCREASE;
 
 public static final String CONNECTION_THROTTLE_DROP_DECREASE = 
"zookeeper.connection_throttle_drop_decrease";
-public static final double DEFAULT_CONNECTION_THROTTLE_DROP_DECREASE;
+private static final double DEFAULT_CONNECTION_THROTTLE_DROP_DECREASE;
 
 public static final String CONNECTION_THROTTLE_DECREASE_RATIO = 
"zookeeper.connection_throttle_decrease_ratio";
-public static final double DEFAULT_CONNECTION_THROTTLE_DECREASE_RATIO;
+private static final double DEFAULT_CONNECTION_THROTTLE_DECREASE_RATIO;
+
+public static final String WEIGHED_CONNECTION_THROTTLE = 
"zookeeper.connection_throttle_weight_enabled";
+private static boolean connectionWeightEnabled;
+
+public static final String GLOBAL_SESSION_WEIGHT = 
"zookeeper.connection_throttle_global_session_weight";
+private static final int DEFAULT_GLOBAL_SESSION_WEIGHT;
+
+public static final String LOCAL_SESSION_WEIGHT = 
"zookeeper.connection_throttle_local_session_weight";
+private static final int DEFAULT_LOCAL_SESSION_WEIGHT;
 
+public static final String RENEW_SESSION_WEIGHT = 
"zookeeper.connection_throttle_renew_session_weight";
+private static final int DEFAULT_RENEW_SESSION_WEIGHT;
+
+// for unit tests only
+protected  static void setConnectionWeightEnabled(boolean enabled) {
+connectionWeightEnabled = enabled;
+logWeighedThrottlingSetting();
+}
+
+private static void logWeighedThrottlingSetting() {
+if (connectionWeightEnabled) {
+LOG.info("Weighed connection throttling is enabled. " +
+"But it will only be effective if connection throttling is 
enabled");
+LOG.info(
+"The weights for different session types are: global {} 
renew {} local {}",
+DEFAULT_GLOBAL_SESSION_WEIGHT,
+DEFAULT_RENEW_SESSION_WEIGHT,
+DEFAULT_LOCAL_SESSION_WEIGHT
+);
+} else {
+LOG.info("Weighed connection throttling is disabled");
+}
+}
 
 static {
-DEFAULT_CONNECTION_THROTTLE_TOKENS = 
Integer.getInteger(CONNECTION_THROTTLE_TOKENS, 0);
-DEFAULT_CONNECTION_THROTTLE_FILL_TIME = 
Integer.getInteger(CONNECTION_THROTTLE_FILL_TIME, 1);
-DEFAULT_CONNECTION_THROTTLE_FILL_COUNT = 
Integer.getInteger(CONNECTION_THROTTLE_FILL_COUNT, 1);
+int tokens = Integer.getInteger(CONNECTION_THROTTLE_TOKENS, 0);
+int fillCount = Integer.getInteger(CONNECTION_THROTTLE_FILL_COUNT, 1);
+
+connectionWeightEnabled = 
Boolean.getBoolean(WEIGHED_CONNECTION_THROTTLE);
+
+// if not specified, the weights for a global session, a local 
session, and a renew session
+// are 3, 1, 2 respectively. The weight for a global session is 3 
because in our connection benchmarking,
+// the throughput of global sessions is about one third of that of 
local sessions. Renewing a session
+// requires is more expensive than establishing a local session and 
cheaper than creating a global session so
 
 Review comment:
   It seems obvious (even without benchmarking) that the cost of creating a 
global session is greater than a local session.
   So, do we need enforce that the weight on a global session is always bigger 
than a local session? Currently there is no such constraint and user can put 

[GitHub] [zookeeper] hanm commented on a change in pull request #1037: ZOOKEEPER-3492: Add weights to server side connection throttling

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1037: ZOOKEEPER-3492: Add weights 
to server side connection throttling
URL: https://github.com/apache/zookeeper/pull/1037#discussion_r313117213
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/BlueThrottle.java
 ##
 @@ -86,36 +90,106 @@
 Random rng;
 
 public static final String CONNECTION_THROTTLE_TOKENS = 
"zookeeper.connection_throttle_tokens";
-public static final int DEFAULT_CONNECTION_THROTTLE_TOKENS;
+private static final int DEFAULT_CONNECTION_THROTTLE_TOKENS;
 
 public static final String CONNECTION_THROTTLE_FILL_TIME = 
"zookeeper.connection_throttle_fill_time";
-public static final int DEFAULT_CONNECTION_THROTTLE_FILL_TIME;
+private static final int DEFAULT_CONNECTION_THROTTLE_FILL_TIME;
 
 public static final String CONNECTION_THROTTLE_FILL_COUNT = 
"zookeeper.connection_throttle_fill_count";
-public static final int DEFAULT_CONNECTION_THROTTLE_FILL_COUNT;
+private static final int DEFAULT_CONNECTION_THROTTLE_FILL_COUNT;
 
 public static final String CONNECTION_THROTTLE_FREEZE_TIME = 
"zookeeper.connection_throttle_freeze_time";
-public static final int DEFAULT_CONNECTION_THROTTLE_FREEZE_TIME;
+private static final int DEFAULT_CONNECTION_THROTTLE_FREEZE_TIME;
 
 public static final String CONNECTION_THROTTLE_DROP_INCREASE = 
"zookeeper.connection_throttle_drop_increase";
-public static final double DEFAULT_CONNECTION_THROTTLE_DROP_INCREASE;
+private static final double DEFAULT_CONNECTION_THROTTLE_DROP_INCREASE;
 
 public static final String CONNECTION_THROTTLE_DROP_DECREASE = 
"zookeeper.connection_throttle_drop_decrease";
-public static final double DEFAULT_CONNECTION_THROTTLE_DROP_DECREASE;
+private static final double DEFAULT_CONNECTION_THROTTLE_DROP_DECREASE;
 
 public static final String CONNECTION_THROTTLE_DECREASE_RATIO = 
"zookeeper.connection_throttle_decrease_ratio";
-public static final double DEFAULT_CONNECTION_THROTTLE_DECREASE_RATIO;
+private static final double DEFAULT_CONNECTION_THROTTLE_DECREASE_RATIO;
+
+public static final String WEIGHED_CONNECTION_THROTTLE = 
"zookeeper.connection_throttle_weight_enabled";
+private static boolean connectionWeightEnabled;
+
+public static final String GLOBAL_SESSION_WEIGHT = 
"zookeeper.connection_throttle_global_session_weight";
+private static final int DEFAULT_GLOBAL_SESSION_WEIGHT;
+
+public static final String LOCAL_SESSION_WEIGHT = 
"zookeeper.connection_throttle_local_session_weight";
+private static final int DEFAULT_LOCAL_SESSION_WEIGHT;
 
+public static final String RENEW_SESSION_WEIGHT = 
"zookeeper.connection_throttle_renew_session_weight";
+private static final int DEFAULT_RENEW_SESSION_WEIGHT;
+
+// for unit tests only
+protected  static void setConnectionWeightEnabled(boolean enabled) {
+connectionWeightEnabled = enabled;
+logWeighedThrottlingSetting();
+}
+
+private static void logWeighedThrottlingSetting() {
+if (connectionWeightEnabled) {
+LOG.info("Weighed connection throttling is enabled. " +
+"But it will only be effective if connection throttling is 
enabled");
+LOG.info(
+"The weights for different session types are: global {} 
renew {} local {}",
+DEFAULT_GLOBAL_SESSION_WEIGHT,
+DEFAULT_RENEW_SESSION_WEIGHT,
+DEFAULT_LOCAL_SESSION_WEIGHT
+);
+} else {
+LOG.info("Weighed connection throttling is disabled");
+}
+}
 
 static {
-DEFAULT_CONNECTION_THROTTLE_TOKENS = 
Integer.getInteger(CONNECTION_THROTTLE_TOKENS, 0);
-DEFAULT_CONNECTION_THROTTLE_FILL_TIME = 
Integer.getInteger(CONNECTION_THROTTLE_FILL_TIME, 1);
-DEFAULT_CONNECTION_THROTTLE_FILL_COUNT = 
Integer.getInteger(CONNECTION_THROTTLE_FILL_COUNT, 1);
+int tokens = Integer.getInteger(CONNECTION_THROTTLE_TOKENS, 0);
+int fillCount = Integer.getInteger(CONNECTION_THROTTLE_FILL_COUNT, 1);
+
+connectionWeightEnabled = 
Boolean.getBoolean(WEIGHED_CONNECTION_THROTTLE);
+
+// if not specified, the weights for a global session, a local 
session, and a renew session
+// are 3, 1, 2 respectively. The weight for a global session is 3 
because in our connection benchmarking,
+// the throughput of global sessions is about one third of that of 
local sessions. Renewing a session
+// requires is more expensive than establishing a local session and 
cheaper than creating a global session so
+// its default weight is set to 2.
+int setting = Integer.getInteger(GLOBAL_SESSION_WEIGHT, 3);
+if (setting <= 0) {
+LOG.warn("Invalid global session weight {}. It should be larger 
than 0");
 
 Review comment:
   Missing the log parameter (the intention was to 

[GitHub] [zookeeper] hanm commented on a change in pull request #1051: Add server side large request throttling

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1051: Add server side large request 
throttling
URL: https://github.com/apache/zookeeper/pull/1051#discussion_r313147680
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
 ##
 @@ -221,6 +221,37 @@
 private RequestThrottler requestThrottler;
 public static final String SNAP_COUNT = "zookeeper.snapCount";
 
+/**
+ * This setting sets a limit on the total number of large requests that
+ * can be inflight and is designed to prevent ZooKeeper from accepting
+ * too many large requests such that the JVM runs out of usable heap and
+ * ultimately crashes.
+ *
+ * The limit is enforced by the {@link checkRequestSize(int, boolean)}
+ * method which is called by the connection layer ({@link NIOServerCnxn},
+ * {@link NettyServerCnxn}) before allocating a byte buffer and pulling
+ * data off the TCP socket. The limit is then checked again by the
+ * ZooKeeper server in {@link processPacket(ServerCnxn, ByteBuffer)} which
+ * also atomically updates {@link currentLargeRequestBytes}. The request is
+ * then marked as a large request, with the request size stored in the 
Request
+ * object so that it can later be decremented from {@link 
currentLargeRequestsBytes}.
+ *
+ * When a request is completed or dropped, the relevant code path calls the
+ * {@link requestFinished(Request)} method which performs the decrement if
+ * needed.
+ */
+private volatile int largeRequestMaxBytes =
+Integer.getInteger("zookeeper.largeRequestMaxBytes", 100 * 1024 * 
1024);
+
+/**
+ * The size threshold after which a request is considered a large request
+ * and is checked against the large request byte limit.
+ */
+private volatile int largeRequestThreshold =
 
 Review comment:
   I think it's necessary to do a validation of the values on these two newly 
added property, and throw argument exceptions if they are off the chart (and 
update doc to reflect the valid ranges.).


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1051: Add server side large request throttling

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1051: Add server side large request 
throttling
URL: https://github.com/apache/zookeeper/pull/1051#discussion_r313144393
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
 ##
 @@ -1358,6 +1395,68 @@ static void setMaxBatchSize(int size) {
 maxBatchSize = size;
 }
 
+public int getLargeRequestMaxBytes() {
+return largeRequestMaxBytes;
+}
+
+public void setLargeRequestMaxBytes(int bytes) {
+largeRequestMaxBytes = bytes;
+}
+
+public int getLargeRequestThreshold() {
+return largeRequestThreshold;
+}
+
+public void setLargeRequestThreshold(int threshold) {
+largeRequestThreshold = threshold;
+}
+
+public int getLargeRequestBytes() {
+return currentLargeRequestBytes.get();
+}
+
+private boolean isLargeRequest(int length) {
+// The large request limit is disabled when threshold is -1
+if (largeRequestThreshold == -1) {
+return false;
+}
+return length > largeRequestThreshold;
+}
+
+private boolean testRequestSize(int length, boolean increment) {
+if (!isLargeRequest(length)) {
+// Always allow small requests
+return true;
+}
+
+LOG.info("length {} increment {}", length, increment);
 
 Review comment:
   This `testRequestSize` is on critical path of every request (when every 
request is a large request - or when it's configured that way with -1), having 
three `LOG.info` in this function will probably lead to miserable performance.
   
   Should we change it to DEBUG or just remove the log completely?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1051: Add server side large request throttling

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1051: Add server side large request 
throttling
URL: https://github.com/apache/zookeeper/pull/1051#discussion_r313145645
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
 ##
 @@ -1358,6 +1395,68 @@ static void setMaxBatchSize(int size) {
 maxBatchSize = size;
 }
 
+public int getLargeRequestMaxBytes() {
+return largeRequestMaxBytes;
+}
+
+public void setLargeRequestMaxBytes(int bytes) {
+largeRequestMaxBytes = bytes;
+}
+
+public int getLargeRequestThreshold() {
+return largeRequestThreshold;
+}
+
+public void setLargeRequestThreshold(int threshold) {
+largeRequestThreshold = threshold;
+}
+
+public int getLargeRequestBytes() {
+return currentLargeRequestBytes.get();
+}
+
+private boolean isLargeRequest(int length) {
+// The large request limit is disabled when threshold is -1
+if (largeRequestThreshold == -1) {
+return false;
+}
+return length > largeRequestThreshold;
+}
+
+private boolean testRequestSize(int length, boolean increment) {
 
 Review comment:
   having a boolean parameter in a function seems an anti pattern to me.
   
   Is there a case when we don't want increment?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520605920
 
 
   @hanm 
   
   >There are lots of changes to exception types thrown by functions, where it 
changes concrete exception types that's a function could actually throw to 
Exception (see some detailed examples in comments). What's the reason behind 
this change?
   
   Those changes are made limited on test* methods where otherwise a long line 
to be breakdown. A test* method should be always ok to be written thrown 
`Exception` because any verification is done in the method and no one should 
rely on a test* method. It itself is a top level method.
   
   
   I support your advice that
   
   > Pure formatting changes: white spaces, curly braces, indentations, etc.
   > None functional changes: remove unneeded imports, unused exceptions types, 
unneeded type parameters, etc.
   
   Let me see what I can do to rebase this pull request. Formerly I give every 
file an auto formatting phase, a glance phase and a fixing error phase. Maybe 
the middle phase is problematic. I should have do it by only a fixing error 
phase.
   
   By the way, following this suggestion we could suppress *NameCheck in this 
pass and give a dedicated pass to revisit it. That is, checkstyle rules 
restrict the pattern of method names, field names and variable names.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520605920
 
 
   @hanm 
   
   >There are lots of changes to exception types thrown by functions, where it 
changes concrete exception types that's a function could actually throw to 
Exception (see some detailed examples in comments). What's the reason behind 
this change?
   
   Those changes are made limited on test* methods where otherwise a long line 
to be breakdown. A test* method should be always ok to be written thrown 
`Exception` because any verification is done in the method and no one should 
rely on a test* method. It itself is a top level method.
   
   Apart from that, I support your advice that
   
   > Pure formatting changes: white spaces, curly braces, indentations, etc.
   > None functional changes: remove unneeded imports, unused exceptions types, 
unneeded type parameters, etc.
   
   Let me see what I can do to rebase this pull request. Formerly I give every 
file an auto formatting phase, a glance phase and a fixing error phase. Maybe 
the middle phase is problematic. I should have do it by only a fixing error 
phase.
   
   By the way, following this suggestion we could suppress *NameCheck in this 
pass and give a dedicated pass to revisit it. That is, checkstyle rules 
restrict the pattern of method names, field names and variable names.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520605920
 
 
   @hanm 
   
   >There are lots of changes to exception types thrown by functions, where it 
changes concrete exception types that's a function could actually throw to 
Exception (see some detailed examples in comments). What's the reason behind 
this change?
   
   Those changes are made limited on test* methods where otherwise a long line 
to be breakdown. A test* method should be always ok to be written thrown 
`Exception` because any verification is done in the method and no one should 
rely on a test* method. It itself is a top level method.
   
   Besides I support your advice that
   
   > Pure formatting changes: white spaces, curly braces, indentations, etc.
   > None functional changes: remove unneeded imports, unused exceptions types, 
unneeded type parameters, etc.
   
   Let me see what I can do to rebase this pull request. Formerly I give every 
file an auto formatting phase, a glance phase and a fixing error phase. Maybe 
the middle phase is problematic. I should have do it by only a fixing error 
phase.
   
   By the way, following this suggestion we could suppress *NameCheck in this 
pass and give a dedicated pass to revisit it. That is, checkstyle rules 
restrict the pattern of method names, field names and variable names.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313086056
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/Util.java
 ##
 @@ -122,50 +121,55 @@ public static File getLogDir(Properties props){
  * @param props properties container
  * @return value of the dbFormatConversion attribute
  */
-public static String getFormatConversionPolicy(Properties props){
+public static String getFormatConversionPolicy(Properties props) {
 return props.getProperty(DB_FORMAT_CONV);
 }
 
 /**
  * Extracts zxid from the file name. The file name should have been created
  * using one of the {@link #makeLogName(long)} or {@link 
#makeSnapshotName(long)}.
- * 
- * @param name the file name to parse
+ *
+ * @param name   the file name to parse
  * @param prefix the file name prefix (snapshot or log)
  * @return zxid
  */
 public static long getZxidFromName(String name, String prefix) {
 long zxid = -1;
-String nameParts[] = name.split("\\.");
+String[] nameParts = name.split("\\.");
 if (nameParts.length >= 2 && nameParts[0].equals(prefix)) {
 try {
 zxid = Long.parseLong(nameParts[1], 16);
-} catch (NumberFormatException e) {
+} catch (NumberFormatException ignore) {
+
 }
 }
 return zxid;
 }
 
 /**
  * Reads a transaction entry from the input archive.
+ *
  * @param ia archive to read from
  * @return null if the entry is corrupted or EOF has been reached; a buffer
  * (possible empty) containing serialized transaction record.
  * @throws IOException
  */
 public static byte[] readTxnBytes(InputArchive ia) throws IOException {
-try{
+try {
 byte[] bytes = ia.readBuffer("txtEntry");
 // Since we preallocate, we define EOF to be an
 // empty transaction
-if (bytes.length == 0)
+if (bytes.length == 0) {
 return bytes;
+}
 if (ia.readByte("EOF") != 'B') {
 LOG.error("Last transaction was partial.");
 return null;
 }
 return bytes;
-}catch(EOFException e){}
+} catch (EOFException ignore) {
+
 
 Review comment:
   should the pattern of adding a `// no-op` comment apply here?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313095998
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
 ##
 @@ -988,15 +985,15 @@ public void run() {
 /**
  * Halts this listener thread.
  */
-void halt(){
-try{
+void halt() {
+try {
 LOG.debug("Trying to close listener: {}", ss);
-if(ss != null) {
+if (ss != null) {
 LOG.debug("Closing listener: {}",
-  + QuorumCnxManager.this.mySid);
++QuorumCnxManager.this.mySid);
 
 Review comment:
   This `+` character shouldn't be there.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313092407
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java
 ##
 @@ -184,12 +182,15 @@ public synchronized boolean check(long time) {
 return (msDelay < learnerMaster.syncTimeout());
 }
 }
-};
+}
+
+;
 
 Review comment:
   This semi-colon is strange. I think it's redundant? 


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313097553
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java
 ##
 @@ -123,24 +132,25 @@
 
 /**
  * Minimum snapshot retain count.
+ *
  * @see org.apache.zookeeper.server.PurgeTxnLog#purge(File, File, int)
  */
-private final int MIN_SNAP_RETAIN_COUNT = 3;
+private final int minSnapRetainCount = 3;
 
 Review comment:
   would rather make `static` than change casing.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313094970
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
 ##
 @@ -202,78 +201,81 @@
 }
 }
 
-static public InitialMessage parse(Long protocolVersion, 
DataInputStream din)
-throws InitialMessageException, IOException {
+public static InitialMessage parse(
+Long protocolVersion,
+DataInputStream din
+) throws InitialMessageException, IOException {
 Long sid;
 
 if (protocolVersion != PROTOCOL_VERSION) {
-throw new InitialMessageException(
-"Got unrecognized protocol version %s", 
protocolVersion);
+throw new InitialMessageException("Got unrecognized protocol 
version %s", protocolVersion);
 }
 
 sid = din.readLong();
 
 int remaining = din.readInt();
 if (remaining <= 0 || remaining > maxBuffer) {
-throw new InitialMessageException(
-"Unreasonable buffer length: %s", remaining);
+throw new InitialMessageException("Unreasonable buffer length: 
%s", remaining);
 }
 
 byte[] b = new byte[remaining];
-int num_read = din.read(b);
+int numRead = din.read(b);
 
 Review comment:
   I disagree (slightly). The more standard naming style is easier to read and 
a one-time cost on open PRs (which will always exist) and on private patches 
doesn't seem too bad. It would be different if private repo owners intended to 
opt out of the change (by reverting the patch when merging) but I don't see why 
that would be preferable. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313084507
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/metric/AvgMinMaxCounter.java
 ##
 @@ -50,16 +49,17 @@ public void addDataPoint(long value) {
 
 private void setMax(long value) {
 long current;
-while (value > (current = max.get())
-&& !max.compareAndSet(current, value))
-;
+while (value > (current = max.get()) && !max.compareAndSet(current, 
value)) {
+// no op
 
 Review comment:
   nice :)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] eolivelli commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
eolivelli commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313081715
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
 ##
 @@ -202,78 +201,81 @@
 }
 }
 
-static public InitialMessage parse(Long protocolVersion, 
DataInputStream din)
-throws InitialMessageException, IOException {
+public static InitialMessage parse(
+Long protocolVersion,
+DataInputStream din
+) throws InitialMessageException, IOException {
 Long sid;
 
 if (protocolVersion != PROTOCOL_VERSION) {
-throw new InitialMessageException(
-"Got unrecognized protocol version %s", 
protocolVersion);
+throw new InitialMessageException("Got unrecognized protocol 
version %s", protocolVersion);
 }
 
 sid = din.readLong();
 
 int remaining = din.readInt();
 if (remaining <= 0 || remaining > maxBuffer) {
-throw new InitialMessageException(
-"Unreasonable buffer length: %s", remaining);
+throw new InitialMessageException("Unreasonable buffer length: 
%s", remaining);
 }
 
 byte[] b = new byte[remaining];
-int num_read = din.read(b);
+int numRead = din.read(b);
 
 Review comment:
   I totally agree!
   I have talked about this in the ML, we should not change variable names and 
method signatures.
   It is dangerous, especially for pending patches waiting for a merge


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313081572
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMXBean.java
 ##
 @@ -25,192 +25,233 @@
 /**
  * @return the server socket port number
  */
-public String getClientPort();
+String getClientPort();
+
 /**
  * @return the zookeeper server version
  */
-public String getVersion();
+String getVersion();
+
 /**
  * @return time the server was started
  */
-public String getStartTime();
+String getStartTime();
+
 /**
  * @return min request latency in ms
  */
-public long getMinRequestLatency();
+long getMinRequestLatency();
+
 /**
  * @return average request latency in ms
  */
-public double getAvgRequestLatency();
+double getAvgRequestLatency();
+
 /**
  * @return max request latency in ms
  */
-public long getMaxRequestLatency();
+long getMaxRequestLatency();
+
 /**
  * @return number of packets received so far
  */
-public long getPacketsReceived();
+long getPacketsReceived();
+
 /**
  * @return number of packets sent so far
  */
-public long getPacketsSent();
+long getPacketsSent();
+
 /**
  * @return number of fsync threshold exceeds so far
  */
-public long getFsyncThresholdExceedCount();
+long getFsyncThresholdExceedCount();
+
 /**
  * @return number of outstanding requests.
  */
-public long getOutstandingRequests();
+long getOutstandingRequests();
+
 /**
- * Current TickTime of server in milliseconds
+ * Current TickTime of server in milliseconds.
  */
-public int getTickTime();
+int getTickTime();
+
 /**
- * Set TickTime of server in milliseconds
+ * Set TickTime of server in milliseconds.
  */
-public void setTickTime(int tickTime);
+void setTickTime(int tickTime);
 
-/** Current maxClientCnxns allowed from a particular host */
-public int getMaxClientCnxnsPerHost();
+/**
+ * Current maxClientCnxns allowed from a particular host.
+ */
+int getMaxClientCnxnsPerHost();
 
-/** Set maxClientCnxns allowed from a particular host */
-public void setMaxClientCnxnsPerHost(int max);
+/**
+ * Set maxClientCnxns allowed from a particular host.
+ */
+void setMaxClientCnxnsPerHost(int max);
 
 /**
- * Current minSessionTimeout of the server in milliseconds
+ * Current minSessionTimeout of the server in milliseconds.
  */
-public int getMinSessionTimeout();
+int getMinSessionTimeout();
+
 /**
- * Set minSessionTimeout of server in milliseconds
+ * Set minSessionTimeout of server in milliseconds.
  */
-public void setMinSessionTimeout(int min);
+void setMinSessionTimeout(int min);
 
 /**
- * Current maxSessionTimeout of the server in milliseconds
+ * Current maxSessionTimeout of the server in milliseconds.
  */
-public int getMaxSessionTimeout();
+int getMaxSessionTimeout();
+
 /**
- * Set maxSessionTimeout of server in milliseconds
+ * Set maxSessionTimeout of server in milliseconds.
  */
-public void setMaxSessionTimeout(int max);
+void setMaxSessionTimeout(int max);
+
+boolean getResponseCachingEnabled();
 
-public boolean getResponseCachingEnabled();
-public void setResponseCachingEnabled(boolean isEnabled);
+void setResponseCachingEnabled(boolean isEnabled);
 
 /* Connection throttling settings */
-public int getConnectionMaxTokens();
-public void setConnectionMaxTokens(int val);
 
 Review comment:
   @nkalmar - looks like you're right that it is just getters/setters together. 
My mind was probably on the groupings of ServerMetrics.
   
   I'm okay with keeping your change as it is, @TisonKun.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313080823
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/NIOServerCnxnFactory.java
 ##
 @@ -962,14 +979,14 @@ public void dumpConnections(PrintWriter pwriter) {
 @Override
 public void resetAllConnectionStats() {
 // No need to synchronize since cnxns is backed by a ConcurrentHashMap
-for(ServerCnxn c : cnxns){
+for (ServerCnxn c : cnxns) {
 c.resetStats();
 }
 }
 
 @Override
 public Iterable> getAllConnectionInfo(boolean brief) {
-HashSet> info = new HashSet>();
+HashSet> info = new HashSet>();
 
 Review comment:
   That's a fair preference. I'll stop pointing out these kind of nits in 
reviewing this diff and stick to validating correctness.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
enixon commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313079906
 
 

 ##
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
 ##
 @@ -928,12 +925,12 @@ else if (serverPath.length() > chrootPath.length())
 packet.replyHeader.setErr(
 KeeperException.Code.CONNECTIONLOSS.intValue());
 throw new IOException("Xid out of order. Got Xid "
-+ replyHdr.getXid() + " with err " +
-+ replyHdr.getErr() +
-" expected Xid "
++ replyHdr.getXid() + " with err "
++ +replyHdr.getErr()
 
 Review comment:
   In this specific case, there are two uses of the `+` operator on the line. 
It's exactly what the old code has so your change preserves the original 
mistake - it's just the kind of thing that I'm surprised compiles.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
hanm commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration 
on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520538013
 
 
   I have my pass over this, and my feedback is it might be a good idea if we 
limit the type of changes to be:
   * Pure formatting changes: white spaces, curly braces, indentations, etc.
   * None functional changes: remove unneeded imports, unused exceptions types, 
unneeded type parameters, etc.
   
   This will provide a simpler conceptual mode to review and merge this patch 
with high confidence that the impact will be limited. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313049905
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumCnxManager.java
 ##
 @@ -202,78 +201,81 @@
 }
 }
 
-static public InitialMessage parse(Long protocolVersion, 
DataInputStream din)
-throws InitialMessageException, IOException {
+public static InitialMessage parse(
+Long protocolVersion,
+DataInputStream din
+) throws InitialMessageException, IOException {
 Long sid;
 
 if (protocolVersion != PROTOCOL_VERSION) {
-throw new InitialMessageException(
-"Got unrecognized protocol version %s", 
protocolVersion);
+throw new InitialMessageException("Got unrecognized protocol 
version %s", protocolVersion);
 }
 
 sid = din.readLong();
 
 int remaining = din.readInt();
 if (remaining <= 0 || remaining > maxBuffer) {
-throw new InitialMessageException(
-"Unreasonable buffer length: %s", remaining);
+throw new InitialMessageException("Unreasonable buffer length: 
%s", remaining);
 }
 
 byte[] b = new byte[remaining];
-int num_read = din.read(b);
+int numRead = din.read(b);
 
 Review comment:
   Can we avoid change names of variable for now? This change is not purely 
style change and might make those who maintains private patches harder. Such 
change can be done separately with a more fine grained approach.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] eolivelli commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
eolivelli commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313046246
 
 

 ##
 File path: 
zookeeper-server/src/test/java/org/apache/zookeeper/test/TestHammer.java
 ##
 @@ -18,41 +18,43 @@
 
 package org.apache.zookeeper.test;
 
-
-import org.apache.zookeeper.CreateMode;
-import org.apache.zookeeper.ZooKeeper;
 import org.apache.zookeeper.AsyncCallback.VoidCallback;
+import org.apache.zookeeper.CreateMode;
 import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooKeeper;
 import org.apache.zookeeper.common.Time;
 
 public class TestHammer implements VoidCallback {
 
-static int REPS = 5;
+private static int reps = 5;
 
 Review comment:
   Yes it does some static analysis but I don't think it requests this change.
   
   @tisunkun which is the rule that fires for this line?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313043052
 
 

 ##
 File path: 
zookeeper-server/src/test/java/org/apache/zookeeper/test/TestHammer.java
 ##
 @@ -18,41 +18,43 @@
 
 package org.apache.zookeeper.test;
 
-
-import org.apache.zookeeper.CreateMode;
-import org.apache.zookeeper.ZooKeeper;
 import org.apache.zookeeper.AsyncCallback.VoidCallback;
+import org.apache.zookeeper.CreateMode;
 import org.apache.zookeeper.ZooDefs.Ids;
+import org.apache.zookeeper.ZooKeeper;
 import org.apache.zookeeper.common.Time;
 
 public class TestHammer implements VoidCallback {
 
-static int REPS = 5;
+private static int reps = 5;
 
 Review comment:
   This is not just a style and formatting change as access level is changed. 
It seems OK, but I'd like to understand how check style works - does it do any 
reachability (possibly other types of static) analysis and make decision based 
on that?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313034715
 
 

 ##
 File path: 
zookeeper-server/src/test/java/org/apache/zookeeper/util/PemReaderTest.java
 ##
 @@ -65,7 +65,7 @@ public PemReaderTest(
 }
 
 @Test
-public void testLoadPrivateKeyFromKeyStore() throws IOException, 
GeneralSecurityException {
+public void testLoadPrivateKeyFromKeyStore() throws Exception {
 
 Review comment:
   Why change the exception type here?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313037852
 
 

 ##
 File path: 
zookeeper-server/src/test/java/org/apache/zookeeper/test/WatcherTest.java
 ##
 @@ -142,18 +137,16 @@ public void processResult(int rc, String path, Object 
ctx) {
 }
 }
 }
-
+
 @Test
-public void testWatcherDisconnectOnClose() 
-throws IOException, InterruptedException, KeeperException 
-{
+public void testWatcherDisconnectOnClose() throws Exception {
 
 Review comment:
   Unnecessary exception type change?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
hanm commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r313032932
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/NIOServerCnxnFactory.java
 ##
 @@ -962,14 +979,14 @@ public void dumpConnections(PrintWriter pwriter) {
 @Override
 public void resetAllConnectionStats() {
 // No need to synchronize since cnxns is backed by a ConcurrentHashMap
-for(ServerCnxn c : cnxns){
+for (ServerCnxn c : cnxns) {
 c.resetStats();
 }
 }
 
 @Override
 public Iterable> getAllConnectionInfo(boolean brief) {
-HashSet> info = new HashSet>();
+HashSet> info = new HashSet>();
 
 Review comment:
   I'd also prefer limiting the scope of this change to be mechanical changes 
that's only appertain to style and formatting.
   
   @TisonKun since you are on this, do you mind to create a couple of JIRAs to 
follow up the issues around log formatting, interface declaration and other 
nits that @enixon pointed out? 


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] ztzg opened a new pull request #1054: ZOOKEEPER-1112: Add support for C client for SASL authentication

2019-08-12 Thread GitBox
ztzg opened a new pull request #1054: ZOOKEEPER-1112: Add support for C client 
for SASL authentication
URL: https://github.com/apache/zookeeper/pull/1054
 
 
   This is a forward-port of Tom Klonikowski's (@kloni) 
[ZOOKEEPER-1112](https://issues.apache.org/jira/browse/ZOOKEEPER-1112) patches 
on top of the current `master` branch.
   
   It stays close to the original patches, and only adds two commits on top:
   
1. A simple way to pass passwords to the console client via files;
2. A fix for an use-after-free in the C client library.
   
   Open questions:
   
1. The patches add an optional dependency on the Cyrus SASL library.
   [Eric Yang 
wrote](https://issues.apache.org/jira/browse/ZOOKEEPER-1112?focusedCommentId=13080468=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13080468):
   
   > Apache Web Server has a module for authenticate sasl using Cyrus SASL 
library. I think the license should be compatible with written agreement from 
CMU.
   
   Who should take care of this?  Is there an established procedure?
   
2. [Tom Klonikowski 
wrote](https://issues.apache.org/jira/browse/ZOOKEEPER-1112?focusedCommentId=13126379=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13126379):
   
   > Maybe part #2 (sasl auth implementation via cyrus sasl) and #3 (cli 
using #2) should be moved to the recipes section (recipes/sasl). What do you 
think?
   
   What do you think?
   
   The existing tests as well as the added SASL tests pass in `st` and `mt` 
modes.  Both `cli_sasl_*` utilities have been tested against a `DIGEST-MD5` 
server; I intend to test them against a GSSAPI one in the near future.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312900302
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/client/HostProvider.java
 ##
 @@ -18,58 +18,60 @@
 
 package org.apache.zookeeper.client;
 
-import org.apache.yetus.audience.InterfaceAudience;
-
 import java.net.InetSocketAddress;
 import java.util.Collection;
+import org.apache.yetus.audience.InterfaceAudience;
 
 /**
  * A set of hosts a ZooKeeper client should connect to.
- * 
- * Classes implementing this interface must guarantee the following:
- * 
- * * Every call to next() returns an InetSocketAddress. So the iterator never
- * ends.
- * 
- * * The size() of a HostProvider may never be zero.
- * 
- * A HostProvider must return resolved InetSocketAddress instances on next() 
if the next address is resolvable.
+ *
+ * Classes implementing this interface must guarantee the following:
+ *
+ * 
+ * Every call to next() returns an InetSocketAddress. So the iterator 
never ends.
+ * The size() of a HostProvider may never be zero.
+ * 
+ *
+ * A HostProvider must return resolved InetSocketAddress instances on 
next() if the next address is resolvable.
  * In that case, it's up to the HostProvider, whether it returns the next 
resolvable address in the list or return
  * the next one as UnResolved.
- * 
- * Different HostProvider could be imagined:
- * 
- * * A HostProvider that loads the list of Hosts from an URL or from DNS 
- * * A HostProvider that re-resolves the InetSocketAddress after a timeout. 
- * * A HostProvider that prefers nearby hosts.
+ *
+ * ifferent HostProvider could be imagined:
 
 Review comment:
   Fixed.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] symat edited a comment on issue #1048: ZOOKEEPER-3188: Improve resilience to network

2019-08-12 Thread GitBox
symat edited a comment on issue #1048: ZOOKEEPER-3188: Improve resilience to 
network
URL: https://github.com/apache/zookeeper/pull/1048#issuecomment-520381529
 
 
   I created a simple docker config with multiple virtual networks and managed 
to test the situation when some of the containers loose the access to one of 
the virtual networks. I uploaded the docker related scripts / configs here: 
https://github.com/symat/zookeeper-docker-test
   
   During these manual tests I found some situations when the previous patch 
didn't work. 
   - the InitialMessage sent during the leader election contained only a single 
election address. If this address was not reachable by the recipient of the 
InitialMessage, then the connection was never successfully initiated. I changed 
the format of the InitialMessage to send all the election addresses and the 
other side will use only the one which is reachable.
   - When an existing tcp connection to an electionAddress is broken, the 
server will try to send notification messages re-using the existing SendWorker 
threads. I would assume that the SendWorker.send() method should die when it 
tries to flush the output stream on the socket which destination is already 
unreachable. However, for some reason it doesn't die. (this could be 
investigated further) Anyway, I added a small logic for the connection 
initiation to verify if the existing destination in SendWorker is still 
reachable. If the destination is unreachable in the SendWorker thread, then we 
can gracefully finish it and during the next connection attempt we will choose 
a destination what is reachable. (this part I fixed in a second commit)
   
   With these modifications I was able to test the following situation 
successfully:
   
   1. starting a zookeeper with nodes, each server listening on two addresses 
(on two separate virtual networks)
   2. waiting the initial leader election to happen
   3. removing the current leader from the virtual network that is used by the 
others as destination
   4. it took a few seconds until all the servers recognised the loss of 
connections, and in 5-10 seconds the connections were re-established and the 
new leader election finished
   
   I will think how to unittest these features. (or should we crate some 
docker-based automated integration test?)
   
   In the mean while I would appreciate a deep review of these changes, as I am 
quite new in the Zookeeper code...


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] symat edited a comment on issue #1048: ZOOKEEPER-3188: Improve resilience to network

2019-08-12 Thread GitBox
symat edited a comment on issue #1048: ZOOKEEPER-3188: Improve resilience to 
network
URL: https://github.com/apache/zookeeper/pull/1048#issuecomment-520381529
 
 
   I created a simple docker config with multiple virtual networks and managed 
to test the situation when some of the containers loose the access to one of 
the virtual networks. I uploaded the docker related scripts / configs here: 
https://github.com/symat/zookeeper-docker-test
   
   During these manual tests I found some situations when the previous patch 
didn't work. 
   - the InitialMessage sent during the leader election contained only a single 
election address. If this address was not reachable by the recipient of the 
InitialMessage, then the connection was never successfully initiated. I changed 
the format of the InitialMessage to send all the election addresses and the 
other side will use only the one which is reachable.
   - When an existing tcp connection to an electionAddress is broken, the 
server will try to send notification messages re-using the existing SendWorker 
threads. I would assume that the SendWorker.send() method should die when it 
tries to flush the output stream on the socket which destination is already 
unreachable. However, for some reason it doesn't die. (this could be 
investigated further) Anyway, I added a small logic for the connection 
initiation to verify if the existing destination in SendWorker is still 
reachable. If the destination is unreachable in the SendWorker thread, then we 
can gracefully finish it and during the next connection attempt we will choose 
a destination what is reachable.
   
   With these modifications I was able to test the following situation 
successfully:
   
   1. starting a zookeeper with nodes, each server listening on two addresses 
(on two separate virtual networks)
   2. waiting the initial leader election to happen
   3. removing the current leader from the virtual network that is used by the 
others as destination
   4. it took a few seconds until all the servers recognised the loss of 
connections, and in 5-10 seconds the connections were re-established and the 
new leader election finished
   
   I will think how to unittest these features. (or should we crate some 
docker-based automated integration test?)
   
   In the mean while I would appreciate a deep review of these changes, as I am 
quite new in the Zookeeper code...


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] symat commented on issue #1048: ZOOKEEPER-3188: Improve resilience to network

2019-08-12 Thread GitBox
symat commented on issue #1048: ZOOKEEPER-3188: Improve resilience to network
URL: https://github.com/apache/zookeeper/pull/1048#issuecomment-520381529
 
 
   I created a simple docker config with multiple virtual networks and managed 
to test the situation when some of the containers loose the access to one of 
the virtual networks. I uploaded the docker related scripts / configs here: 
https://github.com/symat/zookeeper-docker-test
   
   During these manual tests I found some situations when the previous patch 
didn't work. 
   - the InitialMessage sent during the leader election contained only a single 
election address. If this address was not reachable by the recipient of the 
InitialMessage, then the connection was never successfully initiated. I changed 
the format of the InitialMessage to send all the election addresses and the 
other side will use only the one which is reachable.
   - When an existing tcp connection to an electionAddress is broken, the 
server will try to send notification messages re-using the existing SendWorker 
threads. I would assume that the SendWorker.send() method should die when it 
tries to flush the output stream on the socket which destination is already 
unreachable. However, for some reason it doesn't die. (this could be 
investigated further) Anyway, I added a small logic for the connection 
initiation to verify if the existing destination in SendWorker is still 
reachable. If the destination is unreachable in the SendWorker thread, then we 
can gracefully finish it and during the next connection attempt we will choose 
a destination what is reachable.
   
   With these modifications I was able to test the following situation 
successfully:
   
   1. starting a zookeeper with nodes, each server listening on two addresses 
(on two separate virtual networks)
   2. waiting the initial leader election to happen
   3. removing the current leader from the virtual network that is used by the 
others as destination
   4. it took a few seconds until all the servers recognised the loss of 
connections, and in 5-15 seconds the connections were re-established and the 
new leader election finished
   
   I will think how to unittest these features. (or should we crate some 
docker-based automated integration test?)
   
   In the mean while I would appreciate a deep review of these changes, as I am 
quite new in the Zookeeper code...


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312868828
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
 ##
 @@ -212,341 +209,343 @@ public void processRequest(Request request) {
 throw ke;
 }
 
-LOG.debug("{}",request);
+LOG.debug("{}", request);
 
 if (request.isStale()) {
 ServerMetrics.getMetrics().STALE_REPLIES.add(1);
 }
 
 switch (request.type) {
-case OpCode.ping: {
-lastOp = "PING";
-updateStats(request, lastOp, lastZxid);
+case OpCode.ping: {
 
 Review comment:
   It is done by an auto formatting phase. For introduce such a phase many of 
'+'/'{' should be around by space could be fixed. It is a format related change 
and sure, no logical changes.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312868387
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/EphemeralType.java
 ##
 @@ -188,24 +179,27 @@ public static EphemeralType get(long ephemeralOwner) {
 }
 
 /**
- * Make sure the given server ID is compatible with the current extended 
ephemeral setting
+ * Make sure the given server ID is compatible with the current extended 
ephemeral setting.
  *
  * @param serverId Server ID
  * @throws RuntimeException extendedTypesEnabled is true but Server ID is 
too large
  */
 public static void validateServerId(long serverId) {
-// TODO: in the future, serverId should be validated for all cases, 
not just the extendedEphemeralTypesEnabled case
+// TODO: in the future, serverId should be validated for all cases,
 
 Review comment:
   Support. This is also how akka community tracked TODOs and it was a good 
practice yet.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312868023
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/EphemeralType.java
 ##
 @@ -89,8 +78,9 @@ public long toEphemeralOwner(long ttl) {
 if ((ttl > TTL.maxValue()) || (ttl <= 0)) {
 throw new IllegalArgumentException("ttl must be positive and 
cannot be larger than: " + TTL.maxValue());
 }
-//noinspection PointlessBitwiseExpression
 
 Review comment:
   Invalid any more.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312866880
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/client/HostProvider.java
 ##
 @@ -18,58 +18,60 @@
 
 package org.apache.zookeeper.client;
 
-import org.apache.yetus.audience.InterfaceAudience;
-
 import java.net.InetSocketAddress;
 import java.util.Collection;
+import org.apache.yetus.audience.InterfaceAudience;
 
 /**
  * A set of hosts a ZooKeeper client should connect to.
- * 
- * Classes implementing this interface must guarantee the following:
- * 
- * * Every call to next() returns an InetSocketAddress. So the iterator never
- * ends.
- * 
- * * The size() of a HostProvider may never be zero.
- * 
- * A HostProvider must return resolved InetSocketAddress instances on next() 
if the next address is resolvable.
+ *
+ * Classes implementing this interface must guarantee the following:
+ *
+ * 
+ * Every call to next() returns an InetSocketAddress. So the iterator 
never ends.
+ * The size() of a HostProvider may never be zero.
+ * 
+ *
+ * A HostProvider must return resolved InetSocketAddress instances on 
next() if the next address is resolvable.
  * In that case, it's up to the HostProvider, whether it returns the next 
resolvable address in the list or return
  * the next one as UnResolved.
- * 
- * Different HostProvider could be imagined:
- * 
- * * A HostProvider that loads the list of Hosts from an URL or from DNS 
- * * A HostProvider that re-resolves the InetSocketAddress after a timeout. 
- * * A HostProvider that prefers nearby hosts.
+ *
+ * ifferent HostProvider could be imagined:
 
 Review comment:
   Nice catch. Fixing...


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520376969
 
 
   @enixon 
   
   > Do we know whether the change in visibility to some of the methods will 
effect curator or other third parties?
   
   There is no refactoring and nothing public changed. Only reformatting and 
some dead code deletion included. It is expected be unaware of any other third 
parties.
   
   > A question for others: with regards to the TODOs in the code, do we have 
any expectation that they are backed by JIRA tickets and, if so, how can them 
be linked to the ticket number?
   
   Nice question. We could have another pass(whether or not coupled within 
this) to investigate all TODOs and linked to the ticket and when there is no 
one, create one or delete TODO comment.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520376969
 
 
   > Do we know whether the change in visibility to some of the methods will 
effect curator or other third parties?
   
   There is no refactoring and nothing public changed. Only reformatting and 
some dead code deletion included. It is expected be unaware of any other third 
parties.
   
   > A question for others: with regards to the TODOs in the code, do we have 
any expectation that they are backed by JIRA tickets and, if so, how can them 
be linked to the ticket number?
   
   Nice question. We could have another pass(whether or not coupled within 
this) to investigate all TODOs and linked to the ticket and when there is no 
one, create one or delete TODO comment.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312865164
 
 

 ##
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java
 ##
 @@ -184,25 +191,31 @@ public static String validateFileInput(String filePath) {
 /**
  * Visits the subtree with root as given path and calls the passed 
callback with each znode
  * found during the search. It performs a depth-first, pre-order traversal 
of the tree.
- * 
- * Important: This is not an atomic snapshot of the tree 
ever, but the
+ *
+ * Important: This is not an atomic snapshot of the tree 
ever, but the
  * state as it exists across multiple RPCs from zkClient to the ensemble.
  * For practical purposes, it is suggested to bring the clients to the 
ensemble
  * down (i.e. prevent writes to pathRoot) to 'simulate' a snapshot 
behavior.
  */
-public static void visitSubTreeDFS(ZooKeeper zk, final String path, 
boolean watch,
-StringCallback cb) throws KeeperException, InterruptedException {
+public static void visitSubTreeDFS(
+ZooKeeper zk,
+final String path,
+boolean watch,
+StringCallback cb
+) throws KeeperException, InterruptedException {
 PathUtils.validatePath(path);
 
 zk.getData(path, watch, null);
 cb.processResult(Code.OK.intValue(), path, null, path);
 visitSubTreeDFSHelper(zk, path, watch, cb);
 }
 
-@SuppressWarnings("unchecked")
 
 Review comment:
   Yes. This is validated by the build cycle as we enable `-Werror`.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312864700
 
 

 ##
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
 ##
 @@ -928,12 +925,12 @@ else if (serverPath.length() > chrootPath.length())
 packet.replyHeader.setErr(
 KeeperException.Code.CONNECTIONLOSS.intValue());
 throw new IOException("Xid out of order. Got Xid "
-+ replyHdr.getXid() + " with err " +
-+ replyHdr.getErr() +
-" expected Xid "
++ replyHdr.getXid() + " with err "
++ +replyHdr.getErr()
 
 Review comment:
   Here is a rule `+' should be on a new line`. For cleanup I guess you mean 
replace it with `String.format`? It would be a follow up to unify the 
log/Exception format and generation like that for javadoc(ZOOKEEPER-3469). This 
pull request would be a clean start point for all of this request but we should 
prevent it from too complex IMHO.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312863142
 
 

 ##
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
 ##
 @@ -160,7 +157,7 @@
 
 private long sessionId;
 
-private byte sessionPasswd[] = new byte[16];
+private byte[] sessionPasswd;
 
 Review comment:
   Right. Always be initialized in constructor.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] nkalmar commented on issue #610: [ZOOKEEPER-3124] Add the correct comment to show why we need the special logic to handle cversion and pzxid

2019-08-12 Thread GitBox
nkalmar commented on issue #610: [ZOOKEEPER-3124] Add the correct comment to 
show why we need the special logic to handle cversion and pzxid
URL: https://github.com/apache/zookeeper/pull/610#issuecomment-520373434
 
 
   Sure, I can commit, just please rebase @lvfangmin , unfortunately it became 
conflicted.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520330217
 
 
   @nkalmar Thanks for your review and Thanks for @enixon 's comments. I would 
try to give a look and address them hours later.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312809875
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMXBean.java
 ##
 @@ -25,192 +25,233 @@
 /**
  * @return the server socket port number
  */
-public String getClientPort();
+String getClientPort();
+
 /**
  * @return the zookeeper server version
  */
-public String getVersion();
+String getVersion();
+
 /**
  * @return time the server was started
  */
-public String getStartTime();
+String getStartTime();
+
 /**
  * @return min request latency in ms
  */
-public long getMinRequestLatency();
+long getMinRequestLatency();
+
 /**
  * @return average request latency in ms
  */
-public double getAvgRequestLatency();
+double getAvgRequestLatency();
+
 /**
  * @return max request latency in ms
  */
-public long getMaxRequestLatency();
+long getMaxRequestLatency();
+
 /**
  * @return number of packets received so far
  */
-public long getPacketsReceived();
+long getPacketsReceived();
+
 /**
  * @return number of packets sent so far
  */
-public long getPacketsSent();
+long getPacketsSent();
+
 /**
  * @return number of fsync threshold exceeds so far
  */
-public long getFsyncThresholdExceedCount();
+long getFsyncThresholdExceedCount();
+
 /**
  * @return number of outstanding requests.
  */
-public long getOutstandingRequests();
+long getOutstandingRequests();
+
 /**
- * Current TickTime of server in milliseconds
+ * Current TickTime of server in milliseconds.
  */
-public int getTickTime();
+int getTickTime();
+
 /**
- * Set TickTime of server in milliseconds
+ * Set TickTime of server in milliseconds.
  */
-public void setTickTime(int tickTime);
+void setTickTime(int tickTime);
 
-/** Current maxClientCnxns allowed from a particular host */
-public int getMaxClientCnxnsPerHost();
+/**
+ * Current maxClientCnxns allowed from a particular host.
+ */
+int getMaxClientCnxnsPerHost();
 
-/** Set maxClientCnxns allowed from a particular host */
-public void setMaxClientCnxnsPerHost(int max);
+/**
+ * Set maxClientCnxns allowed from a particular host.
+ */
+void setMaxClientCnxnsPerHost(int max);
 
 /**
- * Current minSessionTimeout of the server in milliseconds
+ * Current minSessionTimeout of the server in milliseconds.
  */
-public int getMinSessionTimeout();
+int getMinSessionTimeout();
+
 /**
- * Set minSessionTimeout of server in milliseconds
+ * Set minSessionTimeout of server in milliseconds.
  */
-public void setMinSessionTimeout(int min);
+void setMinSessionTimeout(int min);
 
 /**
- * Current maxSessionTimeout of the server in milliseconds
+ * Current maxSessionTimeout of the server in milliseconds.
  */
-public int getMaxSessionTimeout();
+int getMaxSessionTimeout();
+
 /**
- * Set maxSessionTimeout of server in milliseconds
+ * Set maxSessionTimeout of server in milliseconds.
  */
-public void setMaxSessionTimeout(int max);
+void setMaxSessionTimeout(int max);
+
+boolean getResponseCachingEnabled();
 
-public boolean getResponseCachingEnabled();
-public void setResponseCachingEnabled(boolean isEnabled);
+void setResponseCachingEnabled(boolean isEnabled);
 
 /* Connection throttling settings */
-public int getConnectionMaxTokens();
-public void setConnectionMaxTokens(int val);
 
 Review comment:
   We don't have a checkstyle rules on this case and so there is no so called 
standardized format in fact. It is done by an auto formatting phase. I can 
understand the pros grouping getters and setters. Fair enough to revert this. 
Thanks for your nice catch :P


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] nkalmar commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
nkalmar commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle 
configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#issuecomment-520326781
 
 
   Thanks @TisonKun , I didn't get the chance to review the whole patch. But 
@enixon left great comments, I commented on one where I think the change is 
fine.
   I'm also going to take a thorough look hopefully today.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [zookeeper] nkalmar commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server

2019-08-12 Thread GitBox
nkalmar commented on a change in pull request #1049: ZOOKEEPER-3475 Enable 
Checkstyle configuration on zookeeper-server
URL: https://github.com/apache/zookeeper/pull/1049#discussion_r312805241
 
 

 ##
 File path: 
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMXBean.java
 ##
 @@ -25,192 +25,233 @@
 /**
  * @return the server socket port number
  */
-public String getClientPort();
+String getClientPort();
+
 /**
  * @return the zookeeper server version
  */
-public String getVersion();
+String getVersion();
+
 /**
  * @return time the server was started
  */
-public String getStartTime();
+String getStartTime();
+
 /**
  * @return min request latency in ms
  */
-public long getMinRequestLatency();
+long getMinRequestLatency();
+
 /**
  * @return average request latency in ms
  */
-public double getAvgRequestLatency();
+double getAvgRequestLatency();
+
 /**
  * @return max request latency in ms
  */
-public long getMaxRequestLatency();
+long getMaxRequestLatency();
+
 /**
  * @return number of packets received so far
  */
-public long getPacketsReceived();
+long getPacketsReceived();
+
 /**
  * @return number of packets sent so far
  */
-public long getPacketsSent();
+long getPacketsSent();
+
 /**
  * @return number of fsync threshold exceeds so far
  */
-public long getFsyncThresholdExceedCount();
+long getFsyncThresholdExceedCount();
+
 /**
  * @return number of outstanding requests.
  */
-public long getOutstandingRequests();
+long getOutstandingRequests();
+
 /**
- * Current TickTime of server in milliseconds
+ * Current TickTime of server in milliseconds.
  */
-public int getTickTime();
+int getTickTime();
+
 /**
- * Set TickTime of server in milliseconds
+ * Set TickTime of server in milliseconds.
  */
-public void setTickTime(int tickTime);
+void setTickTime(int tickTime);
 
-/** Current maxClientCnxns allowed from a particular host */
-public int getMaxClientCnxnsPerHost();
+/**
+ * Current maxClientCnxns allowed from a particular host.
+ */
+int getMaxClientCnxnsPerHost();
 
-/** Set maxClientCnxns allowed from a particular host */
-public void setMaxClientCnxnsPerHost(int max);
+/**
+ * Set maxClientCnxns allowed from a particular host.
+ */
+void setMaxClientCnxnsPerHost(int max);
 
 /**
- * Current minSessionTimeout of the server in milliseconds
+ * Current minSessionTimeout of the server in milliseconds.
  */
-public int getMinSessionTimeout();
+int getMinSessionTimeout();
+
 /**
- * Set minSessionTimeout of server in milliseconds
+ * Set minSessionTimeout of server in milliseconds.
  */
-public void setMinSessionTimeout(int min);
+void setMinSessionTimeout(int min);
 
 /**
- * Current maxSessionTimeout of the server in milliseconds
+ * Current maxSessionTimeout of the server in milliseconds.
  */
-public int getMaxSessionTimeout();
+int getMaxSessionTimeout();
+
 /**
- * Set maxSessionTimeout of server in milliseconds
+ * Set maxSessionTimeout of server in milliseconds.
  */
-public void setMaxSessionTimeout(int max);
+void setMaxSessionTimeout(int max);
+
+boolean getResponseCachingEnabled();
 
-public boolean getResponseCachingEnabled();
-public void setResponseCachingEnabled(boolean isEnabled);
+void setResponseCachingEnabled(boolean isEnabled);
 
 /* Connection throttling settings */
-public int getConnectionMaxTokens();
-public void setConnectionMaxTokens(int val);
 
 Review comment:
   It's pretty much just the getters and setters grouped together I think.
   I'm fine with the standardized format, but always open for suggestions, so 
this is a +0 from me :)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services