You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Venkatesan Ramachandran <me...@gmail.com> on 2015/10/06 03:15:42 UTC
Re: Review Request 33443: FALCON-1102: Gather data transfer detail of
replication job submitted from HDFS recipe
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33443/#review101543
-----------------------------------------------------------
common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java (line 411)
<https://reviews.apache.org/r/33443/#comment159007>
Quation: should addCounterToWF() be called only the counters are enabled in the Feed?
common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java (line 434)
<https://reviews.apache.org/r/33443/#comment159005>
Question: Do we need to Fail the workflow if the counters file is not found?
metrics/pom.xml (line 35)
<https://reviews.apache.org/r/33443/#comment158999>
GENERAL: Have you tested this on Secure Cluster?
metrics/src/main/java/org/apache/falcon/job/JobCounters.java (line 73)
<https://reviews.apache.org/r/33443/#comment158998>
is there a specific reason for not using the following to create FileSystem?
HadoopClientFactory.get().createProxiedFileSystem()
This one handles authentication of proxy users across the system.
metrics/src/main/java/org/apache/falcon/job/JobCountersHandler.java (line 33)
<https://reviews.apache.org/r/33443/#comment158969>
Instead of returning null, consider throwing an exception (NotSupported or IllegalArgument) so it will be easier to use this class and debug later.
metrics/src/main/java/org/apache/falcon/job/ReplicationJobCountersList.java (line 27)
<https://reviews.apache.org/r/33443/#comment158977>
is it possible for the DistCP/Falcon Replication job to succeed, but still skip files/bytes? Should we grab the other metrics like
CopyMapper.Counter.SKIP
CopyMapper.Counter.BYTESSKIPPED
FAIL (CopyMapper.Counter.FAIL, CopyMapper.Counter.BYTESFAILED) should fail the DistCp/Replication Job and so no need to get them.
metrics/src/main/java/org/apache/falcon/job/ReplicationJobCountersList.java (line 48)
<https://reviews.apache.org/r/33443/#comment159000>
any specific reason not using ReplicationJobCountersList.valueOf()?
oozie/src/main/java/org/apache/falcon/oozie/feed/FeedReplicationWorkflowBuilder.java (line 62)
<https://reviews.apache.org/r/33443/#comment159001>
nit: write defensive?
"true".equalIgnoreCase(props.getValue()).
so if the value is null for what ever reason we don't NPE.
- Venkatesan Ramachandran
On Sept. 29, 2015, 10:13 a.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33443/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 10:13 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1102
> https://issues.apache.org/jira/browse/FALCON-1102
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> FALCON-1102: Gather data transfer detail of replication job submitted from HDFS recipe
>
>
> Diffs
> -----
>
> addons/recipes/hdfs-replication/src/main/resources/hdfs-replication-workflow.xml 942421f
> common/src/main/java/org/apache/falcon/metadata/InstanceRelationshipGraphBuilder.java 016c622
> common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionArgs.java 9456fb9
> common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java 4454239
> common/src/test/java/org/apache/falcon/metadata/MetadataMappingServiceTest.java 89e8178
> metrics/pom.xml a0358db
> metrics/src/main/java/org/apache/falcon/job/FSReplicationCounters.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobCounters.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobCountersHandler.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobType.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/ReplicationJobCountersList.java PRE-CREATION
> metrics/src/test/java/org/apache/falcon/job/FSReplicationCountersTest.java PRE-CREATION
> oozie/src/main/java/org/apache/falcon/oozie/feed/FSReplicationWorkflowBuilder.java b82f4e0
> oozie/src/main/java/org/apache/falcon/oozie/feed/FeedReplicationWorkflowBuilder.java a7c19cd
> oozie/src/test/java/org/apache/falcon/oozie/feed/OozieFeedWorkflowBuilderTest.java cfce1ae
> oozie/src/test/resources/feed/fs-replication-feed-counters.xml PRE-CREATION
> replication/pom.xml 3cc96fc
> replication/src/main/java/org/apache/falcon/replication/FeedReplicator.java a226058
>
> Diff: https://reviews.apache.org/r/33443/diff/
>
>
> Testing
> -------
>
> Yes. Unit test cases added.
>
>
> Thanks,
>
> Peeyush Bishnoi
>
>
Re: Review Request 33443: FALCON-1102: Gather data transfer detail of
replication job submitted from HDFS recipe
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> > common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java, line 416
> > <https://reviews.apache.org/r/33443/diff/1-4/?file=939668#file939668line416>
> >
> > Quation: should addCounterToWF() be called only the counters are enabled in the Feed?
Ideally addCounterToWF() should be called when counter property "job.counter" are enabled in the feed replication. But here we require addCounterToWF() to be called from recipes as well which is the process entity. I have already put a check in function addCounterToWF() for checking counter file to add counters to WorkflowExecutionArgs.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> > metrics/src/main/java/org/apache/falcon/job/JobCounters.java, line 73
> > <https://reviews.apache.org/r/33443/diff/4/?file=1086588#file1086588line73>
> >
> > is there a specific reason for not using the following to create FileSystem?
> >
> >
> > HadoopClientFactory.get().createProxiedFileSystem()
> >
> > This one handles authentication of proxy users across the system.
Earlier I have used this in differenct function, but as per the earlier review comments I have not used this again. But it looks much simpler to get FS access from conf.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> > metrics/src/main/java/org/apache/falcon/job/JobCountersHandler.java, line 33
> > <https://reviews.apache.org/r/33443/diff/4/?file=1086589#file1086589line33>
> >
> > Instead of returning null, consider throwing an exception (NotSupported or IllegalArgument) so it will be easier to use this class and debug later.
Fixed. Throwing an exception will fail the workflow and hence it can impact execution. I have logged that passed job type is not supported.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> > metrics/src/main/java/org/apache/falcon/job/ReplicationJobCountersList.java, line 48
> > <https://reviews.apache.org/r/33443/diff/4/?file=1086591#file1086591line48>
> >
> > any specific reason not using ReplicationJobCountersList.valueOf()?
I have thought of this earlier as well but valueOf() throws exception if required enum is not defined.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> > oozie/src/main/java/org/apache/falcon/oozie/feed/FeedReplicationWorkflowBuilder.java, line 62
> > <https://reviews.apache.org/r/33443/diff/4/?file=1086594#file1086594line62>
> >
> > nit: write defensive?
> >
> > "true".equalIgnoreCase(props.getValue()).
> >
> > so if the value is null for what ever reason we don't NPE.
Fixed. But I did not put it earlier as this makes code harder to read.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> > metrics/src/main/java/org/apache/falcon/job/ReplicationJobCountersList.java, line 27
> > <https://reviews.apache.org/r/33443/diff/4/?file=1086591#file1086591line27>
> >
> > is it possible for the DistCP/Falcon Replication job to succeed, but still skip files/bytes? Should we grab the other metrics like
> >
> > CopyMapper.Counter.SKIP
> > CopyMapper.Counter.BYTESSKIPPED
> >
> > FAIL (CopyMapper.Counter.FAIL, CopyMapper.Counter.BYTESFAILED) should fail the DistCp/Replication Job and so no need to get them.
Yes I have seen the scenario where distcp/falcon replication job succeeded and data skipped. In that case COPY and BYTESCOPIED will be 0 to signify that for the succeeded replication job, data do not copy. If later more strong requirement will come, will consider this metrics.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> > metrics/pom.xml, line 35
> > <https://reviews.apache.org/r/33443/diff/4/?file=1086586#file1086586line35>
> >
> > GENERAL: Have you tested this on Secure Cluster?
I have not tested this specifically on secure cluster. But I am hoping this will run. Also will take care of this in separate jira if issue appears.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> > common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java, line 439
> > <https://reviews.apache.org/r/33443/diff/1-4/?file=939668#file939668line439>
> >
> > Question: Do we need to Fail the workflow if the counters file is not found?
Good catch. Fixed.
- Peeyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33443/#review101543
-----------------------------------------------------------
On Sept. 29, 2015, 10:13 a.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33443/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 10:13 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1102
> https://issues.apache.org/jira/browse/FALCON-1102
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> FALCON-1102: Gather data transfer detail of replication job submitted from HDFS recipe
>
>
> Diffs
> -----
>
> addons/recipes/hdfs-replication/src/main/resources/hdfs-replication-workflow.xml 942421f
> common/src/main/java/org/apache/falcon/metadata/InstanceRelationshipGraphBuilder.java 016c622
> common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionArgs.java 9456fb9
> common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java 4454239
> common/src/test/java/org/apache/falcon/metadata/MetadataMappingServiceTest.java 89e8178
> metrics/pom.xml a0358db
> metrics/src/main/java/org/apache/falcon/job/FSReplicationCounters.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobCounters.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobCountersHandler.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobType.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/ReplicationJobCountersList.java PRE-CREATION
> metrics/src/test/java/org/apache/falcon/job/FSReplicationCountersTest.java PRE-CREATION
> oozie/src/main/java/org/apache/falcon/oozie/feed/FSReplicationWorkflowBuilder.java b82f4e0
> oozie/src/main/java/org/apache/falcon/oozie/feed/FeedReplicationWorkflowBuilder.java a7c19cd
> oozie/src/test/java/org/apache/falcon/oozie/feed/OozieFeedWorkflowBuilderTest.java cfce1ae
> oozie/src/test/resources/feed/fs-replication-feed-counters.xml PRE-CREATION
> replication/pom.xml 3cc96fc
> replication/src/main/java/org/apache/falcon/replication/FeedReplicator.java a226058
>
> Diff: https://reviews.apache.org/r/33443/diff/
>
>
> Testing
> -------
>
> Yes. Unit test cases added.
>
>
> Thanks,
>
> Peeyush Bishnoi
>
>
Re: Review Request 33443: FALCON-1102: Gather data transfer detail of
replication job submitted from HDFS recipe
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
> On Oct. 6, 2015, 1:15 a.m., Venkatesan Ramachandran wrote:
> >
Thanks Venky for reviewing the patch.
- Peeyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33443/#review101543
-----------------------------------------------------------
On Sept. 29, 2015, 10:13 a.m., Peeyush Bishnoi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33443/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2015, 10:13 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1102
> https://issues.apache.org/jira/browse/FALCON-1102
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> FALCON-1102: Gather data transfer detail of replication job submitted from HDFS recipe
>
>
> Diffs
> -----
>
> addons/recipes/hdfs-replication/src/main/resources/hdfs-replication-workflow.xml 942421f
> common/src/main/java/org/apache/falcon/metadata/InstanceRelationshipGraphBuilder.java 016c622
> common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionArgs.java 9456fb9
> common/src/main/java/org/apache/falcon/workflow/WorkflowExecutionContext.java 4454239
> common/src/test/java/org/apache/falcon/metadata/MetadataMappingServiceTest.java 89e8178
> metrics/pom.xml a0358db
> metrics/src/main/java/org/apache/falcon/job/FSReplicationCounters.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobCounters.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobCountersHandler.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/JobType.java PRE-CREATION
> metrics/src/main/java/org/apache/falcon/job/ReplicationJobCountersList.java PRE-CREATION
> metrics/src/test/java/org/apache/falcon/job/FSReplicationCountersTest.java PRE-CREATION
> oozie/src/main/java/org/apache/falcon/oozie/feed/FSReplicationWorkflowBuilder.java b82f4e0
> oozie/src/main/java/org/apache/falcon/oozie/feed/FeedReplicationWorkflowBuilder.java a7c19cd
> oozie/src/test/java/org/apache/falcon/oozie/feed/OozieFeedWorkflowBuilderTest.java cfce1ae
> oozie/src/test/resources/feed/fs-replication-feed-counters.xml PRE-CREATION
> replication/pom.xml 3cc96fc
> replication/src/main/java/org/apache/falcon/replication/FeedReplicator.java a226058
>
> Diff: https://reviews.apache.org/r/33443/diff/
>
>
> Testing
> -------
>
> Yes. Unit test cases added.
>
>
> Thanks,
>
> Peeyush Bishnoi
>
>