----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28532/#review63346 -----------------------------------------------------------
Just a couple of comments: - Inconsistent naming. It should be "maxConnections", not "maxconnections", not "max_connections". The same applies to every other property. I would also avoid arbitrary abbreviatures such as "evt" in "evt_form_key". - Ideally, the way the HTTP request is formed should be moved out to a pluggable serializer class. This way, the sink can be extended without rewriting it in full. You can see examples in other sources and sinks. - Santiago Mola On Nov. 28, 2014, 7:11 p.m., Jeff Guilmard wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28532/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2014, 7:11 p.m.) > > > Review request for Flume. > > > Repository: flume-git > > > Description > ------- > > FLUME-2524 Adding an HTTP Sink > > Related to JIRA FLUME-2524 > > Flume whould have an HTTP Sink, with following capacities: > Using up to date performant Http Client > Capacity to load balance on multiple target servers (simple round robin) > Handle HTTP Authentication > use HTTP POST > Capacity to send binary data. > > New patch, replacing the first one i posted, but i didn't ask for a review of > first one because of the perf issues. > > This patch looks stable to me. It improves a lot the performances, which was > a major work in order to tune HttpClient. > It adds also the monitoring part that was missing. > This version is not accepting Batch of events (more than 1), as HTTP is not > transactional. I was not sure on how to handle this properly without breaking > the transaction behavior of Flume Channels. Therefore i did not work on that, > even if that could improve performances even more. Could be a future > improvement. > > I've included a patch for the documentation (user guide), as it is a new sink > to configure. > > > Diffs > ----- > > flume-ng-doc/sphinx/FlumeUserGuide.rst > e3aedeb1937404c260a1b81e7f9746a1a2a511d3 > flume-ng-sinks/flume-http-sink/pom.xml PRE-CREATION > > flume-ng-sinks/flume-http-sink/src/main/java/org/apache/flume/sink/http/HTTPSink.java > PRE-CREATION > > flume-ng-sinks/flume-http-sink/src/test/java/org/apache/flume/sink/http/TestHTTPSink.java > PRE-CREATION > flume-ng-sinks/pom.xml 4bac01916b39e25ba8df03eb7d699ba5c639299e > > Diff: https://reviews.apache.org/r/28532/diff/ > > > Testing > ------- > > Basic testing JUnit proposed in the patch. > I've also spent a lot of time to test the performances of the sink, before > managing to get a correct version. > > This patch is now used in production in the company I work for. > > > Thanks, > > Jeff Guilmard > >
