It's all about expected usage patterns. Locking in this case will be per instance and the likelihood of contention is low given no one's going to call add a lot or indeed toString a lot in a short space of time.
So given: Define how concurrency is handled then implement And assuming it's in line with the above contention model I'd say synchronized would be adequate. On 8 February 2011 14:54, Gregg Wonderly <gr...@wonderly.org> wrote: > On 2/7/2011 5:28 PM, Peter Firmstone wrote: > >> Do we really need a concurrent version of this class? How often will Entry >> classes change? >> > > My choice is always to consider concurrency first, unless there is some > element of performance that this impacts. The simple fact is that there is > a toString() that traverses the contents. So, you either must synchronize > all access to the contents, or use a CHM which solves the problem without > any future concurrency surprises. > > I'm really serious about this, because of how I've encountered tons of > "synchronized" blocks in river which introduce huge latency in "processing" > because they cause everyone to wait in line for stuff which very often is > quite independent. > > Yes, this structure is mostly one time mutated. But, its design exposes a > continuous mutation capability. Thus, any "read" of the contents has to > take that into account. > > Gregg Wonderly > > > Gregg Wonderly wrote: >> >>> Yes definitely the case. CHM just makes it a lot easier to not worry >>> about map >>> and set data structures in terms of concurrent access of mutating data. >>> Set >>> and HashSet are useful, but not always in concurrent mutating data >>> management. >>> >>> I still prefer Vector over ArrayList just because it eliminates iteration >>> problems that alway require the same lines of code to resolve. >>> >>> Gregg >>> >>> Sent from my iPhone >>> >>> On Feb 7, 2011, at 2:08 PM, Dan Creswell <dan.cresw...@gmail.com> wrote: >>> >>> All true, still need to document the concurrent contract for this class >>>> and >>>> implement accordingly. >>>> >>>> i.e. We can say "toString" is thread safe etc and then use >>>> ConcurrentHashMap >>>> and friends. >>>> >>>> But we gotta do it right.... >>>> >>>> On 7 February 2011 19:57, Gregg Wonderly <gr...@wonderly.org> wrote: >>>> >>>> On 2/7/2011 5:24 AM, Tom Hobbs wrote: >>>>> >>>>> Sorry, I've got my nit-picking hat on... >>>>>> >>>>>> The null check in the equals method isn't needed because you get a >>>>>> free one by using "instanceof". >>>>>> >>>>>> I know you've warned that the class is not thread-safe, but people >>>>>> often forget that doing a sysout of an object calls toString(). I'd >>>>>> still be tempted to whack some protection into toString to stop it >>>>>> blowing up when people start printing lots of debug messages. >>>>>> >>>>>> Other than that it looks good to me. >>>>>> >>>>>> I'd suggest that it would be easy enough to just use >>>>> ConcurrentHashMap >>>>> instead of HashSet if we are going to move to Java 5 anyway. Then, you >>>>> don't have to worry about concurrency issues with toString() etc. >>>>> >>>>> Gregg Wonderly >>>>> >>>>> >>>>> On Mon, Feb 7, 2011 at 10:56 AM, Peter Firmstone<j...@zeus.net.au> >>>>> >>>>>> wrote: >>>>>> >>>>>> Dan Creswell wrote: >>>>>>> >>>>>>> Solid - probably ought to be a constructor that doesn't add defaults >>>>>>>> too.....and maybe a note about thread safety on add. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> The finished article. >>>>>>> >>>>>>> /* >>>>>>> * Licensed to the Apache Software Foundation (ASF) under one >>>>>>> * or more contributor license agreements. See the NOTICE file >>>>>>> * distributed with this work for additional information >>>>>>> * regarding copyright ownership. The ASF licenses this file >>>>>>> * to you under the Apache License, Version 2.0 (the >>>>>>> * "License"); you may not use this file except in compliance >>>>>>> * with the License. You may obtain a copy of the License at >>>>>>> * >>>>>>> * http://www.apache.org/licenses/LICENSE-2.0 >>>>>>> * >>>>>>> * Unless required by applicable law or agreed to in writing, software >>>>>>> * distributed under the License is distributed on an "AS IS" BASIS, >>>>>>> * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>>>>>> implied. >>>>>>> * See the License for the specific language governing permissions and >>>>>>> * limitations under the License. >>>>>>> */ >>>>>>> >>>>>>> package org.apache.river.api.lookup; >>>>>>> >>>>>>> import java.util.HashSet; >>>>>>> import java.util.Iterator; >>>>>>> import java.util.Set; >>>>>>> import net.jini.lookup.entry.Address; >>>>>>> import net.jini.lookup.entry.Comment; >>>>>>> import net.jini.lookup.entry.Location; >>>>>>> import net.jini.lookup.entry.Name; >>>>>>> import net.jini.lookup.entry.ServiceInfo; >>>>>>> import net.jini.lookup.entry.Status; >>>>>>> import net.jini.lookup.entry.UIDescriptor; >>>>>>> >>>>>>> /** >>>>>>> * A little builder utility class that creates an array of Entry >>>>>>> classes >>>>>>> to >>>>>>> * be used as a parameter for StreamServiceRegistrar. All the jini >>>>>>> platform >>>>>>> * Entry's are included by default. >>>>>>> * >>>>>>> * Note: This class is not threadsafe, use external synchronization if >>>>>>> required. >>>>>>> * >>>>>>> * Suggested by Dan Creswell. >>>>>>> * @author peter >>>>>>> */ >>>>>>> public class DefaultEntries { >>>>>>> private final Set<Class> entrys; >>>>>>> public DefaultEntries() { >>>>>>> entrys = new HashSet<Class>(16); >>>>>>> } >>>>>>> /** >>>>>>> * Add an Entry class. >>>>>>> * @param cl - class >>>>>>> * @return this >>>>>>> */ >>>>>>> public DefaultEntries add(Class cl){ >>>>>>> entrys.add(cl); >>>>>>> return this; >>>>>>> } >>>>>>> /** >>>>>>> * All all the Jini Platform Entry's >>>>>>> * @return >>>>>>> */ >>>>>>> public DefaultEntries addPlatformEntries(){ >>>>>>> add(Comment.class); >>>>>>> add(Location.class); >>>>>>> add(Name.class); >>>>>>> add(ServiceInfo.class); >>>>>>> add(Status.class); >>>>>>> add(UIDescriptor.class); >>>>>>> add(Address.class); >>>>>>> return this; >>>>>>> } >>>>>>> /** >>>>>>> * Remove all Entry's >>>>>>> */ >>>>>>> public void reset(){ >>>>>>> entrys.clear(); >>>>>>> } >>>>>>> /** >>>>>>> * Generate a new array containing all Entry's added since last reset. >>>>>>> * @return >>>>>>> */ >>>>>>> public Class[] getEntries(){ >>>>>>> return entrys.toArray(new Class[entrys.size()]); >>>>>>> } >>>>>>> >>>>>>> @Override >>>>>>> public int hashCode() { >>>>>>> int hash = 3; >>>>>>> hash = 29 * hash + (this.entrys != null ? this.entrys.hashCode() : >>>>>>> 0); >>>>>>> return hash; >>>>>>> } >>>>>>> >>>>>>> @Override >>>>>>> public boolean equals(Object o){ >>>>>>> if (o == null) return false; >>>>>>> if (o instanceof DefaultEntries){ >>>>>>> if (entrys.equals(((DefaultEntries)o).entrys)) return true; >>>>>>> } >>>>>>> return false; >>>>>>> } >>>>>>> @Override >>>>>>> public String toString(){ >>>>>>> String newline = System.getProperty("line.separator"); >>>>>>> StringBuilder sb = new StringBuilder(256); >>>>>>> sb.append("DefaultEntries:"); >>>>>>> sb.append(newline); >>>>>>> Iterator<Class> it = entrys.iterator(); >>>>>>> while (it.hasNext()){ >>>>>>> sb.append(it.next().getName()); >>>>>>> sb.append(newline); >>>>>>> } >>>>>>> return sb.toString(); >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> >>>>>>> >>> >> >> >
