You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Hari Shreedharan <hs...@cloudera.com> on 2013/03/01 00:55:35 UTC
Re: Review Request: FLUME-1916: HDFS sink should poll for # of active
replicas. If less than required, roll the file
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9675/#review17213
-----------------------------------------------------------
+1. Looks good. I have a couple of comments which are nitpicks. If you think they need not be fixed, I will commit this patch.
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/9675/#comment36504>
nit: This getFsDesiredReplication call can be avoided if configuredMinReplicas != null by moving that call to an else block.
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/9675/#comment36507>
Is this method really required? If fs is null it means the registerCurrentStream was not called (or the unregister method was called). Can fs become null in any other case?
- Hari Shreedharan
On Feb. 28, 2013, 6:52 a.m., Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9675/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2013, 6:52 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> This is based on https://issues.apache.org/jira/browse/HBASE-2234 ... essentially if datanodes are dying en masse then we want to close our files as we are writing so that we get new pipeline assignments from the namenode.
>
>
> This addresses bug FLUME-1916.
> https://issues.apache.org/jira/browse/FLUME-1916
>
>
> Diffs
> -----
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst 5ac903e
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java 0786857
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 1f3521e
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java 4ea4327
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 3bd25f4
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSWriter.java abca21f
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockHDFSWriter.java 5e8628b
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java ebe277c
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSinkOnMiniCluster.java 2e71069
>
> Diff: https://reviews.apache.org/r/9675/diff/
>
>
> Testing
> -------
>
> Added unit test.
>
>
> Thanks,
>
> Mike Percy
>
>
Re: Review Request: FLUME-1916: HDFS sink should poll for # of active
replicas. If less than required, roll the file
Posted by Mike Percy <mp...@apache.org>.
> On Feb. 28, 2013, 11:55 p.m., Hari Shreedharan wrote:
> > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java, lines 69-72
> > <https://reviews.apache.org/r/9675/diff/1/?file=263460#file263460line69>
> >
> > nit: This getFsDesiredReplication call can be avoided if configuredMinReplicas != null by moving that call to an else block.
Done
> On Feb. 28, 2013, 11:55 p.m., Hari Shreedharan wrote:
> > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java, lines 100-105
> > <https://reviews.apache.org/r/9675/diff/1/?file=263460#file263460line100>
> >
> > Is this method really required? If fs is null it means the registerCurrentStream was not called (or the unregister method was called). Can fs become null in any other case?
I left it in for now... we might want to expose this to the BucketWriter for the future, I don't think it hurts anything
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9675/#review17213
-----------------------------------------------------------
On March 1, 2013, 1:53 a.m., Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9675/
> -----------------------------------------------------------
>
> (Updated March 1, 2013, 1:53 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> This is based on https://issues.apache.org/jira/browse/HBASE-2234 ... essentially if datanodes are dying en masse then we want to close our files as we are writing so that we get new pipeline assignments from the namenode.
>
>
> This addresses bug FLUME-1916.
> https://issues.apache.org/jira/browse/FLUME-1916
>
>
> Diffs
> -----
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst 5ac903e
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/BucketWriter.java cdc37f6
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 1f3521e
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java 4ea4327
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 3bd25f4
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSWriter.java abca21f
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockHDFSWriter.java 5e8628b
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestBucketWriter.java 99b6150
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestHDFSEventSinkOnMiniCluster.java 2e71069
>
> Diff: https://reviews.apache.org/r/9675/diff/
>
>
> Testing
> -------
>
> Added unit test.
>
>
> Thanks,
>
> Mike Percy
>
>