> On March 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java > > Lines 102-105 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672527#file1672527line102> > > > > I'm wondering whether some of the SQLs and JPA calls of `execute()` > > couldn't be done here. `loadState()` is called only once, but `execute()` > > possibly many times. > > Alonzo Zhou wrote: > hi , if call XCommand.call(), loadState() and execute() will both > execute, so i think this doesnt much matter. did i think wrong?
You're right, these are meant to be called exactly the same times. Nevertheless, semantically it would be much nicer to use `loadState()` for - err, loading the state of the command :D > On March 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java > > Lines 280-285 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672529#file1672529line280> > > > > Why not have this default implementation in `BaseJobServlet` instead? > > Alonzo Zhou wrote: > hi, i noticed that the methods in BaseJobServlet was defined as an > abstract method in addition to doGet and doPut. in order to maintain > consistency, then i defined getWfActionsInCoord as an abstract. There are already non-abstract methods like `checkAuthorizationForApp()` present in `BaseJobServlet`, so I wouldn't mind putting there a `protected` method those implementation is the same for all children. Having duplicate code is never OK - [DRY principle](https://en.wikipedia.org/wiki/Don't_repeat_yourself). > On March 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java > > Lines 1142-1146 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672530#file1672530line1142> > > > > Why not have this default implementation in `BaseJobServlet` instead? > > Alonzo Zhou wrote: > hi, i noticed that the methods in BaseJobServlet was defined as an > abstract method in addition to doGet and doPut. in order to maintain > consistency, then i defined getWfActionsInCoord as an abstract. There are already non-abstract methods like `checkAuthorizationForApp()` present in `BaseJobServlet`, so I wouldn't mind putting there a `protected` method those implementation is the same for all children. Having duplicate code is never OK - [DRY principle](https://en.wikipedia.org/wiki/Don't_repeat_yourself). - András ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57870/#review169856 ----------------------------------------------------------- On March 28, 2017, 9:36 a.m., Alonzo Zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57870/ > ----------------------------------------------------------- > > (Updated March 28, 2017, 9:36 a.m.) > > > Review request for oozie. > > > Repository: oozie-git > > > Description > ------- > > More detailed view of the coordinator’s history can be observed from > perspective of workflow action. > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/client/rest/RestConstants.java > 8ddb1f8 > core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 > core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 > > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java > PRE-CREATION > core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 > core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java d3b4689 > core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 9356768 > core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d > core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a > > core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java > PRE-CREATION > > core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java > 4fc8653 > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 > > > Diff: https://reviews.apache.org/r/57870/diff/2/ > > > Testing > ------- > > > Thanks, > > Alonzo Zhou > >
