You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Ted Malaska <te...@cloudera.com> on 2013/06/28 16:29:03 UTC
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/
-----------------------------------------------------------
(Updated June 28, 2013, 2:29 p.m.)
Review request for Flume.
Changes
-------
Added Hari's review changes
Bugs: 2007
https://issues.apache.org/jira/browse/2007
Repository: flume-git
Description
-------
We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
Diffs (updated)
-----
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
Diff: https://reviews.apache.org/r/11583/diff/
Testing
-------
File Attachments
----------------
Patch-0
https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
Thanks,
Ted Malaska
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Ted Malaska <te...@cloudera.com>.
> On July 1, 2013, 9:47 p.m., Hari Shreedharan wrote:
> > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java, lines 106-119
> > <https://reviews.apache.org/r/11583/diff/3/?file=312899#file312899line106>
> >
> > This parameter comes from the Flume configuration, not the HDFS configuration. So this will always be 30000. You probably need to pass it through a method or the configure method.
> >
> > Am I missing something here?
My bad. I thought it was a HDFS parameter. I will change it to a Flume config. That's easier anyways.
Thanks again. I will fix it tomorrow morning.
- Ted
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/#review22626
-----------------------------------------------------------
On June 28, 2013, 2:29 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11583/
> -----------------------------------------------------------
>
> (Updated June 28, 2013, 2:29 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: 2007
> https://issues.apache.org/jira/browse/2007
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
>
> Diff: https://reviews.apache.org/r/11583/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Patch-0
> https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
>
>
> Thanks,
>
> Ted Malaska
>
>
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/#review22626
-----------------------------------------------------------
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46313>
This parameter comes from the Flume configuration, not the HDFS configuration. So this will always be 30000. You probably need to pass it through a method or the configure method.
Am I missing something here?
- Hari Shreedharan
On June 28, 2013, 2:29 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11583/
> -----------------------------------------------------------
>
> (Updated June 28, 2013, 2:29 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: 2007
> https://issues.apache.org/jira/browse/2007
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
>
> Diff: https://reviews.apache.org/r/11583/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Patch-0
> https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
>
>
> Thanks,
>
> Ted Malaska
>
>
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/#review22989
-----------------------------------------------------------
Ship it!
Ship It!
- Hari Shreedharan
On July 11, 2013, 1:28 a.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11583/
> -----------------------------------------------------------
>
> (Updated July 11, 2013, 1:28 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: 2007
> https://issues.apache.org/jira/browse/2007
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
>
>
> Diffs
> -----
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst 63cad21
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
>
> Diff: https://reviews.apache.org/r/11583/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Patch-0
> https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
>
>
> Thanks,
>
> Ted Malaska
>
>
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/#review22990
-----------------------------------------------------------
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java
<https://reviews.apache.org/r/11583/#comment46773>
This test seems to fail as a result of the last change. Could you please fix this one?
- Hari Shreedharan
On July 11, 2013, 1:28 a.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11583/
> -----------------------------------------------------------
>
> (Updated July 11, 2013, 1:28 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: 2007
> https://issues.apache.org/jira/browse/2007
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
>
>
> Diffs
> -----
>
> flume-ng-doc/sphinx/FlumeUserGuide.rst 63cad21
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
>
> Diff: https://reviews.apache.org/r/11583/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Patch-0
> https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
>
>
> Thanks,
>
> Ted Malaska
>
>
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Ted Malaska <te...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/
-----------------------------------------------------------
(Updated July 11, 2013, 1:28 a.m.)
Review request for Flume.
Changes
-------
Hari is the man
Bugs: 2007
https://issues.apache.org/jira/browse/2007
Repository: flume-git
Description
-------
We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
Diffs (updated)
-----
flume-ng-doc/sphinx/FlumeUserGuide.rst 63cad21
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
Diff: https://reviews.apache.org/r/11583/diff/
Testing
-------
File Attachments
----------------
Patch-0
https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
Thanks,
Ted Malaska
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/#review22966
-----------------------------------------------------------
Hi Ted,
This looks good. Could you please add the new param to the Flume user guide. Also a couple files are missing the ASF License header, which is required. I mentioned one other change below.
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46751>
I think I mentioned this in the last review - could you please make this change:
numberOfCloseRetries = context.getInteger("hdfs.closeTries", 1) - 1;
This will ensure that the current behavior remains as is unless and until this parameter is actually set to be > 1.
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java
<https://reviews.apache.org/r/11583/#comment46749>
Missing ASF license header
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java
<https://reviews.apache.org/r/11583/#comment46750>
Missing ASF license header.
- Hari Shreedharan
On July 10, 2013, 2:26 a.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11583/
> -----------------------------------------------------------
>
> (Updated July 10, 2013, 2:26 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: 2007
> https://issues.apache.org/jira/browse/2007
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
>
> Diff: https://reviews.apache.org/r/11583/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Patch-0
> https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
>
>
> Thanks,
>
> Ted Malaska
>
>
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Ted Malaska <te...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/
-----------------------------------------------------------
(Updated July 10, 2013, 2:26 a.m.)
Review request for Flume.
Changes
-------
Fixed what Hari found
Bugs: 2007
https://issues.apache.org/jira/browse/2007
Repository: flume-git
Description
-------
We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
Diffs (updated)
-----
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
Diff: https://reviews.apache.org/r/11583/diff/
Testing
-------
File Attachments
----------------
Patch-0
https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
Thanks,
Ted Malaska
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Hari Shreedharan <hs...@cloudera.com>.
> On July 10, 2013, 12:24 a.m., Hari Shreedharan wrote:
> > This looks good Ted. Only minor feedback - and one request - to change the closeRetries to closeTries, which defaults to 1 (and if it is set to <1, we set it back to 1 anyway). Also could you please update the user guide?
We don't really need to set it back to 1 - the current logic will work if we set:
numberOfCloseRetries = context.getLong("hdfs.numberOfCloseTries",1) - 1;
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/#review22940
-----------------------------------------------------------
On July 2, 2013, 1:18 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11583/
> -----------------------------------------------------------
>
> (Updated July 2, 2013, 1:18 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: 2007
> https://issues.apache.org/jira/browse/2007
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
>
> Diff: https://reviews.apache.org/r/11583/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Patch-0
> https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
>
>
> Thanks,
>
> Ted Malaska
>
>
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Ted Malaska <te...@cloudera.com>.
> On July 10, 2013, 12:24 a.m., Hari Shreedharan wrote:
> > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java, lines 70-72
> > <https://reviews.apache.org/r/11583/diff/4/?file=314913#file314913line70>
> >
> > Hmm, if number of close retries is set to zero, then the timeBetweenCloseRetries should have no effect right? So we should "unset" the timeBetweenCloseRetries if numberOfCloseRetries < 0 - maybe set it to INT_MAX? But this really does not matter, since the closeHDFSOutputStream checks if we should retry.
I also made this change
> On July 10, 2013, 12:24 a.m., Hari Shreedharan wrote:
> > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java, line 253
> > <https://reviews.apache.org/r/11583/diff/4/?file=314913#file314913line253>
> >
> > If we change this to number of close tries, then this should also take care of that, so we call close() method only that many times.
> >
> > To keep this code the same we should perhaps set numberOfCloseRetries to the value from the config - 1, but also check that the file is not closed before logging the error (to handle the case where the closeTries is set to 1)
This also effect line 62.
- Ted
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/#review22940
-----------------------------------------------------------
On July 2, 2013, 1:18 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11583/
> -----------------------------------------------------------
>
> (Updated July 2, 2013, 1:18 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: 2007
> https://issues.apache.org/jira/browse/2007
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
>
> Diff: https://reviews.apache.org/r/11583/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Patch-0
> https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
>
>
> Thanks,
>
> Ted Malaska
>
>
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Hari Shreedharan <hs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/#review22940
-----------------------------------------------------------
This looks good Ted. Only minor feedback - and one request - to change the closeRetries to closeTries, which defaults to 1 (and if it is set to <1, we set it back to 1 anyway). Also could you please update the user guide?
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46725>
Is this used?
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46718>
Lets make this one private - are you using this one from outside this class?
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46719>
Can you change this one to "hdfs.closeTries" to make it similar to the others? Also, instead of number of close retries, let's make it number of close tries - so if this is 3 - total number of times close is called is 3.
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46720>
The default in HDFS Sink for callTimeout is 10000 - I think we should use the same default here too.
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46723>
Unnecessary newline
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46721>
Hmm, if number of close retries is set to zero, then the timeBetweenCloseRetries should have no effect right? So we should "unset" the timeBetweenCloseRetries if numberOfCloseRetries < 0 - maybe set it to INT_MAX? But this really does not matter, since the closeHDFSOutputStream checks if we should retry.
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46722>
Why so many newlines?
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46726>
If we change this to number of close tries, then this should also take care of that, so we call close() method only that many times.
To keep this code the same we should perhaps set numberOfCloseRetries to the value from the config - 1, but also check that the file is not closed before logging the error (to handle the case where the closeTries is set to 1)
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java
<https://reviews.apache.org/r/11583/#comment46724>
unnecessary newlines
- Hari Shreedharan
On July 2, 2013, 1:18 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11583/
> -----------------------------------------------------------
>
> (Updated July 2, 2013, 1:18 p.m.)
>
>
> Review request for Flume.
>
>
> Bugs: 2007
> https://issues.apache.org/jira/browse/2007
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
> flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
>
> Diff: https://reviews.apache.org/r/11583/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> Patch-0
> https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
>
>
> Thanks,
>
> Ted Malaska
>
>
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Ted Malaska <te...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/
-----------------------------------------------------------
(Updated July 2, 2013, 1:18 p.m.)
Review request for Flume.
Changes
-------
Updated the config to come from Flume Context
Bugs: 2007
https://issues.apache.org/jira/browse/2007
Repository: flume-git
Description
-------
We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
Diffs (updated)
-----
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
Diff: https://reviews.apache.org/r/11583/diff/
Testing
-------
File Attachments
----------------
Patch-0
https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
Thanks,
Ted Malaska
Re: Review Request 11583: HDFS Sink should check if file is closed and retry
if it is not.
Posted by Ted Malaska <te...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11583/
-----------------------------------------------------------
(Updated June 28, 2013, 2:29 p.m.)
Review request for Flume.
Changes
-------
Added Hari's review changes
Bugs: 2007
https://issues.apache.org/jira/browse/2007
Repository: flume-git
Description
-------
We can use the new API added in HDFS-4525. We will need to use reflection though, so we can run against a version of HDFS which does not have this API.
Diffs
-----
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/AbstractHDFSWriter.java bc3b383
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSCompressedDataStream.java 2c2be6a
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSDataStream.java b8214be
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSSequenceFile.java 0383744
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFileSystemCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/MockFsDataOutputStreamCloseRetryWrapper.java PRE-CREATION
flume-ng-sinks/flume-hdfs-sink/src/test/java/org/apache/flume/sink/hdfs/TestUseRawLocalFileSystem.java ffbdde0
Diff: https://reviews.apache.org/r/11583/diff/
Testing
-------
File Attachments
----------------
Patch-0
https://reviews.apache.org/media/uploaded/files/2013/05/31/FLUME-2007-0.patch
Thanks,
Ted Malaska