Return-Path: X-Original-To: apmail-falcon-dev-archive@minotaur.apache.org Delivered-To: apmail-falcon-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 82C8A182F4 for ; Mon, 14 Sep 2015 19:09:46 +0000 (UTC) Received: (qmail 64494 invoked by uid 500); 14 Sep 2015 19:09:41 -0000 Delivered-To: apmail-falcon-dev-archive@falcon.apache.org Received: (qmail 64450 invoked by uid 500); 14 Sep 2015 19:09:41 -0000 Mailing-List: contact dev-help@falcon.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@falcon.apache.org Delivered-To: mailing list dev@falcon.apache.org Received: (qmail 64439 invoked by uid 99); 14 Sep 2015 19:09:40 -0000 Received: from Unknown (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 14 Sep 2015 19:09:40 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id 73CA41A1A1A for ; Mon, 14 Sep 2015 19:09:40 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 5.176 X-Spam-Level: ***** X-Spam-Status: No, score=5.176 tagged_above=-999 required=6.31 tests=[DKIM_ADSP_CUSTOM_MED=0.001, HEADER_FROM_DIFFERENT_DOMAINS=0.001, HTML_MESSAGE=3, KAM_LAZY_DOMAIN_SECURITY=1, NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, RP_MATCHES_RCVD=-0.006] autolearn=disabled Received: from mx1-us-east.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id UVdprKgLWnFe for ; Mon, 14 Sep 2015 19:09:36 +0000 (UTC) Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx1-us-east.apache.org (ASF Mail Server at mx1-us-east.apache.org) with SMTP id 1DB994457F for ; Mon, 14 Sep 2015 19:09:29 +0000 (UTC) Received: (qmail 63705 invoked by uid 99); 14 Sep 2015 19:09:23 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 14 Sep 2015 19:09:23 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 343B3273695; Mon, 14 Sep 2015 19:09:22 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7775077071370341273==" MIME-Version: 1.0 Subject: Re: Review Request 35724: Base framework of the native scheduler From: "Ajay Yadava" To: "Srikanth Sundarrajan" Cc: "Ajay Yadava" , "Falcon" , "Pallavi Rao" Date: Mon, 14 Sep 2015 19:09:22 -0000 Message-ID: <20150914190922.3773.18731@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Ajay Yadava" X-ReviewGroup: Falcon X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/35724/ X-Sender: "Ajay Yadava" References: <20150728110801.1540.36771@reviews.apache.org> In-Reply-To: <20150728110801.1540.36771@reviews.apache.org> Reply-To: "Ajay Yadava" X-ReviewRequest-Repository: falcon-git --===============7775077071370341273== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35724/#review98751 ----------------------------------------------------------- common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 257) This is incorrect, reason being that for certain frequencies like monthly the time in millis between each run keeps changing. This will fail e.g. for monthly feeds. scheduler/pom.xml (line 92) Any reasons to not use 2.2.1 which is the latest? scheduler/pom.xml (line 103) 1.10.19 is the latest. scheduler/pom.xml (line 109) why not 2.8.2? scheduler/src/main/java/org/apache/falcon/exception/DAGEngineException.java (line 23) Description seems to be incorrect. scheduler/src/main/java/org/apache/falcon/notification/service/FalconNotificationService.java (line 48) Incorrect javadoc, it is actually for registering. scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java (line 33) numInstances and it's getter setters are not being used. Please remove it. If you want for future then still delete it and file a JIRA. scheduler/src/main/java/org/apache/falcon/notification/service/event/JobCompletedEvent.java (line 28) Remove stray todo statement from all classes. scheduler/src/main/java/org/apache/falcon/notification/service/event/JobCompletedEvent.java (line 37) Constructor allows construction of incomplete object, which is not a good practice. scheduler/src/main/java/org/apache/falcon/notification/service/event/JobScheduledEvent.java (line 30) All constructors should form complete objects, i.e. all mandatory parameters should be available after construction. Please make this change for all the Event Classes scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 30) Remove stray TODOs scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 37) This field and it's getters and setters are unused. Please remove it. scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java (line 43) not used. Even if it is for future use cases then please remove it for now and file a JIRA so that it doesn't get dropped. scheduler/src/main/java/org/apache/falcon/notification/service/request/NotificationRequest.java (line 38) ORDER is not being set in any of the subclasses. scheduler/src/main/java/org/apache/falcon/notification/service/request/TimeNotificationRequest.java (line 31) Builder pattern should not be used for supplying mandatory patterns as it allows creation of incomplete objects. It should be used only for optional parameters. Please change this from all NotificationRequest subclasses. scheduler/src/main/java/org/apache/falcon/state/EntityState.java (line 71) Incorrect message. scheduler/src/main/java/org/apache/falcon/state/ID.java (line 33) ID is overloaded for both entity key and instance key. Both should have separate classes as the mandatory parameters for both of them are different. scheduler/src/main/java/org/apache/falcon/state/ID.java (line 145) toString seems to be used for InstanceKey this should be separated out. scheduler/src/main/java/org/apache/falcon/state/ID.java (line 189) This is incorrect. compareTo should be called only on objects of same/comparable type and not on generic object. If you use the parameterized version of " Comparator" this will give you compile time error. - Ajay Yadava On July 28, 2015, 11:07 a.m., Pallavi Rao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35724/ > ----------------------------------------------------------- > > (Updated July 28, 2015, 11:07 a.m.) > > > Review request for Falcon and Srikanth Sundarrajan. > > > Bugs: FALCON-1213 > https://issues.apache.org/jira/browse/FALCON-1213 > > > Repository: falcon-git > > > Description > ------- > > The patch has the basic framework for the scheduler. Each of the individual service needs to be implemented completely and will be done as separate JIRAs. The intention of the patch is ensure the base framework satisfies all use cases and get any early feedback in terms of course correction. > > > Diffs > ----- > > common/pom.xml 36de1f5 > common/src/main/java/org/apache/falcon/entity/EntityUtil.java b86d9d7 > common/src/main/java/org/apache/falcon/workflow/WorkflowJobEndNotificationService.java c4f8843 > common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java cfdc84d > pom.xml 31997e8 > scheduler/pom.xml PRE-CREATION > scheduler/src/main/java/org/apache/falcon/exception/DAGEngineException.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/exception/InvalidStateTransitionException.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/exception/ServiceException.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/exception/StateStoreException.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/EntityExecutor.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/NotificationHandler.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutor.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/FalconNotificationService.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/ServicesRegistry.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/event/DataEvent.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/event/Event.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/event/JobCompletedEvent.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/event/JobScheduledEvent.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/event/TimeElapsedEvent.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/impl/DataNotificationService.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/impl/JobCompletionService.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/impl/SchedulerService.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/impl/TimeNotificationService.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/request/DataNotificationRequest.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/request/JobCompletionNotificationRequest.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/request/JobScheduleNotificationRequest.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/request/NotificationRequest.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/notification/service/request/TimeNotificationRequest.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/EntityState.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/EntityStateChangeHandler.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/ID.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/InstanceState.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/InstanceStateChangeHandler.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/StateMachine.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/StateService.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/store/AbstractStateStore.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/workflow/engine/DAGEngine.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java PRE-CREATION > scheduler/src/main/java/org/apache/falcon/workflow/engine/OozieDAGEngine.java PRE-CREATION > scheduler/src/test/java/org/apache/falcon/execution/FalconExecutionServiceTest.java PRE-CREATION > scheduler/src/test/java/org/apache/falcon/notification/service/SchedulerServiceTest.java PRE-CREATION > scheduler/src/test/java/org/apache/falcon/notification/service/TimeNotificationServiceTest.java PRE-CREATION > scheduler/src/test/java/org/apache/falcon/predicate/PredicateTest.java PRE-CREATION > scheduler/src/test/java/org/apache/falcon/state/EntityStateServiceTest.java PRE-CREATION > scheduler/src/test/java/org/apache/falcon/state/InstanceStateServiceTest.java PRE-CREATION > scheduler/src/test/resources/config/cluster/cluster-0.1.xml PRE-CREATION > scheduler/src/test/resources/config/feed/feed-0.1.xml PRE-CREATION > scheduler/src/test/resources/config/process/process-0.1.xml PRE-CREATION > > Diff: https://reviews.apache.org/r/35724/diff/ > > > Testing > ------- > > New UTs have been added. > > > Thanks, > > Pallavi Rao > > --===============7775077071370341273==--