> On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-api/src/main/resources/example-job.xml, line 43 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138730#file1138730line43> > > > > start_time says it is starting from 2014, but cron says run in 2005. We > > should change this to be a different year - something between start and end
Yes. Makes sense, will fix. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-api/src/main/resources/job-0.1.xsd, line 35 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138731#file1138731line35> > > > > Please add <xs:annotation><xs:documentation> for all elements and > > attributes. > > > > See cube-0.1.xsd, for example. <xs:annotation> is added at the definition of type not at the usage of type, it is already there for this type(and most others). I will again do a thorough check again if I have missed something. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/Scheduler.java, > > line 32 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138734#file1138734line32> > > > > I would say lets have class name as SchedulerService for interface and > > SchedulerServiceImpl for implementation to be consistent with other classes. The thought behind this was that names which contain suffix (like "Impl" for implementations) and prefix (like "I" for interface) are bad conventions, just like java doesn't have such conventions. However, I agree it is better to follow any convention consistently rather than mixing it up. Will change it. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/Scheduler.java, > > line 67 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138734#file1138734line67> > > > > Please explain return value for all methods Will fix. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/Scheduler.java, > > line 134 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138734#file1138734line134> > > > > why is getAllJobStats taking jobId? > > > > Also lets add state, user, name, long fromDate and toDate as other > > filters for getAllJobStats. You can look at getAllQueries in > > QueryExecutionService It's a copy paste error :( thanks for catching it. I will add filters. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/SchedulerJobInfo.java, > > line 31 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138735#file1138735line31> > > > > Shall we name the class ScheduleJobInfo instead of SchedulerJobInfo ? I actually wanted to keep it only job, just like Quartz but this terms is heavily overloaded so on the lines of MRJob (<component/framework>Job) I renamed it to SchedulerJob. SchedulerJob does look better to me as this name implies that this job belongs to scheduler. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/SchedulerJobInfo.java, > > line 75 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138735#file1138735line75> > > > > Please add comments to all params in all classes Will fix. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/SchedulerJobInstanceInfo.java, > > line 29 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138736#file1138736line29> > > > > ScheduleJobInstanceInfo? Same as above. Let me know if you still feel differently and I will be happy to rename it. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/Scheduler.java, > > line 71 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138734#file1138734line71> > > > > We should add getJobDetails - which returns SchedulerJobInfo. Makes sense. Will add. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-api/src/main/resources/job-0.1.xsd, line 1 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138731#file1138731line1> > > > > file name as job-0.1.xsd seems very generic. We should have it as > > schedule-0.1.xsd or schedule-job-0.1.xsd I will rename it to scheduler-job-0.1.xsd (on lines of SchedulerJobInfo, unless that also changes, see below) > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-api/src/main/java/org/apache/lens/api/util/DateUtil.java, line 56 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138728#file1138728line56> > > > > Instead of RuntimeException, can they defined as LensException or > > LensAPIException with error codes added? Makes sense, will add. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-api/src/main/java/org/apache/lens/api/util/DateUtil.java, line 32 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138728#file1138728line32> > > > > Can you add comments on what each method is for here? Since this goes > > in lens-api, i'm thinking you are planning to use all these on client side. > > > > If validation happens server side, we should move the class to > > lens-server-api I will add comments. Validations will happen on server side but since these classes are referred by jaxb, they need to be accessible in lens-api. > On Nov. 25, 2015, 7:11 a.m., Amareshwari Sriramadasu wrote: > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/Scheduler.java, > > line 70 > > <https://reviews.apache.org/r/40638/diff/1/?file=1138734#file1138734line70> > > > > Instead of simple String for jobID, Should we have a pojo similar to > > QueryHandle ? Will look at it, thanks for the suggestion. - Ajay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40638/#review107921 ----------------------------------------------------------- On Nov. 24, 2015, 12:20 p.m., Ajay Yadava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40638/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2015, 12:20 p.m.) > > > Review request for lens. > > > Bugs: LENS-879 > https://issues.apache.org/jira/browse/LENS-879 > > > Repository: lens > > > Description > ------- > > Base framework for lens scheduler service and job definition. > > > Diffs > ----- > > lens-api/src/main/java/org/apache/lens/api/util/DateUtil.java PRE-CREATION > lens-api/src/main/java/org/apache/lens/api/util/DateValidator.java > PRE-CREATION > lens-api/src/main/resources/example-job.xml PRE-CREATION > lens-api/src/main/resources/job-0.1.xsd PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/JobStats.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/QuerySchedulerService.java > 9f48d27 > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/Scheduler.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/SchedulerJobInfo.java > PRE-CREATION > > lens-server-api/src/main/java/org/apache/lens/server/api/scheduler/SchedulerJobInstanceInfo.java > PRE-CREATION > > lens-server/src/main/java/org/apache/lens/server/metrics/MetricsServiceImpl.java > 3389aba > > lens-server/src/main/java/org/apache/lens/server/scheduler/QuerySchedulerServiceImpl.java > 24949bd > > lens-server/src/main/java/org/apache/lens/server/scheduler/SchedulerService.java > PRE-CREATION > lens-server/src/main/resources/lensserver-default.xml 5f268cb > > lens-server/src/test/java/org/apache/lens/server/healthcheck/TestHealthChecks.java > e11413e > pom.xml b19857f > src/site/apt/admin/config.apt 88c1489 > > Diff: https://reviews.apache.org/r/40638/diff/ > > > Testing > ------- > > > Thanks, > > Ajay Yadava > >
