----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6377/#review12497 -----------------------------------------------------------
Sorry for the additional review, a few more suggestions are below. flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26593> It's a best practice to always set the causal exception as the cause in the Exception constructor. Don't get rid of the old stacktrace, prefer something like: new FlumeException("Unable to commit read of file: " + filename, originalException) However, in this case, why not just let the method throw IOException? flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26601> We should log at info level whenever this occurs, since it is not a normal condition. flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26605> Nit: can also use Collections.EMPTY_LIST flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26606> Collections.EMPTY_LIST flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26608> No need to log & throw, just throw an IllegalStateException flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26609> No need to log, just throw an IllegalStateException flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26610> This should be indexOf("win") >= 0 flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26611> No need to log, just throw an IllegalStateException flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26604> Nit: "} else {" should live on the same line as the closing and opening brace to match the style of the rest of the Flume code. flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26613> This is a very serious error, I think we should throw an exception in this case. flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26595> This exception should be logged, i.e. logger.warn("Could not find file: " + file, ex); flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java <https://reviews.apache.org/r/6377/#comment26596> This exception should be logged - Mike Percy On Oct. 12, 2012, 5:15 p.m., Patrick Wendell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6377/ > ----------------------------------------------------------- > > (Updated Oct. 12, 2012, 5:15 p.m.) > > > Review request for Flume. > > > Description > ------- > > This patch adds a spooling directory based source. The idea is that a user > can have a spool directory where files are deposited for ingestion into > flume. Once ingested, the files are clearly renamed and the implementation > guarantees at-least-once delivery semantics similar to those achieved within > flume itself, even across failures and restarts of the JVM running the code. > > This helps fill the gap for people who want a way to get reliable delivery of > events into flume, but don't want to directly write their application against > the flume API. They can simply drop log files off in a spooldir and let flume > ingest asynchronously (using some shell scripts or other automated process). > > Unlike the prior iteration, this patch implements a first-class source. It > also extends the avro client to support spooling in a similar manner. > > > This addresses bug FlUME-1425. > https://issues.apache.org/jira/browse/FlUME-1425 > > > Diffs > ----- > > > flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceConfiguration.java > da804d7 > > flume-ng-configuration/src/main/java/org/apache/flume/conf/source/SourceType.java > abbbf1c > flume-ng-core/src/main/java/org/apache/flume/client/avro/AvroCLIClient.java > 4a5ecae > > flume-ng-core/src/main/java/org/apache/flume/client/avro/BufferedLineReader.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/client/avro/LineReader.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySourceConfigurationConstants.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/client/avro/TestBufferedLineReader.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/client/avro/TestSpoolingFileLineReader.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java > PRE-CREATION > flume-ng-doc/sphinx/FlumeUserGuide.rst 953a670 > > Diff: https://reviews.apache.org/r/6377/diff/ > > > Testing > ------- > > Extensive unit tests and I also built and played with this using a stub flume > agent. If you look at the JIRA I have a configuration file for an agent that > will print out Avro events to the command line - that's helpful when testing > this. > > > Thanks, > > Patrick Wendell > >
