----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3831/#review5188 -----------------------------------------------------------
Thanks for the patch Prasad. Some feedback below: * please add a couple of enums to SourceType and give a short name to these sources. I suggest something along the lines of SYSLOG_UDP and SYSLOG_TCP. flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/3831/#comment11372> I think this source qualifies for an EventDrivenSource type and not a PollableSource. The reason being that it is spawning a server socket and will thus assume the processing of client connections itself. flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/3831/#comment11373> (assuming that this is the processing logic of the accept thread once this is converted to EDS) it is not clear how the client connection is getting closed correctly. The typical paradigm is that the ServerSocket.accept() listens in a loop and spawns off the connected clients in new threads. The new thread is then responsible for cleaning up it's own resources. flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/3831/#comment11370> indentation flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java <https://reviews.apache.org/r/3831/#comment11371> This call will block until a client connects to the server socket. If no client connects, the start() method will hang indefinitely and thus cause the agent to hang. Am I missing something here? flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java <https://reviews.apache.org/r/3831/#comment11374> Same feedback as for the tcp source. This is a blocking call and needs to be turned into a EDS. - Arvind On 2012-02-16 21:49:51, Prasad Mujumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3831/ > ----------------------------------------------------------- > > (Updated 2012-02-16 21:49:51) > > > Review request for Flume. > > > Summary > ------- > > syslog source support for Flume 1.x > > > This addresses bug FLUME-892. > https://issues.apache.org/jira/browse/FLUME-892 > > > Diffs > ----- > > flume-ng-core/pom.xml d753fa1 > flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/3831/diff > > > Testing > ------- > > regression tests > Added new test for UDP soruce. tested the TCP source manually. Haven't found > a package that has the tcp syslog client. will add a testcase with custom tcp > client to test the source. > > > Thanks, > > Prasad > >
