[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16116209#comment-16116209
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/308#discussion_r131591962
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -880,19 +876,20 @@ public void run() {
  * 
  * @return boolean  Value of variable running
  */
-synchronized boolean finish() {
-if(!running){
-/*
- * Avoids running finish() twice. 
- */
-return running;
-}
-running = false;
-
-this.interrupt();
-threadCnt.decrementAndGet();
-return running;
-}
+   boolean finish() {
--- End diff --

Yes.we should keep a blance between optimization and readability.the most 
importance thing for this optimization is whether this function has faced a 
performace problem and this optimization is overdesign. 
Haha.self-criticism.However ,I find the shadow of this programming paradigm in 
zk code base 
[Line501~Line507](https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/ClientCnxn.java
 ) 



> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>
> 1.the finish() of Send/RecvWorker in QuorumCnxManager changes to 
> double-checked lock style 
> [https://en.wikipedia.org/wiki/Double-checked_locking]
> ,a trivial code changes implement a smaller granularity lock to have a better 
> perfermance in too fierce multithread situation.
> 2.testInitiateConnection() is redundant test function which is only used in 
> TestCase,so I refactor it.
> 3.some codes don't abide to Java Programme Specification ,so I lift a finger 
> to format them



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16116208#comment-16116208
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/308#discussion_r131591446
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -233,25 +233,25 @@ public QuorumCnxManager(QuorumPeer self) {
 listener = new Listener();
 listener.setName("QuorumPeerListener");
 }
-
+
 /**
- * Invokes initiateConnection for testing purposes
- * 
+ * establish Connection with sid using its electionAddr.
  * @param sid
+ * @param sock
+ * @throws IOException
  */
-public void testInitiateConnection(long sid) throws Exception {
-LOG.debug("Opening channel to server " + sid);
-Socket sock = new Socket();
+   public void establishConnection(Long sid, Socket sock) throws 
IOException {
+   LOG.debug("Opening channel to server " + sid);
 setSockOpts(sock);
 sock.connect(self.getVotingView().get(sid).electionAddr, cnxTO);
-initiateConnection(sock, sid);
-}
+LOG.debug("Connected to server " + sid);
+   }
 
 /**
  * If this server has initiated the connection, then it gives up on the
  * connection if it loses challenge. Otherwise, it keeps the 
connection.
  */
-public boolean initiateConnection(Socket sock, Long sid) {
+public boolean initiateConnection(Long sid, Socket sock) {
--- End diff --

I think put the mutable variables after fixed variables may be better in 
the parameter list;
such as :
`private void myMethod(int a, Integer b, String s1, StringBuilder s2){}`


> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>
> 1.the finish() of Send/RecvWorker in QuorumCnxManager changes to 
> double-checked lock style 
> [https://en.wikipedia.org/wiki/Double-checked_locking]
> ,a trivial code changes implement a smaller granularity lock to have a better 
> perfermance in too fierce multithread situation.
> 2.testInitiateConnection() is redundant test function which is only used in 
> TestCase,so I refactor it.
> 3.some codes don't abide to Java Programme Specification ,so I lift a finger 
> to format them



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16116205#comment-16116205
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/308#discussion_r131591226
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -233,25 +233,25 @@ public QuorumCnxManager(QuorumPeer self) {
 listener = new Listener();
 listener.setName("QuorumPeerListener");
 }
-
+
--- End diff --

I cannot help doing this although I know it will increase review work.I 
will keep it in my mind next time. **"When we say "do it separately" in the 
context of zk, it probably implies 'never'"** Quotation from hanm.


> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>
> 1.the finish() of Send/RecvWorker in QuorumCnxManager changes to 
> double-checked lock style 
> [https://en.wikipedia.org/wiki/Double-checked_locking]
> ,a trivial code changes implement a smaller granularity lock to have a better 
> perfermance in too fierce multithread situation.
> 2.testInitiateConnection() is redundant test function which is only used in 
> TestCase,so I refactor it.
> 3.some codes don't abide to Java Programme Specification ,so I lift a finger 
> to format them



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113511#comment-16113511
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/308#discussion_r131261856
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -233,25 +233,25 @@ public QuorumCnxManager(QuorumPeer self) {
 listener = new Listener();
 listener.setName("QuorumPeerListener");
 }
-
+
--- End diff --

style changes are great but would it be possible to separate them from 
changes to functionality? that way it becomes easier to understand changes that 
are made when going through the version control log


> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>
> 1.the finish() of Send/RecvWorker in QuorumCnxManager changes to 
> double-checked lock style 
> [https://en.wikipedia.org/wiki/Double-checked_locking]
> ,a trivial code changes implement a smaller granularity lock to have a better 
> perfermance in too fierce multithread situation.
> 2.testInitiateConnection() is redundant test function which is only used in 
> TestCase,so I refactor it.
> 3.some codes don't abide to Java Programme Specification ,so I lift a finger 
> to format them



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113513#comment-16113513
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/308#discussion_r131262128
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -233,25 +233,25 @@ public QuorumCnxManager(QuorumPeer self) {
 listener = new Listener();
 listener.setName("QuorumPeerListener");
 }
-
+
 /**
- * Invokes initiateConnection for testing purposes
- * 
+ * establish Connection with sid using its electionAddr.
  * @param sid
+ * @param sock
+ * @throws IOException
  */
-public void testInitiateConnection(long sid) throws Exception {
-LOG.debug("Opening channel to server " + sid);
-Socket sock = new Socket();
+   public void establishConnection(Long sid, Socket sock) throws 
IOException {
+   LOG.debug("Opening channel to server " + sid);
 setSockOpts(sock);
 sock.connect(self.getVotingView().get(sid).electionAddr, cnxTO);
-initiateConnection(sock, sid);
-}
+LOG.debug("Connected to server " + sid);
+   }
 
 /**
  * If this server has initiated the connection, then it gives up on the
  * connection if it loses challenge. Otherwise, it keeps the 
connection.
  */
-public boolean initiateConnection(Socket sock, Long sid) {
+public boolean initiateConnection(Long sid, Socket sock) {
--- End diff --

nit: why did this need to be changed?


> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>
> 1.the finish() of Send/RecvWorker in QuorumCnxManager changes to 
> double-checked lock style 
> [https://en.wikipedia.org/wiki/Double-checked_locking]
> ,a trivial code changes implement a smaller granularity lock to have a better 
> perfermance in too fierce multithread situation.
> 2.testInitiateConnection() is redundant test function which is only used in 
> TestCase,so I refactor it.
> 3.some codes don't abide to Java Programme Specification ,so I lift a finger 
> to format them



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16113512#comment-16113512
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/308#discussion_r131261499
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -880,19 +876,20 @@ public void run() {
  * 
  * @return boolean  Value of variable running
  */
-synchronized boolean finish() {
-if(!running){
-/*
- * Avoids running finish() twice. 
- */
-return running;
-}
-running = false;
-
-this.interrupt();
-threadCnt.decrementAndGet();
-return running;
-}
+   boolean finish() {
--- End diff --

I understand what you are trying to do here and why this is optimized. I'm 
just concerned that that any performance increase here would not be worth the 
decrease in readability.


> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>
> 1.the finish() of Send/RecvWorker in QuorumCnxManager changes to 
> double-checked lock style 
> [https://en.wikipedia.org/wiki/Double-checked_locking]
> ,a trivial code changes implement a smaller granularity lock to have a better 
> perfermance in too fierce multithread situation.
> 2.testInitiateConnection() is redundant test function which is only used in 
> TestCase,so I refactor it.
> 3.some codes don't abide to Java Programme Specification ,so I lift a finger 
> to format them



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16092584#comment-16092584
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

Github user hanm commented on the issue:

https://github.com/apache/zookeeper/pull/308
  
Nice comment. Please be patient, there are a lot of pull requests these 
days and many of them have a higher priority (major feature, important bug 
fixes, and so on) and our limited number of reviewers have other duties in 
their day job / life. That said, we definitely value every contribution we 
receive, it just takes time to review all patches.

For this pull request, please:
* Update the JIRA with a detailed descriptions on motivations / impact of 
this change. If it's optimization describe why this optimization is needed and 
what impact it has (an example: the optimization could increase throughput of 
read/write mixed workload by 100x).

* Put a title on the pull request, and more details on the descriptions of 
the pull request.


> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>




--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16091242#comment-16091242
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

Github user maoling commented on the issue:

https://github.com/apache/zookeeper/pull/308
  
```
boolean isMyShitCodeReviewed = false;
while (!isMyShitCodeReviewed) {
synchronized (this) {
feelSad();
feelLonely();
feelDespaired();
}   
Thread.sleep(24 * 3600 * 1000);
}
```


> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>




--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16083928#comment-16083928
 ] 

Hadoop QA commented on ZOOKEEPER-2842:
--

-1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+1 tests included.  The patch appears to include 3 new or modified tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

-1 core tests.  The patch failed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/877//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/877//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/877//console

This message is automatically generated.

> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>




--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2842) optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formates some codes

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16083903#comment-16083903
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2842:
---

GitHub user maoling opened a pull request:

https://github.com/apache/zookeeper/pull/308

ZOOKEEPER-2842

1.optimize the finish() of Send/RecvWorker in QuorumCnxManager 
2.remove testInitiateConnection() 
3. formates some codes

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/maoling/zookeeper ZOOKEEPER-2842

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/308.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #308


commit 71f1d9380b2617a1fdd27de448b6027ae1df40ce
Author: maoling 
Date:   2017-07-12T12:11:20Z

ZOOKEEPER-2842: optimize the finish() of Send/RecvWorker in 
QuorumCnxManager and remove testInitiateConnection() and formate some codes




> optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove 
> testInitiateConnection() and formates some codes
> 
>
> Key: ZOOKEEPER-2842
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2842
> Project: ZooKeeper
>  Issue Type: Improvement
>  Components: quorum
>Reporter: maoling
>Priority: Trivial
>




--
This message was sent by Atlassian JIRA
(v6.4.14#64029)