[GitHub] [zookeeper] TisonKun edited a comment on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
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
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
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
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 a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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] TisonKun commented on a change in pull request #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
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
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
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
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
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
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
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 commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
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
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
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
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] TisonKun commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
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] nkalmar commented on issue #1049: ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server
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
