You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by maoling <gi...@git.apache.org> on 2017/07/12 12:35:50 UTC
[GitHub] zookeeper pull request #308: 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 <ma...@sina.com>
Date: 2017-07-12T12:11:20Z
ZOOKEEPER-2842: optimize the finish() of Send/RecvWorker in QuorumCnxManager and remove testInitiateConnection() and formate some codes
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #308: ZOOKEEPER-2842
Posted by maoling <gi...@git.apache.org>.
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);
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #308: ZOOKEEPER-2842:Optimize the finish() of Send/Re...
Posted by afine <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #308: ZOOKEEPER-2842:Optimize the finish() of Send/Re...
Posted by maoling <gi...@git.apache.org>.
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 )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #308: ZOOKEEPER-2842
Posted by hanm <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #308: ZOOKEEPER-2842:Optimize the finish() of Send/Re...
Posted by maoling <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #308: ZOOKEEPER-2842:Optimize the finish() of Send/Re...
Posted by afine <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #308: ZOOKEEPER-2842:Optimize the finish() of Send/Re...
Posted by maoling <gi...@git.apache.org>.
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){}`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #308: ZOOKEEPER-2842:Optimize the finish() of Send/Re...
Posted by afine <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---