You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Pallavi Rao <pa...@inmobi.com> on 2016/11/03 06:05:36 UTC
Re: Review Request 51424: Effective Time in Entity Update
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51424/#review154674
-----------------------------------------------------------
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 545)
<https://reviews.apache.org/r/51424/#comment224300>
EffectiveTime query parameter is not added.
client/src/main/java/org/apache/falcon/client/FalconClient.java (line 926)
<https://reviews.apache.org/r/51424/#comment224301>
Should be StringUtils.isBlank(type). Else, it will always throw an exception.
common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 1207)
<https://reviews.apache.org/r/51424/#comment224307>
Nit : Typo in method name. HasCode should be HashCode
common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 1209)
<https://reviews.apache.org/r/51424/#comment224308>
Out of curiosity. Why hashCode and not directly use checksum?
oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java (line 1473)
<https://reviews.apache.org/r/51424/#comment224319>
How is effectiveTime in the future handled?
prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java (line 316)
<https://reviews.apache.org/r/51424/#comment224331>
equalsIgnoreCase?
webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java (line 346)
<https://reviews.apache.org/r/51424/#comment224332>
Why effectiveTime as a dimension?
common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java (line 201)
<https://reviews.apache.org/r/51424/#comment224302>
Why does a method variable need to be concurrent?
common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java (line 211)
<https://reviews.apache.org/r/51424/#comment224303>
Too much nesting. Hard to read. Can move the runnable to an inner class?
common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java (line 227)
<https://reviews.apache.org/r/51424/#comment224305>
Nit: May be use debug level. Will help in figuring which libs were copied
common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java (line 237)
<https://reviews.apache.org/r/51424/#comment224304>
As HDFS latencies are not predictable, either make the five minute configurable OR await without timeout.
common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java (line 294)
<https://reviews.apache.org/r/51424/#comment224306>
Shouldn't it be an exception if the base staging path itself is missing?
docs/src/site/twiki/restapi/EntityUpdate.twiki (line 15)
<https://reviews.apache.org/r/51424/#comment224312>
Might want to add some additional documentation on the behavior. What happens to overlapping instances etc.
prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java (line 370)
<https://reviews.apache.org/r/51424/#comment224329>
equalsIgnoreCase?
- Pallavi Rao
On Oct. 19, 2016, 6:43 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, 6:43 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
> docs/src/site/twiki/restapi/EntityUpdate.twiki cbf33db
> 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
>
>
Re: Review Request 51424: Effective Time in Entity Update
Posted by sandeep samudrala <sa...@gmail.com>.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 545
> > <https://reviews.apache.org/r/51424/diff/1-3/?file=1485665#file1485665line545>
> >
> > EffectiveTime query parameter is not added.
Added the query param.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > client/src/main/java/org/apache/falcon/client/FalconClient.java, line 926
> > <https://reviews.apache.org/r/51424/diff/1-3/?file=1485665#file1485665line926>
> >
> > Should be StringUtils.isBlank(type). Else, it will always throw an exception.
Corrected it.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityUtil.java, line 1211
> > <https://reviews.apache.org/r/51424/diff/1-3/?file=1485667#file1485667line1211>
> >
> > Nit : Typo in method name. HasCode should be HashCode
Changed it.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java, line 316
> > <https://reviews.apache.org/r/51424/diff/1-3/?file=1485692#file1485692line316>
> >
> > equalsIgnoreCase?
Fixed it.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java, line 201
> > <https://reviews.apache.org/r/51424/diff/3/?file=1541713#file1541713line201>
> >
> > Why does a method variable need to be concurrent?
Not required. It was part of very early changes. Changed it.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java, line 227
> > <https://reviews.apache.org/r/51424/diff/3/?file=1541713#file1541713line227>
> >
> > Nit: May be use debug level. Will help in figuring which libs were copied
Changed it.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java, line 237
> > <https://reviews.apache.org/r/51424/diff/3/?file=1541713#file1541713line237>
> >
> > As HDFS latencies are not predictable, either make the five minute configurable OR await without timeout.
Made it configurable.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java, line 370
> > <https://reviews.apache.org/r/51424/diff/3/?file=1541741#file1541741line370>
> >
> > equalsIgnoreCase?
Corrected it.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java, line 294
> > <https://reviews.apache.org/r/51424/diff/3/?file=1541713#file1541713line294>
> >
> > Shouldn't it be an exception if the base staging path itself is missing?
Removing the check altogether as the paths created in prepareEntityBuildPath().
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > docs/src/site/twiki/restapi/EntityUpdate.twiki, line 15
> > <https://reviews.apache.org/r/51424/diff/3/?file=1541724#file1541724line15>
> >
> > Might want to add some additional documentation on the behavior. What happens to overlapping instances etc.
Done. Updated it.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityDictionaryUtil.java, line 211
> > <https://reviews.apache.org/r/51424/diff/3/?file=1541713#file1541713line211>
> >
> > Too much nesting. Hard to read. Can move the runnable to an inner class?
Moved it to inner class.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityUtil.java, line 1213
> > <https://reviews.apache.org/r/51424/diff/1-3/?file=1485667#file1485667line1213>
> >
> > Out of curiosity. Why hashCode and not directly use checksum?
Its upto the filesystem implementation to use the checksum.tostring, which doesn't qualify for comparision in this case .
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java, line 346
> > <https://reviews.apache.org/r/51424/diff/1-3/?file=1485700#file1485700line346>
> >
> > Why effectiveTime as a dimension?
Not required. Removing it.
> On Nov. 3, 2016, 6:05 a.m., Pallavi Rao wrote:
> > oozie/src/main/java/org/apache/falcon/workflow/engine/OozieWorkflowEngine.java, line 1487
> > <https://reviews.apache.org/r/51424/diff/1-3/?file=1485688#file1485688line1487>
> >
> > How is effectiveTime in the future handled?
Made changes to handle the same.
- sandeep
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51424/#review154674
-----------------------------------------------------------
On Nov. 11, 2016, 4:38 a.m., sandeep samudrala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51424/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2016, 4:38 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/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
> docs/src/site/twiki/restapi/EntityUpdate.twiki cbf33db
> 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/oozie/process/SparkProcessWorkflowBuilder.java 51db75d
> 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
> src/conf/startup.properties 6d82516
> 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/cli/FalconCLIIT.java 5cdbf93
> webapp/src/test/java/org/apache/falcon/resource/EntityManagerJerseyIT.java 876ada5
>
> Diff: https://reviews.apache.org/r/51424/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> sandeep samudrala
>
>