> On Aug. 28, 2016, 7:10 p.m., Praveen Adlakha wrote: > > Here are my some overall comments. > > 1. There are no unit tests and user documentation (I know you intend to add > > it, just recording it for my personal future reference). > > 2. Effective time is valid only for process update and not for other > > entities. Please add this validation at both - client and server end. > > 3. In the current form, this will break instance search. Once the instances > > are killed, as graph db is not updated with effective time. > > 4. The design doc mentions parallelism, however I couldn't find any > > parallelism in jar copying etc. This might be a performance issue like the > > past > > 5. Design doc also contains reference to certain features like running only > > one failed instance between successful instances by passing a command line > > option called appPath which is missing. > > > > > > In the current form there are several functional and correctness issues as > > listed below. > > 1. This feature will break entity sla features as it contains some > > instances for monitoring sla and they might be affected with entity > > update(and sla update along with it) > > 2. Touch command is also a way to do an update, however it is not updated > > to accept effectiveTime parameter. > > 3. This feature will also not work in cases where input feed instances may > > have been deleted by retention and on passing effectiveTime from past those > > instances will not succeed. > > 4. Instance search feature will also break by these changes as we are not > > taking care of existing graph db instances. > > 5. After several effective updates, it will be hard to keep track of which > > workflow/code worked with which input/jars.
1. Added unit tests and documentation 2. Added the check at server level and client level. 3. This would be the same case when user kills any instance its no different. 4. I had changed the copy part to run in parallel. 5. Its not about running an instance between any reference points. Its about capability for rerunning a specific instance with a specific given lib path. For the same I don't want to handle as part of effective time. I had a raisea sub jira(FALCON-2168) under the umbrella jira 1. SLA features need to be handled separately. I had captured a jira for the same (FALCON-2169). I will send a separate patch for the same. But just to add more, this is no different from a user running a set of succeeded instances in the past where SLA wouldn't check. But we can still argue saying that this user has specifically called for an effective time , this should be handled in SLA. I will capture the changes in the jira mentioned above. 2. I had updated for touch too. 3. This is explicitly user's call. Again this is no different from user rerunning a very old instance where input feed retention has deleted the data. In which case the same would be applied to rerun too. On another note, user might not have scheduled the feed at all. In short, this is user's understanding on all the process' input to handle for effective time w.r.t input feed's availability be it RERUN or be it Update with effectiveTime. 4. Again this is no different from user rerunning an old instance. 5. Code handles by itself to run which instance with what all paths. On another note, get instance params for an instance would return the libpath(jars) being used as the oozie.libpath is being set to the workflow props. - sandeep ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51424/#review147106 ----------------------------------------------------------- On Oct. 19, 2016, 3:03 p.m., sandeep samudrala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51424/ > ----------------------------------------------------------- > > (Updated Oct. 19, 2016, 3:03 p.m.) > > > Review request for Falcon and Pallavi Rao. > > > Repository: falcon-git > > > Description > ------- > > Effective Time in Entity Update > > > Diffs > ----- > > cli/src/main/java/org/apache/falcon/cli/FalconCLI.java 0dd11f6 > cli/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java a8aea52 > client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java > 5d6eff5 > client/src/main/java/org/apache/falcon/client/FalconCLIConstants.java > 04f1599 > client/src/main/java/org/apache/falcon/client/FalconClient.java 8f77fad > common/src/main/java/org/apache/falcon/entity/ClusterHelper.java f89def3 > common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java > PRE-CREATION > common/src/main/java/org/apache/falcon/entity/EntityLibEntry.java > PRE-CREATION > common/src/main/java/org/apache/falcon/entity/EntityUtil.java 8fe316c > common/src/main/java/org/apache/falcon/entity/ProcessHelper.java e563d18 > > common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java > 38fa3ae > common/src/main/java/org/apache/falcon/update/UpdateHelper.java 266319f > > common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java > 16a1753 > common/src/test/java/org/apache/falcon/entity/EntityDictionaryUtilTest.java > PRE-CREATION > common/src/test/java/org/apache/falcon/update/UpdateHelperTest.java 826686f > docs/src/site/twiki/falconcli/Touch.twiki afbd848 > docs/src/site/twiki/falconcli/UpdateEntity.twiki 146a60f > oozie/src/main/java/org/apache/falcon/oozie/OozieBundleBuilder.java 5f93cc2 > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedBundleBuilder.java > c758411 > > oozie/src/main/java/org/apache/falcon/oozie/process/HiveProcessWorkflowBuilder.java > 9f9579c > > oozie/src/main/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilder.java > f93a599 > > oozie/src/main/java/org/apache/falcon/oozie/process/PigProcessWorkflowBuilder.java > a1a7c12 > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java > 6661dd5 > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionCoordinatorBuilder.java > 91f4757 > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionWorkflowBuilder.java > 20eeffd > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java > 394600c > > oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java > 05b513e > oozie/src/test/resources/config/process/dumb-hive-process.xml c504074 > oozie/src/test/resources/config/process/hive-process-FSInputFeed.xml > d871377 > oozie/src/test/resources/config/process/hive-process-FSOutputFeed.xml > 23d96c3 > oozie/src/test/resources/config/process/hive-process.xml 4dac8e9 > oozie/src/test/resources/config/process/pig-process-0.1.xml 8d20cee > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java > aefd699 > > prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java > 3bdeb99 > > prism/src/main/java/org/apache/falcon/resource/extensions/ExtensionManager.java > 92b5531 > > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java > 07334d6 > > scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java > 9ba62a1 > > shell/src/main/java/org/apache/falcon/shell/commands/FalconEntityCommands.java > 35a6f2a > src/build/checkstyle.xml 292a0a3 > unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 53073f0 > > unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java > 7398c8a > unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java bfc8b08 > unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java 0bc7755 > unit/src/test/resources/process.xml 6854311 > webapp/src/main/java/org/apache/falcon/resource/ConfigSyncService.java > 7b32bd5 > > webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java > 5525207 > webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseyIT.java > 876ada5 > > Diff: https://reviews.apache.org/r/51424/diff/ > > > Testing > ------- > > > Thanks, > > sandeep samudrala > >
