falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Peeyush Bishnoi" <bpeey...@yahoo.co.in>
Subject Re: Review Request 33443: FALCON-1102: Gather data transfer detail of replication job submitted from HDFS recipe
Date Wed, 07 Oct 2015 09:00:53 GMT


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


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