> On 三月 23, 2017, 11:35 a.m., András Piros wrote: > > Overall direction is OK, please see my comments.
Thanks for your reviewing man! I changed some following your advices, yet there're still some places that confuse me and I wrote them in comments. > On 三月 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java > > Lines 78 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672527#file1672527line78> > > > > Logging a `WARN` here wouldn't hurt. > > > > Why not return `Collections.emptyList()`? wfAction is a element of List<WorkflowActionBean> , in order to keep the length and order of the list returned. So add a null element. > On 三月 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java > > Lines 81 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672527#file1672527line81> > > > > Logging a `WARN` here wouldn't hurt. > > > > Why not return `Collections.emptyList()`? wfAction is a element of List<WorkflowActionBean> , in order to keep the length and order of the list returned. So add a null element. > On 三月 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java > > Lines 87 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672527#file1672527line87> > > > > Explaining a bit more w/ a decent log message wouldn't hurt. Yet after careful consideration, I still hold my point that this logging message is decent enough. > On 三月 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. hi , if call XCommand.call(), loadState() and execute() will both execute, so i think this doesnt much matter. did i think wrong? > On 三月 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? 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. > On 三月 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? 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. > On 三月 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java > > Lines 401 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672531#file1672531line401> > > > > Please make it more resilient to `NumberFormatException`. hi, the method service(HttpServletRequest request, HttpServletResponse response) in JsonRestServlet catch the IllegalArgumentException and handle it , I'm wondering whether we should catch the NumberFormatException here? > On 三月 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java > > Lines 408 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672531#file1672531line408> > > > > Please make it more resilient to `NumberFormatException`. hi, the method service(HttpServletRequest request, HttpServletResponse response) in JsonRestServlet catch the IllegalArgumentException and handle it , so I'm wondering whether we should catch the NumberFormatException here? > On 三月 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java > > Lines 70 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672533#file1672533line70> > > > > Why 2? no particular reason, just a value I picked, as I think it doesn't matter anyway > On 三月 23, 2017, 11:35 a.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java > > Lines 261 (patched) > > <https://reviews.apache.org/r/57870/diff/1/?file=1672534#file1672534line261> > > > > 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. :) thx for your advice. i add some tests: 1) unparseable query string for offset/len. 2) offset/len out of range. 3) calling V0JobServlet or V1JobServlet instead. but i think the case that jobId doesnt exist shouldnt test here, because it is CoordWfActionInfoXCommand, rather than v2JobServlet, that handles it. - Alonzo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57870/#review169856 ----------------------------------------------------------- On 三月 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 三月 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 > >
