Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server #1049

Closed
wants to merge 14 commits into from

Conversation

@TisonKun
Copy link
Contributor

commented Aug 10, 2019

  • org/apache/zookeeper
  • org/apache/zookeeper/admin
  • org/apache/zookeeper/cli
  • org/apache/zookeeper/client
  • org/apache/zookeeper/common
  • org/apache/zookeeper/jmx
  • org/apache/zookeeper/metrics
  • org/apache/zookeeper/metrics/impl
  • org/apache/zookeeper/server
  • org/apache/zookeeper/server/admin
  • org/apache/zookeeper/server/auth
  • org/apache/zookeeper/server/command
  • org/apache/zookeeper/server/metric
  • org/apache/zookeeper/server/persistence
  • org/apache/zookeeper/server/quorum
  • org/apache/zookeeper/server/quorum/auth
  • org/apache/zookeeper/server/quorum/flexible
  • org/apache/zookeeper/server/util
  • org/apache/zookeeper/server/watch
  • org/apache/zookeeper/test
  • org/apache/zookeeper/util
  • org/apache/zookeeper/version/util
@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Preview and for concurrent review. Hopefully remove "WIP" in the next day.

@maoling

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

awesome!!!

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Jenkins fails on QuorumPeerMainTest.testLeaderOutOfView. In the last two commits here I confirm no refactoring. Is it an unstable test?

@eolivelli

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

It is a flaky test, no problem

@eolivelli

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

@lvfangmin @enixon @hanm please follow this story.
We should move quickly in order not to break much stuff

@enixon

This comment has been minimized.

Copy link

commented Aug 10, 2019

Change looks good to me

@TisonKun TisonKun changed the title [WIP] ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server ZOOKEEPER-3475 Enable Checkstyle configuration on zookeeper-server Aug 11, 2019

@TisonKun TisonKun force-pushed the TisonKun:ZOOKEEPER-3475 branch 2 times, most recently from 6bf0c7b to 61d5e20 Aug 11, 2019

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Fails on

[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1559:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1634:8: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1644:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1825:85: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:1882:80: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2086:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2194:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2420:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2484:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2548:8: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2555:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2629:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:2755:14: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:3009:8: Unable to get class information for @throws tag 'KeeperException.NoWatcherException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:3023:36: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:3057:8: Unable to get class information for @throws tag 'KeeperException.NoWatcherException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:3066:36: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java:54:36: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
[ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java:128:36: Unable to get class information for @throws tag 'KeeperException'. [JavadocMethod]
Audit done.
[INFO] There are 20 errors reported by Checkstyle 8.17 with checkstyle-strict.xml ruleset.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1550,8] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1559,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1634,8] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1644,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1825,85] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[1882,80] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[2086,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[2194,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[2420,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[2484,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[2548,8] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException.InvalidACLException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[2555,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[2629,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[2755,14] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[3009,8] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException.NoWatcherException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[3023,36] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[3057,8] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException.NoWatcherException'.
[ERROR] src/main/java/org/apache/zookeeper/ZooKeeper.java:[3066,36] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZKUtil.java:[54,36] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.
[ERROR] src/main/java/org/apache/zookeeper/ZKUtil.java:[128,36] (javadoc) JavadocMethod: Unable to get class information for @throws tag 'KeeperException'.

cannot reproduce local. Still fail even suppress the rule. Digging...

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

saved with the last fixup. If we want to merge this pr into master in multi commits, ping me to rebase the commits or manually rebase and merge it.(merge the last fixup to the corresponding commit)

@eolivelli

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

Thank you @TisonKun

our commit script (zk-merge.py) will squash all of the commits.

I will review.

+ replyHdr.getErr() +
" expected Xid "
+ replyHdr.getXid() + " with err "
+ +replyHdr.getErr()

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

This + + can be cleaned up.

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 12, 2019

Author Contributor

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 comment has been minimized.

Copy link
@enixon

enixon Aug 12, 2019

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.

@enixon
Copy link

left a comment

This is an awesome change! A long time coming and a very welcome cleaning up of old format. Thank you!

The size of the PR is going to be an issue but I think a manageable one. There are nits to be addressed so multiple eyes to go over the changes would be very good. I'm not worried about the effect on other outstanding PRs as they ought to be able to handle the merge conflicts fairly easily.

I tried to call out anything notable, consequently some of my comments can be ignored as not directly actionable on this PR (it would just be nice to accomplish all style modifications in one sweep). Made it to DataTreeMXBean - will continue later.

Do we know whether the change in visibility to some of the methods will effect curator or 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?

Map<EventType, Set<Watcher>> removedWatchers = new HashMap<>();
HashSet<Watcher> childWatchersToRem = new HashSet<>();
removedWatchers.put(EventType.ChildWatchRemoved, childWatchersToRem);
HashSet<Watcher> dataWatchersToRem = new HashSet<>();

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Do these need to be HashSet or can they be Set?

HostProvider aHostProvider,
ZKClientConfig clientConfig
) throws IOException {
LOG.info("Initiating client connection, connectString="

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Log formatting could be cleaned.

HostProvider aHostProvider,
ZKClientConfig clientConfig
) throws IOException {
LOG.info("Initiating client connection, connectString=" + connectString

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Log formatting could be cleaned.


protected MyCommandOptions cl = new MyCommandOptions();
protected HashMap<Integer,String> history = new HashMap<Integer,String>( );
protected HashMap<Integer, String> history = new HashMap<>();

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

HashMap still necessary or can it be Map?

@@ -279,8 +287,8 @@ public void respondToServer(byte[] serverToken, ClientCnxn cnxn) {
sendSaslPacket(saslToken, cnxn);
}
} catch (SaslException e) {
LOG.error("SASL authentication failed using login context '" +
this.getLoginContext() + "' with exception: {}", e);
LOG.error("SASL authentication failed using login context '"

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Mix of log formats?

@@ -289,8 +297,9 @@ public void respondToServer(byte[] serverToken, ClientCnxn cnxn) {
if (saslClient.isComplete()) {
// GSSAPI: server sends a final packet after authentication succeeds
// or fails.
if ((serverToken == null) && (saslClient.getMechanismName().equals("GSSAPI")))
if ((serverToken == null) && (saslClient.getMechanismName().equals("GSSAPI"))) {

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

A thought - would we like our checkstyle to flag "variable.equals(constant)" as less preferable than "constant.equals(variable)"?

try {
setCversionPzxid(parentName, cTxn.getParentCVersion(),
header.getZxid());
} catch (KeeperException.NoNodeException e) {
LOG.error("Failed to set parent cversion for: " +
parentName, e);
LOG.error("Failed to set parent cversion for: " + parentName, e);

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Log formatting

.debug("Deleting ephemeral node " + path
+ " for session 0x"
+ Long.toHexString(session));
LOG.debug("Deleting ephemeral node " + path + " for session 0x" + Long.toHexString(session));

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Log formatting

@enixon
Copy link

left a comment

Made it to AdminServer.java - it's slow going because of quantity, not because any single change is difficult to understand. :)

Once again, excellent work @TisonKun .

@lvfangmin and @jhuan31 , fyi

@@ -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

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

dropped noinspection?

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 12, 2019

Author Contributor

Invalid any more.

@@ -176,7 +166,8 @@ public static EphemeralType get(long ephemeralOwner) {
long extendedFeatureBit = getExtendedFeatureBit(ephemeralOwner);
EphemeralType ephemeralType = extendedFeatureMap.get(extendedFeatureBit);
if (ephemeralType == null) {
throw new IllegalArgumentException(String.format("Invalid ephemeralOwner. [%s]", Long.toHexString(ephemeralOwner)));
throw new IllegalArgumentException(
String.format("Invalid ephemeralOwner. [%s]", Long.toHexString(ephemeralOwner)));

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Would be great to add the 0x

*
* @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,

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

This is the kind of TODO that I'm wondering how the community tracks. In my perfect world, each is accompanied by a comment like "ZK-156".

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 12, 2019

Author Contributor

Support. This is also how akka community tracked TODOs and it was a good practice yet.

ChangeRecord cr = zks.outstandingChanges.remove();
ServerMetrics.getMetrics().OUTSTANDING_CHANGES_REMOVED.add(1);
if (cr.zxid < zxid) {
LOG.warn("Zxid outstanding " + cr.zxid
+ " is less than current " + zxid);
+ " is less than current " + zxid);

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Log formatting

case OpCode.ping: {
lastOp = "PING";
updateStats(request, lastOp, lastZxid);
case OpCode.ping: {

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

There's a lot of churn in the PR in this switch statement but as far as I can see, no logical changes.

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 12, 2019

Author Contributor

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.

@@ -534,36 +527,41 @@ public void closeAll(ServerCnxn.DisconnectReason reason) {
cnxn.close(reason);
} catch (Exception e) {
LOG.warn("Ignoring exception closing cnxn sessionid 0x"
+ Long.toHexString(cnxn.getSessionId()), e);
+ Long.toHexString(cnxn.getSessionId()), e);

This comment has been minimized.

Copy link
@enixon

private final ArrayList<ExecutorService> workers =
new ArrayList<ExecutorService>();
private final ArrayList<ExecutorService> workers = new ArrayList<>();

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Can this be List instead of ArrayList?

@@ -317,7 +331,7 @@ public boolean isTxnLogSyncEnabled() {
boolean enabled = snapshotSizeFactor >= 0;
if (enabled) {
LOG.info("On disk txn sync enabled with snapshotSizeFactor "
+ snapshotSizeFactor);
+ snapshotSizeFactor);

This comment has been minimized.

Copy link
@enixon
return;
int error;
if (shouldRequireClientSaslAuth()) {
LOG.warn("Closing client connection due to server requires client SASL authenticaiton,"

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

Mind fixing the spelling "authenticaiton" while you're at it? :)


/* Connection throttling settings */
public int getConnectionMaxTokens();
public void setConnectionMaxTokens(int val);

This comment has been minimized.

Copy link
@enixon

enixon Aug 11, 2019

This case is interesting. I feel like there is information in the line groupings of the method names in this class that is being lost as it shifts to the standardized format. I don't know if it's worth inserting comments to preserve that info.

This comment has been minimized.

Copy link
@nkalmar

nkalmar Aug 12, 2019

Contributor

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 comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 12, 2019

Author Contributor

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 comment has been minimized.

Copy link
@enixon

enixon Aug 12, 2019

@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.

@nkalmar

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

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.

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@nkalmar Thanks for your review and Thanks for @enixon 's comments. I would try to give a look and address them hours later.

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@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.

@hanm
Copy link
Contributor

left a comment

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?

@@ -65,7 +65,7 @@ public PemReaderTest(
}

@Test
public void testLoadPrivateKeyFromKeyStore() throws IOException, GeneralSecurityException {
public void testLoadPrivateKeyFromKeyStore() throws Exception {

This comment has been minimized.

Copy link
@hanm

hanm Aug 12, 2019

Contributor

Why change the exception type here?

public void testWatcherDisconnectOnClose()
throws IOException, InterruptedException, KeeperException
{
public void testWatcherDisconnectOnClose() throws Exception {

This comment has been minimized.

Copy link
@hanm

hanm Aug 12, 2019

Contributor

Unnecessary exception type change?

import org.apache.zookeeper.common.Time;

public class TestHammer implements VoidCallback {

static int REPS = 50000;
private static int reps = 50000;

This comment has been minimized.

Copy link
@hanm

hanm Aug 12, 2019

Contributor

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 comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 12, 2019

Contributor

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?

}

byte[] b = new byte[remaining];
int num_read = din.read(b);
int numRead = din.read(b);

This comment has been minimized.

Copy link
@hanm

hanm Aug 12, 2019

Contributor

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 comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 12, 2019

Contributor

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 comment has been minimized.

Copy link
@enixon

enixon Aug 12, 2019

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 comment has been minimized.

Copy link
@hanm

hanm Aug 12, 2019

Contributor

@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.

@hanm

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

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.

@enixon
Copy link

left a comment

I've gone through all the non-test file changes and it looks good. Only a few comments.

Please disregard my previous comments on log formatting and use of interfaces, those can be pushed to a separate PR.

&& !max.compareAndSet(current, value))
;
while (value > (current = max.get()) && !max.compareAndSet(current, value)) {
// no op

This comment has been minimized.

Copy link
@enixon
if (ia.readByte("EOF") != 'B') {
LOG.error("Last transaction was partial.");
return null;
}
return bytes;
}catch(EOFException e){}
} catch (EOFException ignore) {

This comment has been minimized.

Copy link
@enixon

enixon Aug 12, 2019

should the pattern of adding a // no-op comment apply here?

};
}

;

This comment has been minimized.

Copy link
@enixon

enixon Aug 12, 2019

This semi-colon is strange. I think it's redundant?

}

byte[] b = new byte[remaining];
int num_read = din.read(b);
int numRead = din.read(b);

This comment has been minimized.

Copy link
@enixon

enixon Aug 12, 2019

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.

LOG.debug("Closing listener: {}",
+ QuorumCnxManager.this.mySid);
+QuorumCnxManager.this.mySid);

This comment has been minimized.

Copy link
@enixon

enixon Aug 12, 2019

This + character shouldn't be there.

* @see org.apache.zookeeper.server.PurgeTxnLog#purge(File, File, int)
*/
private final int MIN_SNAP_RETAIN_COUNT = 3;
private final int minSnapRetainCount = 3;

This comment has been minimized.

Copy link
@enixon

enixon Aug 12, 2019

would rather make static than change casing.

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@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.

@hanm

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

@TisonKun sounds good.

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

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

@eolivelli

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

@tisunkun thank you for this effort.
I don't know if you have to start from scratch or fix up this branch.
All of the commits will be squashed into a single one.

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@eolivelli

All of the commits will be squashed into a single one.

I am aware of this. In consideration of reviews, it would be better to start a new pull request?

@eolivelli

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

If we continue in this PR we won't lose the comments, so it may be better.
In my experience when a PR has too many comments github breaks.

Do whatever is more comfortable for you, I see it is an huge patch

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I have redone the task enable checkstyle configuration and resolve conflict. Basically it focuses on whitespaces, imports, operators on a new line, redundant or out-of-order modifiers, TODO comments, and if/else also with curly.

For javadoc discussion, let's move to ZOOKEEPER-3469.

For variable/class names discussion(whether change it or not on conflict with suggested name pattern), let's move to ZOOKEEPER-3507.

For a proper line break strategy, or even we don't want it, let's discuss in ZOOKEEPER-3508.

Log format issue is now tracked by ZOOKEEPER-3509.

ZOOKEEPER-3475: Enable checkstyle configuration on zookeeper-server
Signed-off-by: tison <wander4096@gmail.com>

@TisonKun TisonKun force-pushed the TisonKun:ZOOKEEPER-3475 branch from ce29c7a to 082a6d1 Aug 14, 2019

@eolivelli
Copy link
Contributor

left a comment

I am reviewing this diff.
I am still in the middle of the diff. I left comments for the first half.

server = "server." + i + "=localhost:" + PortAssignment.unique()
+ ":" + PortAssignment.unique() + ":participant;localhost:"
+ clientPorts[i];
server = "server."

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 15, 2019

Contributor

it is worth reverting

server = "server." + i + "=localhost:" + PortAssignment.unique()
+ ":" + PortAssignment.unique() + ":participant;localhost:"
+ clientPorts[i];
server = "server."

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 15, 2019

Contributor

it is worth reverting

+ qu.getPeer(i).peer.getQuorumAddress().getPort() + ":"
+ qu.getPeer(i).peer.getElectionAddress().getPort() + ";"
+ "127.0.0.1:" + qu.getPeer(i).peer.getClientPort());
members.add("server."

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 15, 2019

Contributor

it is worth reverting

server = "server." + i + "=localhost:" + PortAssignment.unique()
+ ":" + PortAssignment.unique() + ":participant;localhost:"
+ clientPorts[i];
server = "server."

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 15, 2019

Contributor

it is worth reverting

server = "server." + i + "=127.0.0.1:" + PortAssignment.unique()
+ ":" + PortAssignment.unique() + ":participant;127.0.0.1:"
+ clientPorts[i];
server = "server."

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 15, 2019

Contributor

it is worth reverting

@eolivelli
Copy link
Contributor

left a comment

I have finished my review.
That's a great and huge work

I left my final comments.
Summary of major comments:

  • I think it is worth to not change lines that generate "server.1.xxx" configurations.
  • I also left a comment about using "Assert.fail" instead of a "import static org.junit.Assert.fail"
+ ":" + (PORT_QP_LE2) + ";" + CLIENT_PORT_QP2
+ "\nserver.3=127.0.0.1:"
+ (PORT_OBS)+ ":" + (PORT_OBS_LE) + ":observer" + ";" + CLIENT_PORT_OBS;
String quorumCfgSection = "server.1=127.0.0.1:"

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 15, 2019

Contributor

it is worth reverting
can you take a look to all this similar cases in tests ?
I won't add any other comments on this kind of issue

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 15, 2019

Author Contributor

Yes. It requires another pass about line break things. I will take care of it.

} catch (KeeperException.NodeExistsException ke) {
// expected, sort of
} catch (KeeperException ke) {
Assert.fail("Unexpected exception code for create " + testDirOnZK + ": " + ke.getMessage());

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 15, 2019

Contributor

I noticed only now this fact: don't we have a rule to forbid the usage of "Assert.xxxx" and force the usage of import static "org.junit.Assert.fail" ?

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 15, 2019

Author Contributor

We can add a forbidden pattern import org.junit.Assert;

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 15, 2019

Author Contributor

the rule should be like

    <module name="RegexpSingleline">
        <!-- Checks that do not use import org.junit.Assert. -->
        <property name="format" value="import org\.junit\.Assert;" />
        <property name="message" value="Always import static org.junit.Assert.[static-method]." />
        <property name="severity" value="error" />
    </module>

but I would like do it in another pass since it would affect on other modules.

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 15, 2019

Contributor

Not needed for me.
Let's keep it as it is now.
Thanks

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 15, 2019

Author Contributor

Hmmm I have done it with a script remove import org.junit.Assert; and
replace \ Assert. with \ . And then add a redundant import static headers which is followed by an imports clean up automate phase.

I do it inside zookeeper-server scope and it should not be hard to review.(just import -> import static)

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Line breaks, i.e., confusing line formats request changed above, suffer from lack usage of local variables, which causes quite a lot long line without semantic awareness. I would like to add some local variables to help formatting. Since it is limited in local variables, it would not conflict outside world.

@eolivelli

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

I think we are on our way.
Ping me when you are done

If the other guys (@hanm @enixon) that started a review approve this change I will be happy to commit it as soon as possible

TisonKun added 2 commits Aug 15, 2019
@hanm

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

reviewing, feedback on the way.

TisonKun added 5 commits Aug 16, 2019
fixup! fixup! fixup! fixup! fixup! fixup! ZOOKEEPER-3475: Enable chec…
…kstyle configuration on zookeeper-server
fixup! fixup! fixup! fixup! fixup! fixup! fixup! ZOOKEEPER-3475: Enab…
…le checkstyle configuration on zookeeper-server
fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! ZOOKEEPER-347…
…5: Enable checkstyle configuration on zookeeper-server
fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! ZOOKEE…
…PER-3475: Enable checkstyle configuration on zookeeper-server
fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…
… ZOOKEEPER-3475: Enable checkstyle configuration on zookeeper-server
@hanm
Copy link
Contributor

left a comment

about half way through; some comments.

serverAddress,
RETRY_CONN_MSG,
e);
LOG.warn("Session 0x{} for server {}, unexpected error{}", Long.toHexString(getSessionId()), serverAddress, RETRY_CONN_MSG, e);

This comment has been minimized.

Copy link
@hanm

hanm Aug 16, 2019

Contributor

This line after reformatting looks awfully long. Is it possible to teach check style so it insert line breaks when a single line exceeds certain preconfigured width?

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 16, 2019

Author Contributor

It is possible that we enable LineLength rule on zookeeper-server that forbid every line exceed a preconfigure value. See ZOOKEEPER-3508 for it.

We can tell an auto-formatter to break a line but it would introduce awful breaks as Enrico request to change.

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 16, 2019

Author Contributor

In fact I locally turn on LineLength and see that there is about 200 files break the rules, a pass to take care of it is ok for this pr. But not all of them have a reasonable break down strategy to apply on. Some of them are just because too much indent level so break line length rule.

private static void visitSubTreeDFSHelper(ZooKeeper zk, final String path,
boolean watch, StringCallback cb)
throws KeeperException, InterruptedException {
private static void visitSubTreeDFSHelper(ZooKeeper zk, final String path, boolean watch, StringCallback cb) throws KeeperException, InterruptedException {

This comment has been minimized.

Copy link
@hanm

hanm Aug 16, 2019

Contributor

very long line again

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 16, 2019

Author Contributor

This pattern can be resolve easier. Since chop down method declaration is a fixed pattern.

/**
* This Id represents anyone.
*/
public final Id ANYONE_ID_UNSAFE = new Id("world", "anyone");
Id ANYONE_ID_UNSAFE = new Id("world", "anyone");

This comment has been minimized.

Copy link
@hanm

hanm Aug 16, 2019

Contributor

this removed final.. intended?

This comment has been minimized.

Copy link
@TisonKun

TisonKun Aug 16, 2019

Author Contributor

Any field in an interface must be public static final. There is a rule redundant modifier check this.

TisonKun added 3 commits Aug 16, 2019
fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…
… fixup! ZOOKEEPER-3475: Enable checkstyle configuration on zookeeper-server
fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…
… fixup! fixup! ZOOKEEPER-3475: Enable checkstyle configuration on zookeeper-server
fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…
… fixup! fixup! fixup! ZOOKEEPER-3475: Enable checkstyle configuration on zookeeper-server
@hanm
hanm approved these changes Aug 17, 2019
Copy link
Contributor

left a comment

LGTM.

@TisonKun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Thanks for your review @hanm !

@eolivelli I also gave it a second pass and reverted/addressed ugly formats by hand(line breaks almost, as mentioned above, hard to give a rule but will still digging). If you can give it another pass we possibly merge it before any conflict.

@eolivelli
Copy link
Contributor

left a comment

LGTM

Great work

CI is green.

@hanm can you please merge this patch? Otherwise I will do tomorrow

@asfgit asfgit closed this in fe940cd Aug 17, 2019

@TisonKun TisonKun deleted the TisonKun:ZOOKEEPER-3475 branch Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.