----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4497/#review6490 -----------------------------------------------------------
Overall, looks good. Some minor feedback below: flume-ng-core/src/main/java/org/apache/flume/event/EventHelper.java <https://reviews.apache.org/r/4497/#comment14137> Maybe log this? flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java <https://reviews.apache.org/r/4497/#comment14138> If there is no "port", context.getInteger() will return null here - causing autoboxing of a null value - ending in NullPointerException. Either port has to be an Integer or you must check for the null value before assignment. flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java <https://reviews.apache.org/r/4497/#comment14144> Why would charsRead be 0, if the client sent a line longer than maxLineLength? Shouldn't it be > maxLineLength? flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java <https://reviews.apache.org/r/4497/#comment14143> nit: Shouldn't this go at the beginning of the loop? Otherwise doesn't the first iteration of the loop just do nothing? Since the buffer was just created, it is obviously empty, so the first run of the loop seems like a no-op. - Hari On 2012-03-27 09:19:51, Mike Percy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4497/ > ----------------------------------------------------------- > > (Updated 2012-03-27 09:19:51) > > > Review request for Flume. > > > Summary > ------- > > The NetcatSource does not create one event per newline. Instead, it creates > one event per TCP session. > > I have addressed multiple issues with NetcatSource in this patch: > > 1. Properly process a new event per newline. > 2. Enforce a maximum length per line, to ensure clients cannot run the server > out of memory. (This is now configurable; the default is 512 characters.) > 3. Properly flush responses to the client. > 4. Increment counters for successful processing and failure. > 5. Use shutdownNow() when shutting down the server, otherwise an open client > connection will cause the server to hang on shutdown. > 6. Made the inner classes of NetcatSource private; I believe making them > public was unintentional. > 7. Corrected unit test so it now sends a newline with each request. Also now > checks for "OK" response from server. > 8. Attempting to sneak in a minor fix for the EventHelper.dumpEvent() method > into this patch, since I'm testing with LoggerSink and it's bugging me when > it throws an exception on a zero-length body. > > > This addresses bug FLUME-1037. > https://issues.apache.org/jira/browse/FLUME-1037 > > > Diffs > ----- > > flume-ng-core/src/main/java/org/apache/flume/event/EventHelper.java a326a70 > flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java > a841b0e > flume-ng-node/src/test/java/org/apache/flume/source/TestNetcatSource.java > fb2a960 > > Diff: https://reviews.apache.org/r/4497/diff > > > Testing > ------- > > Unit tests pass. Did a bunch of manual testing using the nc tool with a > Netcat source and a Logger sink. > > > Thanks, > > Mike > >
