> On 2012-03-28 21:43:06, Hari Shreedharan wrote: > > flume-ng-core/src/main/java/org/apache/flume/event/EventHelper.java, line 47 > > <https://reviews.apache.org/r/4497/diff/2/?file=96930#file96930line47> > > > > Maybe log this?
Logging this isn't needed... this is not an exceptional case (just an empty body). This is just working around a bug in HexDump where it improperly handles a zero-length array. > On 2012-03-28 21:43:06, Hari Shreedharan wrote: > > flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java, line > > 130 > > <https://reviews.apache.org/r/4497/diff/2/?file=96931#file96931line130> > > > > 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. This comes after the line: Configurables.ensureRequiredNonNull(context, "bind", "port"); So by the time it reaches this statement, "port" is guaranteed not to be null. > On 2012-03-28 21:43:06, Hari Shreedharan wrote: > > flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java, line > > 315 > > <https://reviews.apache.org/r/4497/diff/2/?file=96931#file96931line315> > > > > Why would charsRead be 0, if the client sent a line longer than > > maxLineLength? Shouldn't it be > maxLineLength? No, the condition here is that we didn't read any additional data, the buffer is full, and we searched and didn't find any newlines. Therefore we conclude that the client sent a longer line than can fit into our buffer. I'll add this explanation in a comment and update the patch. > On 2012-03-28 21:43:06, Hari Shreedharan wrote: > > flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java, line > > 328 > > <https://reviews.apache.org/r/4497/diff/2/?file=96931#file96931line328> > > > > 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. You are right; this ordering is a remnant from a work-in-progress incarnation of the implementation. I will reorder these steps to be more intuitive. - Mike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4497/#review6490 ----------------------------------------------------------- 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 > >
