----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51424/#review147081 -----------------------------------------------------------
cli/src/main/java/org/apache/falcon/cli/FalconCLI.java (line 94) <https://reviews.apache.org/r/51424/#comment214091> Please also add updateClusterDependents, touch, dependencies, list, summary, and lookup operations also to the list for the sake of completeness. cli/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java (line 169) <https://reviews.apache.org/r/51424/#comment214092> description is incorrect, copy-paste error? common/pom.xml (line 246) <https://reviews.apache.org/r/51424/#comment214093> Can you please add the version to the parent-pom.xml? common/pom.xml (line 247) <https://reviews.apache.org/r/51424/#comment214097> Why is jython required? I couldn't find any usage in the patch. common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 98) <https://reviews.apache.org/r/51424/#comment214094> If it is specific to process only, then why not move it to ProcessHelper.java? common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 99) <https://reviews.apache.org/r/51424/#comment214095> Can you please add comments for all 3 variables explaining what is their purpose with example. common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 100) <https://reviews.apache.org/r/51424/#comment214096> common/src/main/java/org/apache/falcon/entity/ProcessHelper.java (line 96) <https://reviews.apache.org/r/51424/#comment214098> Please add javadoc for all methods. common/src/main/java/org/apache/falcon/entity/ProcessHelper.java (line 97) <https://reviews.apache.org/r/51424/#comment214100> common/src/main/java/org/apache/falcon/entity/ProcessHelper.java (line 98) <https://reviews.apache.org/r/51424/#comment214101> try-catch should be put on as tight scope as possible for better readability and maintenance. common/src/main/java/org/apache/falcon/entity/ProcessHelper.java (line 100) <https://reviews.apache.org/r/51424/#comment214099> Please use StringUtils.isBlank() common/src/main/java/org/apache/falcon/entity/ProcessHelper.java (line 252) <https://reviews.apache.org/r/51424/#comment214102> Same as above for try-catch scope. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 50) <https://reviews.apache.org/r/51424/#comment214103> Please explain in the comments what is Lib Dictionary. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 58) <https://reviews.apache.org/r/51424/#comment214162> Please add javadocs for all public methods. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 68) <https://reviews.apache.org/r/51424/#comment214106> Please use try-with resources. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 72) <https://reviews.apache.org/r/51424/#comment214107> Please add logging with appropriate context. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 81) <https://reviews.apache.org/r/51424/#comment214111> libCheckSumsMap will be a better name. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 89) <https://reviews.apache.org/r/51424/#comment214108> Same as above for try-with resources. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 94) <https://reviews.apache.org/r/51424/#comment214110> Please add logging. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 98) <https://reviews.apache.org/r/51424/#comment214112> Rename to createLibCheckSumsDictionary common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 99) <https://reviews.apache.org/r/51424/#comment214170> Input parameter should be Process instead of entity as it is only called for process and not other entities. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 110) <https://reviews.apache.org/r/51424/#comment214166> If the path doesn't exist then fs.exists will return false. How is it sure in case of IOException that the file doesn't exist? common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 113) <https://reviews.apache.org/r/51424/#comment214168> Staging path in context of Falcon has a special meaning, it will be better to reword the message and rename the variable to convey that it is first moved to a temporary location. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 115) <https://reviews.apache.org/r/51424/#comment214167> Unused variable md5 common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 119) <https://reviews.apache.org/r/51424/#comment214169> won't workflowLibPath convey the meaning better? common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 122) <https://reviews.apache.org/r/51424/#comment214171> It is never called for feed, so please remove the type check and the else if part. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 126) <https://reviews.apache.org/r/51424/#comment214172> Again inconsistent naming convention, the separator is called WF_LIB_SEPARATOR and path is called userDefinedLibPath. Please make everything consistent to follow workflow lib semantics common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 131) <https://reviews.apache.org/r/51424/#comment214174> If wf_lib path contains multiple directories with same jar, how does oozie handle it? Is our behavior consistent with that? common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 135) <https://reviews.apache.org/r/51424/#comment214175> The if statement can be simplified, by starting with an empty hash map instead of null. Here the empty check is redundant. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 144) <https://reviews.apache.org/r/51424/#comment214178> Can you please document it in detail in the user docs, JIRA and the design doc 1. how the jars are getting stored, which path, permissions of the directories and the jars etc. 2. how are the duplicates handled common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 148) <https://reviews.apache.org/r/51424/#comment214173> Please log the error message. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 155) <https://reviews.apache.org/r/51424/#comment214176> Please avoid using stagingPath in variable names as it has a special meaning in context of Falcon. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 159) <https://reviews.apache.org/r/51424/#comment214177> This check can be done upfront and we can avoid creating staging dir if jarPath doesn't exist. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 168) <https://reviews.apache.org/r/51424/#comment214179> Please add javadoc. common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java (line 195) <https://reviews.apache.org/r/51424/#comment214180> Name of the method says earliest, but it seems here that the larger modification time implies file is smaller and hence will appear first in the sorted list. common/src/main/java/org/apache/falcon/entity/v0/EntityLibDictionary.java (line 24) <https://reviews.apache.org/r/51424/#comment214105> Name is a misnomer as it is not a dictionary(set of key-value pairs), please change. Again, inconsistent naming between EntityLibDictionary and ProcessLibDictionary across the codebase. common/src/main/java/org/apache/falcon/entity/v0/EntityLibDictionary.java (line 30) <https://reviews.apache.org/r/51424/#comment214114> Rename parameter to libPathWithChecksum. common/src/main/java/org/apache/falcon/entity/v0/EntityLibDictionary.java (line 45) <https://reviews.apache.org/r/51424/#comment214115> setters are not being used anywhere. common/src/main/java/org/apache/falcon/entity/v0/EntityLibDictionary.java (line 53) <https://reviews.apache.org/r/51424/#comment214116> common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java (line 140) <https://reviews.apache.org/r/51424/#comment214104> This doesn't compile! oozie/src/main/java/org/apache/falcon/oozie/OozieBundleBuilder.java <https://reviews.apache.org/r/51424/#comment214184> Intuitively, it doesn't make sense to move this code to its parent class OozieEntityBuilder as we are changing the libpath only for process. oozie/src/main/java/org/apache/falcon/oozie/OozieEntityBuilder.java (line 302) <https://reviews.apache.org/r/51424/#comment214182> Better will be to make a new function called getProcessLibPath and leave methods for other entities unchanged. oozie/src/main/java/org/apache/falcon/oozie/feed/FeedBundleBuilder.java (line 85) <https://reviews.apache.org/r/51424/#comment214181> getLibPath doesn't require cluster for anything except process, so it will make the code much more readable and maintainable if we could keep this existing definition as it is(may need to remove @override). oozie/src/main/java/org/apache/falcon/oozie/feed/FeedReplicationWorkflowBuilder.java (line 82) <https://reviews.apache.org/r/51424/#comment214185> Since there is no change for feeds, these files shouldn't be changed. If a different method signature is required for process, then only in builders for process change should be reflected, probably by overloading / providing a new specific method for process, along with a comment on why this special handling is required. oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java (line 114) <https://reviews.apache.org/r/51424/#comment214109> remove dead code. prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java (line 359) <https://reviews.apache.org/r/51424/#comment214188> It will be nice to check it at the start of the update API method. This will ensure that check is done for all code branches without repeating it again and again. prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java (line 1481) <https://reviews.apache.org/r/51424/#comment214117> The if part of the if-else is not required. prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java (line 313) <https://reviews.apache.org/r/51424/#comment214113> Looks like the code allows for effective time to be passed for all types of entities (including clusters as per recent cluster update feature). scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java (line 577) <https://reviews.apache.org/r/51424/#comment214186> Why this change is required? scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java (line 582) <https://reviews.apache.org/r/51424/#comment214187> Should be UnsupportedOperationException. src/build/checkstyle.xml (line 122) <https://reviews.apache.org/r/51424/#comment214189> Let's keep it at default value of 150. It is good to ensure readability and maintenance. - Praveen Adlakha 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 > >
