Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Zookeeper >> mail # dev >> Re: svn commit: r1225200 - in /zookeeper/trunk: ./ src/java/main/org/apache/zookeeper/server/ src/java/test/org/apache/zookeeper/server/quorum/ src/java/test/org/apache/zookeeper/test/


+
Patrick Hunt 2011-12-28, 17:45
+
Camille Fournier 2011-12-28, 18:12
+
Camille Fournier 2011-12-28, 18:31
+
Camille Fournier 2011-12-28, 19:18
+
Patrick Hunt 2011-12-28, 19:24
+
Ted Yu 2011-12-28, 20:58
+
Patrick Hunt 2011-12-28, 21:04
Copy link to this message
-
Re: svn commit: r1225200 - in /zookeeper/trunk: ./ src/java/main/org/apache/zookeeper/server/ src/java/test/org/apache/zookeeper/server/quorum/ src/java/test/org/apache/zookeeper/test/
I don't think creating it as a flat sync is a good idea, so if we want this
I think we need to refactor that structure to be concurrent.

C
On Dec 28, 2011 2:24 PM, "Patrick Hunt" <[EMAIL PROTECTED]> wrote:

> I think it needs to be fixed. It's obviously incorrect. Also if
> someone changes the underlying implementation at some point it might
> bite them.
>
> Given it wasn't applied to 3.4 branch yet I'd suggest revert, fix the
> patch, then reapply to both branches. (that's easiest/bulletproof imo)
>
> I'd recommend a new ticket to fix the issues you identified (refactor,
> etc...) Neha was looking for further items to work on, this would be a
> good one.
>
> Patrick
>
> On Wed, Dec 28, 2011 at 11:18 AM, Camille Fournier <[EMAIL PROTECTED]>
> wrote:
> > Also, Pat, the JIRA is still open. I will look to revert in a minute
> > although I think the sync changes may necessitate a new JIRA entirely.
> >
> > On Wed, Dec 28, 2011 at 1:31 PM, Camille Fournier <[EMAIL PROTECTED]>
> wrote:
> >> After looking for a few mins, here are my observations:
> >>
> >> The implementation of HashSet in jdk 1.6 uses an underlying HashMap,
> which
> >> uses a bog-standard int for the size. So, I completely agree that we can
> >> get invalid results for a point in time. But does anyone really care
> about
> >> the *exact* moment-in-time number of cnxns in the system, in a system
> where
> >> cnxns are coming and going? At best you'll see a value that will likely
> be
> >> out of date the moment you read it.
> >>
> >> But, if we're going to do this, I do think there may be a case for
> >> refactoring to a concurrent data structure given that we are randomly
> >> syncing on the cnxns set in a way that makes adding additional
> interactions
> >> with it error prone in this way. And we should definitely remove that
> >> getConnections method, if anyone ever iterated over that set they would
> be
> >> in a sad concurrency situation.
> >> Seems possibly worth a whole new ticket to do this. What do you think?
> >>
> >> Thanks,
> >> C
> >>
> >> On Wed, Dec 28, 2011 at 1:12 PM, Camille Fournier <[EMAIL PROTECTED]
> >wrote:
> >>
> >>> I'm not sure it's meaningful enough to be worth the sync overhead. We
> >>> should look into that. The alternative if we must is using a proper
> >>> concurrent collection. Neha, any thoughts?
> >>>
> >>> C
> >>> On Dec 28, 2011 12:45 PM, "Patrick Hunt" <[EMAIL PROTECTED]> wrote:
> >>>
> >>>> I believe there is a bug in this commit. The "cnxns" size() call is
> >>>> not being synchronized. This will lead to invalid results at best, at
> >>>> worst outright failure (hard to say w/o knowing the implementation of
> >>>> HashSet).
> >>>>
> >>>> Camille can you work with Neha to get this fixed? Perhaps in the
> >>>> meantime (if it's going to take a while) you can revert this change,
> >>>> re-open the jira, update the patch, and reapply at some later time?
> >>>>
> >>>> Patrick
> >>>>
> >>>> On Wed, Dec 28, 2011 at 6:55 AM,  <[EMAIL PROTECTED]> wrote:
> >>>> > Author: camille
> >>>> > Date: Wed Dec 28 14:55:37 2011
> >>>> > New Revision: 1225200
> >>>> >
> >>>> > URL: http://svn.apache.org/viewvc?rev=1225200&view=rev
> >>>> > Log:
> >>>> > ZOOKEEPER-1321: Add number of client connections metric in JMX and
> srvr
> >>>> (Neha Narkhede via camille)
> >>>> >
> >>>> > Modified:
> >>>> >    zookeeper/trunk/CHANGES.txt
> >>>> >
> >>>>
>  zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java
> >>>> >
> >>>>
>  zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java
> >>>> >
> >>>>
>  zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxn.java
> >>>> >
> >>>>
>  zookeeper/trunk/src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java
> >>>> >
> >>>>
>  zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java
> >>>> >
> >>>>
>  zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ServerStats.java
> >>>> >
> >>>>
>  zookeeper/trunk/src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java
+
Patrick Hunt 2011-12-28, 21:24
+
Camille Fournier 2011-12-28, 21:26
+
Patrick Hunt 2011-12-28, 21:32
+
Camille Fournier 2011-12-28, 21:53
+
Patrick Hunt 2011-12-28, 22:12
+
Camille Fournier 2011-12-28, 22:21
+
Patrick Hunt 2011-12-28, 22:45