> On aug. 10, 2018, 2:44 du, András Piros wrote: > > core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java > > Lines 143 (patched) > > <https://reviews.apache.org/r/67885/diff/5/?file=2068666#file2068666line143> > > > > What about using a `CopyOnWriteArrayList` or a `ConcurrentHashSet` > > instead?
Changed List to ConcurrentHashSet, although a sync block is still necessary (at least it seems). > On aug. 10, 2018, 2:44 du, András Piros wrote: > > core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java > > Lines 154 (patched) > > <https://reviews.apache.org/r/67885/diff/5/?file=2068666#file2068666line154> > > > > Shouldn't we remove only when `executor.execute()` happened before? This was the original implementation as well (that is, full --> throw away). > On aug. 10, 2018, 2:44 du, András Piros wrote: > > core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java > > Lines 163 (patched) > > <https://reviews.apache.org/r/67885/diff/5/?file=2068666#file2068666line163> > > > > Here we rely on the fact that `commandFinished()` is called only once > > per task. If called multiple times, we're in a problem. Yeah, there are tests (actually there was a bug which I spotted after checking more thoroughly) to make sure that there is only one decrement for every increment. > On aug. 10, 2018, 2:44 du, András Piros wrote: > > core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java > > Lines 167 (patched) > > <https://reviews.apache.org/r/67885/diff/5/?file=2068666#file2068666line167> > > > > Sure we need to give reference to `ThreadPoolExecutor` in a `public` > > way? It's just to make CallableQueueService happy (it has reference to an executor instance), this way we don't have to modify that class heavily. > On aug. 10, 2018, 2:44 du, András Piros wrote: > > core/src/main/java/org/apache/oozie/service/CallableQueueService.java > > Lines 165 (patched) > > <https://reviews.apache.org/r/67885/diff/5/?file=2068668#file2068668line165> > > > > Unsure why we need an `AtomicInteger` here. Probably not needed, this was the original implementation, didn't want to change it. > On aug. 10, 2018, 2:44 du, András Piros wrote: > > core/src/main/java/org/apache/oozie/service/CallableQueueService.java > > Lines 182 (patched) > > <https://reviews.apache.org/r/67885/diff/5/?file=2068668#file2068668line182> > > > > Unsure why we need an `AtomicInteger` here. See above. > On aug. 10, 2018, 2:44 du, András Piros wrote: > > core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java > > Lines 207 (patched) > > <https://reviews.apache.org/r/67885/diff/5/?file=2068670#file2068670line207> > > > > Are there 4 priority levels, from 0 to 3? Yes - added constants to make this clear. - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67885/#review207076 ----------------------------------------------------------- On aug. 15, 2018, 9:49 de, Peter Bacsko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67885/ > ----------------------------------------------------------- > > (Updated aug. 15, 2018, 9:49 de) > > > Review request for oozie, András Piros, Peter Cseh, Kinga Marton, and Robert > Kanter. > > > Repository: oozie-git > > > Description > ------- > > Still just a POC. > > Tests are almost completely missing. > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java > PRE-CREATION > core/src/main/java/org/apache/oozie/service/CallableQueueService.java > ef8d58da5 > core/src/main/java/org/apache/oozie/util/PriorityDelayQueue.java 75c20698c > core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java > PRE-CREATION > core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java > 9c2a11d6f > > > Diff: https://reviews.apache.org/r/67885/diff/6/ > > > Testing > ------- > > Executed TestCallableQueueService which passed completely. > > > Thanks, > > Peter Bacsko > >
