----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51424/#review147551 -----------------------------------------------------------
Adding to Praveen's overall comments: With Spark support being added and Spark related libs being added differently (via spark-attributes), extra work might be required to support effective time for spark engine. It is alright to park a JIRA to enhance this feature for Spark. But, appropriate checks and error messages must be added. cli/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java (line 303) <https://reviews.apache.org/r/51424/#comment214736> Also add parseDateString(effectiveTime), to ensure the date is of valid format common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 63) <https://reviews.apache.org/r/51424/#comment214861> Should we also set the perms appropriately? common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 66) <https://reviews.apache.org/r/51424/#comment215283> EntityLibDictionary does not have a toString() method. You might want to verify if the serialization/deserialization is happening correctly. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 76) <https://reviews.apache.org/r/51424/#comment214860> Nit: Better to keep the method name and parameter names of read/write methods similar, for readabiltiy sake. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 117) <https://reviews.apache.org/r/51424/#comment215284> We are assuming /tmp exists. Can you use cluster's "temp" location instead? common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 139) <https://reviews.apache.org/r/51424/#comment215285> This creates new jars. How about updated jars? We are not using the checksum in the dictionary? common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 147) <https://reviews.apache.org/r/51424/#comment215294> Should we also write the dictionary file in this method itself? common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 151) <https://reviews.apache.org/r/51424/#comment215286> How will the invoker of this method interpret null? common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 168) <https://reviews.apache.org/r/51424/#comment215288> Should it be "getMostRecentDictionaryPath"? Method name is a little confusing. common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 238) <https://reviews.apache.org/r/51424/#comment215290> We are checking for checksum of only workflow.xml and not jars? Shouldn't be iterating over the checksums variable? common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 250) <https://reviews.apache.org/r/51424/#comment215291> Log and throw. common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java (line 119) <https://reviews.apache.org/r/51424/#comment215292> Why does this need to be part of AbstractWorkflowEngine. Can't it be private method? oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java (line 113) <https://reviews.apache.org/r/51424/#comment215297> Not a good idea to pass variables that get modified by methods. If you pull writeLibDict into createLibDictionary, you can read it back. Better would be for createLibDictionary to return the dictionary and take in the final path. oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java (line 123) <https://reviews.apache.org/r/51424/#comment215295> As mentioned in another comment, may be createLibDictionary should write the dictionary file too, rather than doing it as a separate step. Then, jars and the dictionary file are created together. oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionWorkflowBuilder.java (line 129) <https://reviews.apache.org/r/51424/#comment215298> IOException does not mean file not found. oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java (line 1300) <https://reviews.apache.org/r/51424/#comment215299> Cleaner to do the isWfUpdated check inside this method. oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java (line 1905) <https://reviews.apache.org/r/51424/#comment215300> Should we also ensure the retry doesn't kick in for these instances? Or, is it inherently taken care of? scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java (line 581) <https://reviews.apache.org/r/51424/#comment215301> As mentioned above, please make this private method. This need not be exposed as a public method. - Pallavi Rao On Aug. 25, 2016, 9:20 a.m., sandeep samudrala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51424/ > ----------------------------------------------------------- > > (Updated Aug. 25, 2016, 9:20 a.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/pom.xml 96cb7f5 > common/src/main/java/org/apache/falcon/entity/EntityUtil.java aef1fd5 > common/src/main/java/org/apache/falcon/entity/ProcessHelper.java e563d18 > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java > PRE-CREATION > common/src/main/java/org/apache/falcon/entity/v0/EntityLibDictionary.java > PRE-CREATION > common/src/main/java/org/apache/falcon/update/UpdateHelper.java 266319f > > common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java > cccbe3b > > common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java > 16a1753 > oozie/src/main/java/org/apache/falcon/oozie/ExportWorkflowBuilder.java > af7431a > oozie/src/main/java/org/apache/falcon/oozie/ImportWorkflowBuilder.java > 2d93189 > oozie/src/main/java/org/apache/falcon/oozie/OozieBundleBuilder.java 5f93cc2 > oozie/src/main/java/org/apache/falcon/oozie/OozieCoordinatorBuilder.java > f555b64 > oozie/src/main/java/org/apache/falcon/oozie/OozieEntityBuilder.java a856f8a > > oozie/src/main/java/org/apache/falcon/oozie/OozieOrchestrationWorkflowBuilder.java > 8d45d7a > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedBundleBuilder.java > c758411 > > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedReplicationWorkflowBuilder.java > db647aa > > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedRetentionWorkflowBuilder.java > fd51ed0 > > 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/ProcessExecutionWorkflowBuilder.java > 20eeffd > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java > c371d69 > > oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java > 05b513e > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java > aefd699 > > prism/src/main/java/org/apache/falcon/resource/extensions/ExtensionManager.java > 92b5531 > > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java > 249c273 > > 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/TestFalconUnit.java 0bc7755 > webapp/src/main/java/org/apache/falcon/resource/ConfigSyncService.java > 7b32bd5 > > webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java > 657ef9e > webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseyIT.java > 876ada5 > > Diff: https://reviews.apache.org/r/51424/diff/ > > > Testing > ------- > > > Thanks, > > sandeep samudrala > >
