----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21046/#review42223 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/service/DistributedUUIDService.java <https://reviews.apache.org/r/21046/#comment76012> All of the other HA Services are named ZKSomethingService; can we name this either ZKUUIDService or ZKDistributedUUIDService so it matches and looks like it belongs with them? core/src/main/java/org/apache/oozie/service/DistributedUUIDService.java <https://reviews.apache.org/r/21046/#comment76014> I think we should use a different RetryPolicy here. RetryOneTime will only retry once (with a 1 sec wait) if it fails to increment the ID; in other words, Oozie will fail to submit the job if it can't increment the ID twice in a row (I suppose that could happen with lots of busy Oozies?) I think we should use the ExponentialBackoffRetry like I used in ZKUtils: https://github.com/apache/oozie/blob/master/core/src/main/java/org/apache/oozie/util/ZKUtils.java#L156 ZKUtils is set to retry 3 times with an exponentially increasing wait, which is more likely to succeed (wireless router do something similar when they get collisions) core/src/main/java/org/apache/oozie/service/DistributedUUIDService.java <https://reviews.apache.org/r/21046/#comment76019> This could be a race condition with other Oozie servers. For example: - ID starts at 0 - Server A calls get() and receives 0 - Server B calls get() and receives 0 - Server A calls increment() - ID becomes 1 - Server B calls increment() - ID becomes 2 While the end value for the ID is correct, both Servers will get 0 for their ID, which will be bad. Instead, I think you just need to do this: value = atomicIdGenerator.increment(); (You're already checking value.succeeded() and returning value.preValue()) core/src/test/java/org/apache/oozie/service/TestDistributedUUIDService.java <https://reviews.apache.org/r/21046/#comment76021> This should be DistributedUUIDService (or whatever we're calling it), not ZKJobsConcurrencyService core/src/test/java/org/apache/oozie/service/TestDistributedUUIDService.java <https://reviews.apache.org/r/21046/#comment76024> Can you add a test that creates two DistributedUUIDServices (or whatever we're calling it) and tests for race conditions between the two? e.g. if you call them at the same time, they don't get the same ID - Robert Kanter On May 3, 2014, 12:21 a.m., Purshotam Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21046/ > ----------------------------------------------------------- > > (Updated May 3, 2014, 12:21 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1715 > https://issues.apache.org/jira/browse/OOZIE-1715 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-1715 Distributed ID sequence for HA > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/service/DistributedUUIDService.java > e69de29 > core/src/main/java/org/apache/oozie/service/UUIDService.java 836815d > core/src/test/java/org/apache/oozie/service/TestDistributedUUIDService.java > e69de29 > > Diff: https://reviews.apache.org/r/21046/diff/ > > > Testing > ------- > > UTC > > > Thanks, > > Purshotam Shah > >
