> On Feb. 3, 2015, 10:27 a.m., Shwetha GS wrote: > > common/src/main/java/org/apache/falcon/entity/lock/MemoryLocks.java, line 42 > > <https://reviews.apache.org/r/30116/diff/2/?file=843611#file843611line42> > > > > you can instantiate this at 'private static MemoryLocks instance' to > > avoid getInstance() synchronised > > Suhas Vasu wrote: > Out of curiousity, advantages of changing this to private static over > synchronized getInstance() ?
getInstance() is called from a lot of places. Instead of synchronising each call, initialising at static variable(which is guaranteed to be called once) is better. > On Feb. 3, 2015, 10:27 a.m., Shwetha GS wrote: > > common/src/main/java/org/apache/falcon/entity/lock/MemoryLocks.java, line 57 > > <https://reviews.apache.org/r/30116/diff/2/?file=843611#file843611line57> > > > > this should be synchronised so that 2 threads will not get the same lock > > Suhas Vasu wrote: > I am just relying on the concurrent hashmap to handle the syncronization > of adding to the map. should be ok right ? Take an example of 2 threads trying to acquire lock for the same entity at the same time with this sequence of execution(assuming its not already locked): t1 -> !locks.containsKey(entityName) will return true t2 -> !locks.containsKey(entityName) will also return true t1 -> locks.put(entityName, MAP_VALUE_CONSTANT) will get lock t2 -> locks.put(entityName, MAP_VALUE_CONSTANT) will also get lock Since acquireLock() is not synchronised, both t1 and t2 will get the lock in this case. Instead of making the method synchronised, you can use ConcurrentHashMap.putIfAbsent() and check the return type if lock is successful. putIfAbsent is atomic > On Feb. 3, 2015, 10:27 a.m., Shwetha GS wrote: > > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java, > > line 334 > > <https://reviews.apache.org/r/30116/diff/2/?file=843612#file843612line334> > > > > acquireLock() doesn't throw exception? > > Suhas Vasu wrote: > It might. In acquire lock we add the entry into a concurrent hash map. It > might throw up in case of concurrent writes. acquireLock() signature doesn't define any exception. When is the exception thrown? - Shwetha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30116/#review70728 ----------------------------------------------------------- On Feb. 2, 2015, 12:51 p.m., Suhas Vasu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30116/ > ----------------------------------------------------------- > > (Updated Feb. 2, 2015, 12:51 p.m.) > > > Review request for Falcon. > > > Repository: falcon-git > > > Description > ------- > > In distributed mode, when parallel update command are issued for same entity, > they are not syncronized. > This leads to multiple coordinators being spawned in Oozie. > > We need to ensure that the update method is syncronized. > > > Diffs > ----- > > common/src/main/java/org/apache/falcon/entity/lock/MemoryLocks.java > PRE-CREATION > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java > 0d34ef3 > > Diff: https://reviews.apache.org/r/30116/diff/ > > > Testing > ------- > > UT's were added and were successful > > > Thanks, > > Suhas Vasu > >
