> On July 22, 2012, 4:32 p.m., John Sirois wrote: > > src/java/src/org/apache/mesos/state/InMemoryState.java, line 35 > > <https://reviews.apache.org/r/6079/diff/3/?file=125949#file125949line35> > > > > See below, but this is non-atomic, > > java.util.concurrent.ConcurrentMap#putIfAbsent
Done. > On July 22, 2012, 4:32 p.m., John Sirois wrote: > > src/java/src/org/apache/mesos/state/InMemoryState.java, line 77 > > <https://reviews.apache.org/r/6079/diff/3/?file=125949#file125949line77> > > > > static - you don't access the enclosing instance Done. > On July 22, 2012, 4:32 p.m., John Sirois wrote: > > src/java/src/org/apache/mesos/state/InMemoryState.java, line 101 > > <https://reviews.apache.org/r/6079/diff/3/?file=125949#file125949line101> > > > > static Done. > On July 22, 2012, 4:32 p.m., John Sirois wrote: > > src/java/src/org/apache/mesos/state/InMemoryState.java, line 65 > > <https://reviews.apache.org/r/6079/diff/3/?file=125949#file125949line65> > > > > People can turn these off and it seems better anyway to define what > > happens when set fails - ie: introduce a RuntimeException subclass or > > re-use IllegalStateException Removed it, will fail via ConcurrentMap.replace. > On July 22, 2012, 4:32 p.m., John Sirois wrote: > > src/java/src/org/apache/mesos/state/InMemoryState.java, line 64 > > <https://reviews.apache.org/r/6079/diff/3/?file=125949#file125949line64> > > > > This implementation is non-atomic which violates the State contract. A > > few things to conder, HashMap is fully synchronized and buys you nothing > > for check and act. What you want is found in > > java.util.concurrent.ConcurrentMap#replace > > John Sirois wrote: > Sorry about that - Hashtable is fully synchronized, but you don't use > that - you use HashMap. The fundamental issue of non-atomicity in get and > set still stands. Replaced HashMap with ConcurrentMap, added equals and hashCode to Entry and got ride of swap altogether in favor of ConcurrentMap.replace -- so much better! - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6079/#review9347 ----------------------------------------------------------- On July 22, 2012, 4:37 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6079/ > ----------------------------------------------------------- > > (Updated July 22, 2012, 4:37 a.m.) > > > Review request for mesos, John Sirois and Florian Leibert. > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/Makefile.am 10f1101 > src/java/src/org/apache/mesos/state/InMemoryState.java PRE-CREATION > src/java/src/org/apache/mesos/state/Variable.java PRE-CREATION > > Diff: https://reviews.apache.org/r/6079/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
