falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sandeep samudrala <sandys...@gmail.com>
Subject Re: Review Request 51424: Effective Time in Entity Update
Date Wed, 19 Oct 2016 15:08:52 GMT


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > cli/src/main/java/org/apache/falcon/cli/FalconEntityCLI.java, line 305
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485662#file1485662line305>
> >
> >     Also add parseDateString(effectiveTime), to ensure the date is of valid format

Added.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, line
117
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line117>
> >
> >     We are assuming /tmp exists. Can you use cluster's "temp" location instead?

Ack. Changed to cluster temp location


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, line
139
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line139>
> >
> >     This creates new jars. How about updated jars? We are not using the checksum
in the dictionary?

Modified to check the checksum of any existing jar while checking for the jar entry.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, line
147
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line147>
> >
> >     Should we also write the dictionary file in this method itself?

Yes. Moved the writing of the file to this method.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, line
151
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line151>
> >
> >     How will the invoker of this method interpret null?

This method is now modified to just return the map of entries.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, line
168
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line168>
> >
> >     Should it be "getMostRecentDictionaryPath"? Method name is a little confusing.

Actually the comparator is written wrongly. Corrected it. 
I am comparing it with the earliest Dictionary than recent Dictionary


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/update/UpdateHelper.java, line 238
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485671#file1485671line238>
> >
> >     We are checking for checksum of only workflow.xml and not jars? Shouldn't be
iterating over the checksums variable?

Only worfklow is being tracked here. Jars have changed or not , the update will continue with
new coordinator from the effective time but with almost the same entry as the previous entry.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/update/UpdateHelper.java, line 250
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485671#file1485671line250>
> >
> >     Log and throw.

Ack. Logged


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java,
line 119
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485673#file1485673line119>
> >
> >     Why does this need to be part of AbstractWorkflowEngine. Can't it be private
method?

Kept it so that the same method can be implemented falcon workflow engine too when required.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java, line
113
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485686#file1485686line113>
> >
> >     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.

Made the changes accordingly to call write method inside createLibDictionary.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessBundleBuilder.java, line
123
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485686#file1485686line123>
> >
> >     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.

Made the changes accordingly


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java,
line 1909
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485688#file1485688line1909>
> >
> >     Should we also ensure the retry doesn't kick in for these instances? Or, is
it inherently taken care of?

Added ignoreInstances in the next line. Had missed it.
Taking care of it by ignoring these instances, where falcon retry doesnt retry on a instance
that is ignored.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > scheduler/src/main/java/org/apache/falcon/workflow/engine/FalconWorkflowEngine.java,
line 581
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485693#file1485693line581>
> >
> >     As mentioned above, please make this private method. This need not be exposed
as a public method.

Ack. Removed it from FalconWorkflowEngine


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionWorkflowBuilder.java,
line 131
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485687#file1485687line131>
> >
> >     IOException does not mean file not found.

Modified the error code and throwing out the exception now.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, line
76
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line76>
> >
> >     Nit: Better to keep the method name and parameter names of read/write methods
similar, for readabiltiy sake.

renamed write method in par with read method.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, line
66
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line66>
> >
> >     EntityLibDictionary does not have a toString() method. You might want to verify
if the serialization/deserialization is happening correctly.

Fixed it. Implemented toString to EntityLibEntry(EntityLibDictionary).


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/v0/EntityDictionaryUtil.java, line
63
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485669#file1485669line63>
> >
> >     Should we also set the perms appropriately?

Have added the permissions in accordance.


> On Sept. 7, 2016, 6:51 a.m., Pallavi Rao wrote:
> > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java,
line 1300
> > <https://reviews.apache.org/r/51424/diff/1/?file=1485688#file1485688line1300>
> >
> >     Cleaner to do the isWfUpdated check inside this method.

Ack. moved isWfUpdaated to canUpdateBundle.


- sandeep


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51424/#review147551
-----------------------------------------------------------


On Oct. 19, 2016, 3:03 p.m., sandeep samudrala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51424/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2016, 3:03 p.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/src/main/java/org/apache/falcon/entity/ClusterHelper.java f89def3 
>   common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java PRE-CREATION

>   common/src/main/java/org/apache/falcon/entity/EntityLibEntry.java PRE-CREATION 
>   common/src/main/java/org/apache/falcon/entity/EntityUtil.java 8fe316c 
>   common/src/main/java/org/apache/falcon/entity/ProcessHelper.java e563d18 
>   common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 38fa3ae

>   common/src/main/java/org/apache/falcon/update/UpdateHelper.java 266319f 
>   common/src/main/java/org/apache/falcon/workflow/engine/AbstractWorkflowEngine.java
16a1753 
>   common/src/test/java/org/apache/falcon/entity/EntityDictionaryUtilTest.java PRE-CREATION

>   common/src/test/java/org/apache/falcon/update/UpdateHelperTest.java 826686f 
>   docs/src/site/twiki/falconcli/Touch.twiki afbd848 
>   docs/src/site/twiki/falconcli/UpdateEntity.twiki 146a60f 
>   oozie/src/main/java/org/apache/falcon/oozie/OozieBundleBuilder.java 5f93cc2 
>   oozie/src/main/java/org/apache/falcon/oozie/feed/FeedBundleBuilder.java c758411 
>   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/ProcessExecutionCoordinatorBuilder.java
91f4757 
>   oozie/src/main/java/org/apache/falcon/oozie/process/ProcessExecutionWorkflowBuilder.java
20eeffd 
>   oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java 394600c

>   oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java
05b513e 
>   oozie/src/test/resources/config/process/dumb-hive-process.xml c504074 
>   oozie/src/test/resources/config/process/hive-process-FSInputFeed.xml d871377 
>   oozie/src/test/resources/config/process/hive-process-FSOutputFeed.xml 23d96c3 
>   oozie/src/test/resources/config/process/hive-process.xml 4dac8e9 
>   oozie/src/test/resources/config/process/pig-process-0.1.xml 8d20cee 
>   prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java aefd699 
>   prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java
3bdeb99 
>   prism/src/main/java/org/apache/falcon/resource/extensions/ExtensionManager.java 92b5531

>   prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java
07334d6 
>   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/FalconUnitTestBase.java bfc8b08 
>   unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java 0bc7755 
>   unit/src/test/resources/process.xml 6854311 
>   webapp/src/main/java/org/apache/falcon/resource/ConfigSyncService.java 7b32bd5 
>   webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java 5525207

>   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