----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57870/#review169856 -----------------------------------------------------------
Overall direction is OK, please see my comments. core/src/main/java/org/apache/oozie/CoordinatorEngine.java Lines 1012 (patched) <https://reviews.apache.org/r/57870/#comment242541> I'd rename to `getWfActionByJobIdAndName()`. core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 43 (patched) <https://reviews.apache.org/r/57870/#comment242529> There are no code paths where `offset` or `len` wouldn't be overwritten in a constructor. Please remove initialization here and make both variables `final`. core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 47 (patched) <https://reviews.apache.org/r/57870/#comment242531> Why not have `50` as default `len`? Matching `CoordJobGetActionsSubsetJPAExecutor`, then. core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 51 (patched) <https://reviews.apache.org/r/57870/#comment242530> Why not have `action.info` as constant? And a newline between `super()` and the rest would make the code more readable. core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 67 (patched) <https://reviews.apache.org/r/57870/#comment242532> Logging an `ERROR` here wouldn't hurt. core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 78 (patched) <https://reviews.apache.org/r/57870/#comment242534> Logging a `WARN` here wouldn't hurt. Why not return `Collections.emptyList()`? core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 81 (patched) <https://reviews.apache.org/r/57870/#comment242535> Logging a `WARN` here wouldn't hurt. Why not return `Collections.emptyList()`? core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 85 (patched) <https://reviews.apache.org/r/57870/#comment242538> Doing a `DEBUG` log here wouldn't hurt. core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 87 (patched) <https://reviews.apache.org/r/57870/#comment242536> Explaining a bit more w/ a decent log message wouldn't hurt. core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 88 (patched) <https://reviews.apache.org/r/57870/#comment242537> Why not return `Collections.emptyList()`? core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java Lines 102-105 (patched) <https://reviews.apache.org/r/57870/#comment242540> 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. core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java Lines 280-285 (patched) <https://reviews.apache.org/r/57870/#comment242542> Why not have this default implementation in `BaseJobServlet` instead? core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java Lines 1142-1146 (patched) <https://reviews.apache.org/r/57870/#comment242543> Why not have this default implementation in `BaseJobServlet` instead? core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java Lines 401 (patched) <https://reviews.apache.org/r/57870/#comment242544> Please make it more resilient to `NumberFormatException`. core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java Lines 408 (patched) <https://reviews.apache.org/r/57870/#comment242545> Please make it more resilient to `NumberFormatException`. core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java Lines 411 (patched) <https://reviews.apache.org/r/57870/#comment242547> You can have it within `try` block. core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java Lines 42 (patched) <https://reviews.apache.org/r/57870/#comment242556> I'd also create more test scenarios here, like one testing boundaries for `offset` and `len`, and calling the parameterless constructor for both. core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java Lines 48-49 (patched) <https://reviews.apache.org/r/57870/#comment242549> Actually `LocalOozie.start()` already calls `Services.init()`, so no need to call that twice. core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java Lines 56 (patched) <https://reviews.apache.org/r/57870/#comment242550> Actually `LocalOozie.stop()` already calls `Services.destroy()`, so no need to do so twice. core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java Lines 62 (patched) <https://reviews.apache.org/r/57870/#comment242551> Maybe a message that `jpaService` could not be found. core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java Lines 64-78 (patched) <https://reviews.apache.org/r/57870/#comment242553> Extract methods `createCoordJob()` and `createCoordAction()` (called by the former 4 times). core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java Lines 91-98 (patched) <https://reviews.apache.org/r/57870/#comment242554> Have a unit test method only covering that use case with a decent name. core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java Lines 100-106 (patched) <https://reviews.apache.org/r/57870/#comment242555> Have a unit test method only covering that use case with a decent name. core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java Line 65 (original), 68 (patched) <https://reviews.apache.org/r/57870/#comment242557> Either rename `started` to `startedCoordJobs`, or have a POJO `StartedCoordinatorJob` that has `coordinatorJob` and `started` fields. Since right now we have both `CoordinatorJob` and `WorkflowActionBean` instances in this class, `started` is not more descriptive enough to understand what it refers to. core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java Lines 70 (patched) <https://reviews.apache.org/r/57870/#comment242558> Why 2? core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java Lines 261 (patched) <https://reviews.apache.org/r/57870/#comment242562> Great idea to add functional tests as well :) However, I'd focus more on the edge cases that can happen when calling `V2JobServlet` and weren't covered in other unit tests: unparseable query string for `offset` / `len`, querying a `jobId` that doesn't exist, calling `V0JobServlet` or `V1JobServlet` instead, etc. core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java Lines 266-273 (patched) <https://reviews.apache.org/r/57870/#comment242559> Extract to separate test method. core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java Lines 275-298 (patched) <https://reviews.apache.org/r/57870/#comment242560> Extract to separate test method. core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java Lines 290-291 (patched) <https://reviews.apache.org/r/57870/#comment242561> Since we have autoboxing, it's unnecessary to call `new Integer()`. - András Piros On March 23, 2017, 9:07 a.m., Alonzo Zhou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57870/ > ----------------------------------------------------------- > > (Updated March 23, 2017, 9:07 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 > 4e38b4a > core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 > > 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/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/1/ > > > Testing > ------- > > > Thanks, > > Alonzo Zhou > >
