----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43829/#review122285 -----------------------------------------------------------
client/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java (line 47) <https://reviews.apache.org/r/43829/#comment184239> use effectiveTime please. More user-friendly client/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java (line 126) <https://reviews.apache.org/r/43829/#comment184240> Can we re-phrase this - "Applies changes from the given effective time" client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java (line 105) <https://reviews.apache.org/r/43829/#comment184241> Update Java doc too. client/src/main/java/org/apache/falcon/client/FalconClient.java (line 401) <https://reviews.apache.org/r/43829/#comment184242> Where is this web resource used? common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java (line 263) <https://reviews.apache.org/r/43829/#comment184243> EntityUtil.getEntityProperties is already doing this. Don't think this method is needed. common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 97) <https://reviews.apache.org/r/43829/#comment184515> Would be much cleaner if we had the checksum in a separate class with serialize and deserialize methods. common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 98) <https://reviews.apache.org/r/43829/#comment184508> Nit : A utility method. The arg name can just be path. common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 106) <https://reviews.apache.org/r/43829/#comment184509> Add array size check just to be safe. common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 145) <https://reviews.apache.org/r/43829/#comment184511> Why are there 2 invocations of copy? common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 165) <https://reviews.apache.org/r/43829/#comment184510> Why ConcurrentHashMap? common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 189) <https://reviews.apache.org/r/43829/#comment184513> Why ConcurrentHashMap? common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 190) <https://reviews.apache.org/r/43829/#comment184514> Should we create the dest directory when doCopy is false? common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 223) <https://reviews.apache.org/r/43829/#comment184530> How are we doing the error handling : Lets say I copied over some files and there was IOException. There will be a staging dir that is only partially populated. Should we copy the files instead to a tmp dir and then do a move, so the staging directory is clean? common/src/main/java/org/apache/falcon/update/UpdateHelper.java (line 228) <https://reviews.apache.org/r/43829/#comment184250> You can use EntityUtil.getLastestStagingPath or modify it. common/src/main/java/org/apache/falcon/workflow/WorkflowJobEndNotificationService.java (line 246) <https://reviews.apache.org/r/43829/#comment184246> This change is already in trunk common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java (line 113) <https://reviews.apache.org/r/43829/#comment184247> Why does this need to be part of AbstractWorkflowEngine? It is just internal detail for the update API. oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java (line 135) <https://reviews.apache.org/r/43829/#comment184537> As mentioned before, will be much cleaner if you had a POJO for dictionary and serialize and deserialize methods against it. oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java (line 624) <https://reviews.apache.org/r/43829/#comment184540> private method please. - Pallavi Rao On Feb. 23, 2016, 12:43 p.m., sandeep samudrala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43829/ > ----------------------------------------------------------- > > (Updated Feb. 23, 2016, 12:43 p.m.) > > > Review request for Falcon. > > > Bugs: FALCON-1406 > https://issues.apache.org/jira/browse/FALCON-1406 > > > Repository: falcon-git > > > Description > ------- > > FALCON-1406. Initial patch for effective time in EntityMutations > > > Diffs > ----- > > client/src/main/java/org/apache/falcon/cli/FalconCLI.java a1f42ce > client/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java 6f9d620 > client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java > 4f86d9b > client/src/main/java/org/apache/falcon/client/FalconClient.java 597f608 > common/src/main/java/org/apache/falcon/entity/EntityUtil.java 96befa1 > common/src/main/java/org/apache/falcon/entity/FileSystemStorage.java > ece8b5d > common/src/main/java/org/apache/falcon/entity/ProcessHelper.java bbfca68 > common/src/main/java/org/apache/falcon/update/UpdateHelper.java 6603bc6 > common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionArgs.java > 3363e1f > > common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java > 5866369 > > common/src/main/java/org/apache/falcon/workflow/WorkflowJobEndNotificationService.java > faea25c > > common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java > b899a58 > oozie/src/main/java/org/apache/falcon/oozie/ExportWorkflowBuilder.java > a55656c > oozie/src/main/java/org/apache/falcon/oozie/ImportWorkflowBuilder.java > cae8497 > 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 a36ee79 > > oozie/src/main/java/org/apache/falcon/oozie/OozieOrchestrationWorkflowBuilder.java > e137e11 > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedBundleBuilder.java > c758411 > > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedReplicationWorkflowBuilder.java > 5a62130 > > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedRetentionWorkflowBuilder.java > b9e3848 > > 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 > 806810e > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionCoordinatorBuilder.java > f5c9948 > > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionWorkflowBuilder.java > 7d5b331 > > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java > ebf23da > > oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java > 8d824ba > prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java > 3ebe612 > prism/src/main/java/org/apache/falcon/resource/AbstractInstanceManager.java > b35ae9a > > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java > 2dc727d > rerun/src/main/java/org/apache/falcon/rerun/handler/RetryHandler.java > fac32b3 > rerun/src/main/java/org/apache/falcon/rerun/queue/ActiveMQueue.java 3168c31 > > scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java > 7ce2420 > unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 37221f3 > > unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java > 7398c8a > unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java aaf2b37 > webapp/src/main/java/org/apache/falcon/resource/ConfigSyncService.java > aa15dcc > > webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java > e97adff > webapp/src/test/java/org/apache/falcon/cli/FalconCLIIT.java a1668c1 > webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseyIT.java > f336422 > > Diff: https://reviews.apache.org/r/43829/diff/ > > > Testing > ------- > > Bit of manual testing done. I will further beautify the code along with few > more UTS(which I am fixing currently). Till then consider this for functional > review. > > > Thanks, > > sandeep samudrala > >
