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
+
Camille Fournier 2011-12-28, 21:10
+
Patrick Hunt 2011-12-28, 21:24
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'm not sure I agree in the assumption that monitoring pulls happen
infrequently...
On Dec 28, 2011 4:24 PM, "Patrick Hunt" <[EMAIL PROTECTED]> wrote:

> They seem like distinct changes to me. In particular getting the size
> is going to happen infrequently (monitoring pull) so I don't see a
> problem fixing the existing patch in the same way the code currently
> handles cnxns access, with a separate jira to do the refactoring. Am I
> missing something?
>
> Patrick
>
> On Wed, Dec 28, 2011 at 1:10 PM, Camille Fournier <[EMAIL PROTECTED]>
> wrote:
> > 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:
+
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