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 51424: Effective Time in Entity Update
Date Wed, 07 Sep 2016 06:51:59 GMT

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


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