[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2010-03-07 Thread Hadoop QA (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12842403#action_12842403
 ] 

Hadoop QA commented on ZOOKEEPER-59:


-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12438130/ZOOKEEPER-59.patch
  against trunk revision 919706.

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

-1 tests included.  The patch doesn't appear to include any new or modified 
tests.
Please justify why no tests are needed for this patch.

+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 warnings.

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

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

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

Test results: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/135/testReport/
Findbugs warnings: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/135/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/135/console

This message is automatically generated.

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch, ZOOKEEPER-59.patch, 
 ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2010-03-06 Thread Flavio Paiva Junqueira (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12842248#action_12842248
 ] 

Flavio Paiva Junqueira commented on ZOOKEEPER-59:
-

As I already mentioned above, I don't think it requires a test. Ben, could you 
please check my previous post to see if you agree?

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch, ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2010-03-06 Thread Benjamin Reed (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12842281#action_12842281
 ] 

Benjamin Reed commented on ZOOKEEPER-59:


ah yes, i see what you mean, but i think i would prefer a outstandingRequests 
that might be stale to a possible deadlock.

i agree that this doesn't need a test. this is a latent bug that is hard to be 
reproduced and a test would be very specific to the implementation of the bug.

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch, ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2010-03-05 Thread Hadoop QA (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12842186#action_12842186
 ] 

Hadoop QA commented on ZOOKEEPER-59:


-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12437656/ZOOKEEPER-59.patch
  against trunk revision 919640.

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

-1 tests included.  The patch doesn't appear to include any new or modified 
tests.
Please justify why no tests are needed for this patch.

+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 warnings.

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

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

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

Test results: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/131/testReport/
Findbugs warnings: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/131/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/131/console

This message is automatically generated.

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch, ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2010-03-03 Thread Benjamin Reed (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12840735#action_12840735
 ] 

Benjamin Reed commented on ZOOKEEPER-59:


i like how you removed the nested locking, but it looks like you missed an 
opportunity in the last block. don't you really want to 

{noformat}
synchronized(this) {outstandingRequests--;}
{noformat}

so that you aren't locking this and this.factory?

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch, ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2010-03-03 Thread Flavio Paiva Junqueira (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12840800#action_12840800
 ] 

Flavio Paiva Junqueira commented on ZOOKEEPER-59:
-

Good point, Ben. I haven't done it because of the predicate:

{noformat}
outstandingRequests  1
{noformat}

in that block.

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch, ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2010-03-02 Thread Hadoop QA (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12840464#action_12840464
 ] 

Hadoop QA commented on ZOOKEEPER-59:


-1 overall.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12437656/ZOOKEEPER-59.patch
  against trunk revision 915956.

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

-1 tests included.  The patch doesn't appear to include any new or modified 
tests.
Please justify why no tests are needed for this patch.

+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 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: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/125/testReport/
Findbugs warnings: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/125/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/125/console

This message is automatically generated.

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch, ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2010-01-15 Thread Mahadev konar (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12800933#action_12800933
 ] 

Mahadev konar commented on ZOOKEEPER-59:


flavio, will you be updating this patch?

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2009-10-08 Thread Patrick Hunt (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12763567#action_12763567
 ] 

Patrick Hunt commented on ZOOKEEPER-59:
---

Hi Flavio, what's the status of this patch?

 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Flavio Paiva Junqueira
 Fix For: 3.3.0

 Attachments: ZOOKEEPER-59.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 nioservercnxn.readrequ...@444
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 nioservercnxn.sendrespo...@740
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn

2008-07-08 Thread Flavio Paiva Junqueira (JIRA)

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12611796#action_12611796
 ] 

Flavio Paiva Junqueira commented on ZOOKEEPER-59:
-

Mahadev, Thanks for fixing the patch. My eclipse still hasn't leaned how to 
generate diffs appropriately. Your assessment of the patch is right, and the 
observation on outstandingRequests seems correct to me.

Andrew, I agree with you on getInProcess(). It is probably best to make it 
synchronized or simply to use an atomic integer for requestsInProcess. Now, on 
your description of the connection problem, I think this is an important 
observation because when I observed the problem, the log messages did not show 
the server receiving anything. Also, if the server receives and we don't 
disable sending, then either ping requests are not going through the pipeline 
of request processors (once it gets to FinalRequestProcessor, the server sends 
a response immediately) or sendResponse is not doing its job.



 Synchronized block in NIOServerCnxn
 ---

 Key: ZOOKEEPER-59
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59
 Project: Zookeeper
  Issue Type: Bug
  Components: server
Reporter: Flavio Paiva Junqueira
Assignee: Benjamin Reed
 Attachments: ZOOKEEPER-59_1.patch, ZOOKEEPER-59_2.patch


 There are two synchronized blocks locking on different objects, and to me 
 they should be guarded by the same object. Here are the parts of the code I'm 
 talking about:
 {noformat}
 [EMAIL PROTECTED]
 ...
   synchronized (this) {
 outstandingRequests++;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit) {
 disableRecv();
 // following lines should not be needed since we are 
 already
 // reading
 // } else {
 // enableRecv();
 }
 } 
 {noformat}
 {noformat}
 [EMAIL PROTECTED]
 ...
  synchronized (this.factory) {
 outstandingRequests--;
 // check throttling
 if (zk.getInProcess()  factory.outstandingLimit
 || outstandingRequests  1) {
 sk.selector().wakeup();
 enableRecv();
 }
 }
 {noformat}
 I think the second one is correct, and the first synchronized block should be 
 guarded by this.factory. 
 This could be related to issue ZOOKEEPER-57, but I have no concrete 
 indication that this is the case so far.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.