falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pallavi Rao <pallavi....@inmobi.com>
Subject Re: Review Request 43829: Effective time in Entity updates.
Date Tue, 08 Mar 2016 10:13:51 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message