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