----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8596/#review14504 -----------------------------------------------------------
Mike, Nice work!! A few comments below. My only concern about the implementation is how often fsync() is being is being called. I don't know how often that will be and if it's often enough I could see the source being unable to keep up with input data. flume-ng-clients/flume-ng-forwarder/src/main/java/org/apache/flume/clients/forwarder/FlumeTool.java <https://reviews.apache.org/r/8596/#comment30880> Should this tool stuff be in core? Seems very useful for more than the forwarder... flume-ng-clients/flume-ng-forwarder/src/main/java/org/apache/flume/clients/forwarder/ToolRunner.java <https://reviews.apache.org/r/8596/#comment30879> Returns null if the name is not found flume-ng-clients/flume-ng-forwarder/src/test/java/org/apache/flume/clients/forwarder/Blah2.java <https://reviews.apache.org/r/8596/#comment30881> Probably a mistake? flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileEventParser.java <https://reviews.apache.org/r/8596/#comment30882> As opposed to the note, could you use the interface annotations? flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileEventParser.java <https://reviews.apache.org/r/8596/#comment30883> If this delete fails we will poison there data. I think we show throw an exception if it fails. flume-ng-core/src/main/java/org/apache/flume/serialization/EventDeserializerFactory.java <https://reviews.apache.org/r/8596/#comment30878> Do you think we could give a better error message here? That the class is not of Builder type? flume-ng-core/src/main/java/org/apache/flume/serialization/LineDeserializer.java <https://reviews.apache.org/r/8596/#comment30887> nit: args are reverse order as the builder flume-ng-core/src/main/java/org/apache/flume/serialization/LineDeserializer.java <https://reviews.apache.org/r/8596/#comment30884> readLine can return null. flume-ng-core/src/main/java/org/apache/flume/serialization/PositionTrackerFile.java <https://reviews.apache.org/r/8596/#comment30888> How often do we feel storePosition is going to be called? The reason I am wondering is that sync is pretty expensive. flume-ng-core/src/test/java/org/apache/flume/client/avro/TestSpoolingFileLineReader.java <https://reviews.apache.org/r/8596/#comment30885> How come all these tests are now ignored? -- nevermind I see the comment about this being an early patch flume-ng-core/src/test/java/org/apache/flume/serialization/TestResettableFileInputStream.java <https://reviews.apache.org/r/8596/#comment30886> Should be true by default? (ie false only in the ide of the developer? - Brock Noland On Dec. 14, 2012, 9:56 a.m., Mike Percy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8596/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2012, 9:56 a.m.) > > > Review request for Flume. > > > Description > ------- > > Defines EventDeserializer interface and uses it from the spooling source. > Progress is persisted as bytes are read from the underlying file. > > > This addresses bug FLUME-1632. > https://issues.apache.org/jira/browse/FLUME-1632 > > > Diffs > ----- > > flume-ng-clients/flume-ng-forwarder/pom.xml PRE-CREATION > > flume-ng-clients/flume-ng-forwarder/src/main/java/org/apache/flume/clients/forwarder/FileClient.java > PRE-CREATION > > flume-ng-clients/flume-ng-forwarder/src/main/java/org/apache/flume/clients/forwarder/FlumeTool.java > PRE-CREATION > > flume-ng-clients/flume-ng-forwarder/src/main/java/org/apache/flume/clients/forwarder/ToolRegistry.java > PRE-CREATION > > flume-ng-clients/flume-ng-forwarder/src/main/java/org/apache/flume/clients/forwarder/ToolRunner.java > PRE-CREATION > > flume-ng-clients/flume-ng-forwarder/src/test/java/org/apache/flume/clients/forwarder/Blah2.java > PRE-CREATION > > flume-ng-clients/flume-ng-forwarder/src/test/resources/flume-log4jtest.properties > PRE-CREATION > flume-ng-clients/pom.xml 7ea2d1d > flume-ng-core/pom.xml 2554a0e > flume-ng-core/src/main/avro/TransferStateFileMeta.avsc PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/client/avro/AvroCLIClient.java > 37e9ffa > > flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileEventParser.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/client/avro/SpoolingFileLineReader.java > 8362299 > > flume-ng-core/src/main/java/org/apache/flume/serialization/EventDeserializer.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/serialization/EventDeserializerFactory.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/serialization/EventDeserializerType.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/serialization/EventSerDe.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/serialization/EventSerializer.java > a418935 > > flume-ng-core/src/main/java/org/apache/flume/serialization/LineDeserializer.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/serialization/PositionTracker.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/serialization/PositionTrackerFile.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/serialization/Resettable.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/serialization/ResettableFileInputStream.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/serialization/ResettableInputStream.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/serialization/Seekable.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java > 61824d8 > > flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySourceConfigurationConstants.java > 806a661 > > flume-ng-core/src/test/java/org/apache/flume/client/avro/TestSpoolingFileLineReader.java > 740bc98 > > flume-ng-core/src/test/java/org/apache/flume/serialization/TestResettableFileInputStream.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java > 6e87b21 > flume-ng-core/src/test/resources/TestResettableFileInputStream_1.avro > PRE-CREATION > > flume-ng-core/src/test/resources/TestResettableFileInputStream_1.truncated.avro > PRE-CREATION > pom.xml 53ac96b > > Diff: https://reviews.apache.org/r/8596/diff/ > > > Testing > ------- > > Early patch. Not all unit tests are passing yet. I am posting this for early > feedback. > > > Thanks, > > Mike Percy > >
