falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Praveen Adlakha <adlakha.prav...@gmail.com>
Subject Re: Review Request 51424: Effective Time in Entity Update
Date Sun, 28 Aug 2016 18:25:23 GMT

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


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