----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49770/#review142352 -----------------------------------------------------------
lens-api/src/main/java/org/apache/lens/api/scheduler/SchedulerJobInfo.java (line 55) <https://reviews.apache.org/r/49770/#comment208159> Rename variable too to reflect this is status, not state. lens-cube/src/main/java/org/apache/lens/cube/parse/TimerangeResolver.java (lines 132 - 136) <https://reviews.apache.org/r/49770/#comment208161> Do we have test cases for this? lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java (line 1116) <https://reviews.apache.org/r/49770/#comment208162> Let's mention unit too in the property name. probably `current.time.millis` lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/SchedulerService.java (line 55) <https://reviews.apache.org/r/49770/#comment207935> `@returns` lens-server-api/src/main/java/org/apache/lens/server/scheduler/SchedulerJobInstanceState.java (line 19) <https://reviews.apache.org/r/49770/#comment207937> `server.api.scheduler` lens-server-api/src/main/java/org/apache/lens/server/scheduler/SchedulerJobInstanceState.java (line 54) <https://reviews.apache.org/r/49770/#comment208163> Can we call it `State` instead of `STATE`? lens-server-api/src/main/java/org/apache/lens/server/scheduler/SchedulerJobInstanceState.java (line 185) <https://reviews.apache.org/r/49770/#comment208164> Same as above: Let's name it `Event` instead of `EVENT`. lens-server-api/src/main/java/org/apache/lens/server/scheduler/SchedulerJobState.java (line 19) <https://reviews.apache.org/r/49770/#comment208165> `org.apache.lens.server.api.scheduler` lens-server-api/src/main/java/org/apache/lens/server/scheduler/SchedulerJobState.java (line 54) <https://reviews.apache.org/r/49770/#comment208166> `State`. http://stackoverflow.com/a/3069863/459384 lens-server/src/main/java/org/apache/lens/server/scheduler/ScheduleResource.java (line 92) <https://reviews.apache.org/r/49770/#comment208167> This might be confusing. In listing queries, there's a form param named `state`. Here, we have one named `status`. lens-server/src/main/java/org/apache/lens/server/scheduler/ScheduleResource.java (line 204) <https://reviews.apache.org/r/49770/#comment208168> Can we return some more helpful info for the user? When will the operation not be successful? lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerDAO.java (lines 140 - 144) <https://reviews.apache.org/r/49770/#comment208169> Mismatch. One place uses `state`, another uses `status`. lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerEventListener.java (line 35) <https://reviews.apache.org/r/49770/#comment208170> Let's take the value from configutation. We might need an option to dynamically size the scheduler event listener depending on scale. lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerEventListener.java (line 84) <https://reviews.apache.org/r/49770/#comment208171> Let's try to use `QueryExecutionService` instead of `Impl` lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerEventListener.java (line 93) <https://reviews.apache.org/r/49770/#comment208172> Same as above lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerEventListener.java (line 116) <https://reviews.apache.org/r/49770/#comment208173> Shall we move line 97 to here? There is no use uf `currentTime` between there and here. lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerEventListener.java (line 135) <https://reviews.apache.org/r/49770/#comment208174> Can we remove this? If not, can you provide some more details in the comment itself wrt what handling is required? lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerEventListener.java (line 148) <https://reviews.apache.org/r/49770/#comment208175> Shall we move the `nextTransition` method to the enum instead of the `State` object? That way, creating such new instances can be avoided. lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerServiceImpl.java (line 58) <https://reviews.apache.org/r/49770/#comment208176> +1 lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerServiceImpl.java (line 151) <https://reviews.apache.org/r/49770/#comment208177> We might want to estimate with fail-on-partial set to false. lens-server/src/main/java/org/apache/lens/server/scheduler/util/UtilityMethods.java (line 19) <https://reviews.apache.org/r/49770/#comment208178> we have another utility class `org.apache.lens.server.util.UtilityMethods`. Let's merge this class into that. - Rajat Khandelwal On July 15, 2016, 2:42 p.m., Lavkesh Lahngir wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49770/ > ----------------------------------------------------------- > > (Updated July 15, 2016, 2:42 p.m.) > > > Review request for lens. > > > Bugs: LENS-128 > https://issues.apache.org/jira/browse/LENS-128 > > > Repository: lens > > > Description > ------- > > Implementaion of core scheduler: > > > Diffs > ----- > > lens-api/src/main/java/org/apache/lens/api/scheduler/SchedulerJobInfo.java > 7d06689 > > lens-api/src/main/java/org/apache/lens/api/scheduler/SchedulerJobInstanceInfo.java > 8158576 > > lens-api/src/main/java/org/apache/lens/api/scheduler/SchedulerJobInstanceRun.java > PRE-CREATION > lens-cube/src/main/java/org/apache/lens/cube/parse/TimerangeResolver.java > 33ec9d9 > > lens-server-api/src/main/java/org/apache/lens/server/api/LensConfConstants.java > bd9b1ab > > lens-server-api/src/main/java/org/apache/lens/server/api/events/SchedulerAlarmEvent.java > 3ca7eb9 > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/SchedulerService.java > d0af876 > > lens-server-api/src/main/java/org/apache/lens/server/scheduler/SchedulerJobInstanceState.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/scheduler/SchedulerJobState.java > PRE-CREATION > lens-server/src/main/java/org/apache/lens/server/BaseLensService.java > 74bc0be > lens-server/src/main/java/org/apache/lens/server/LensServices.java 7618669 > > lens-server/src/main/java/org/apache/lens/server/scheduler/ScheduleResource.java > 39c4d98 > > lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerDAO.java > bf99fde > > lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerEventListener.java > PRE-CREATION > > lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerQueryEventListener.java > PRE-CREATION > > lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerServiceImpl.java > 3952671 > > lens-server/src/main/java/org/apache/lens/server/scheduler/notification/services/AlarmService.java > a4cdd83 > > lens-server/src/main/java/org/apache/lens/server/scheduler/state/SchedulerJobInstanceState.java > 95057e4 > > lens-server/src/main/java/org/apache/lens/server/scheduler/state/SchedulerJobState.java > d21cd05 > > lens-server/src/main/java/org/apache/lens/server/scheduler/util/UtilityMethods.java > 31783ad > > lens-server/src/main/java/org/apache/lens/server/session/LensSessionImpl.java > e77c7fa > > lens-server/src/test/java/org/apache/lens/server/scheduler/SchedulerDAOTest.java > d76a586 > > lens-server/src/test/java/org/apache/lens/server/scheduler/TestSchedulerServiceImpl.java > PRE-CREATION > > lens-server/src/test/java/org/apache/lens/server/scheduler/notification/services/AlarmServiceTest.java > 06883ae > > Diff: https://reviews.apache.org/r/49770/diff/ > > > Testing > ------- > > > Thanks, > > Lavkesh Lahngir > >
