You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2012/02/10 01:43:59 UTC
Review Request: FLUME-892: Support for SysLog as source
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/
-----------------------------------------------------------
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/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java PRE-CREATION
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
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
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/#review5027
-----------------------------------------------------------
See comments below.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
<https://reviews.apache.org/r/3831/#comment11030>
SyslogUtils.extractEvent can return null. It doesn't appear as though ChannelProcessor.processEvent is prepared to handle null.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
<https://reviews.apache.org/r/3831/#comment11036>
PollableSourceRunner expects the process method to return non-null. Should BACKOFF be returned or should the IOException be wrapped and rethrown?
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
<https://reviews.apache.org/r/3831/#comment11031>
Is Source.process supposed to be thread safe? If not, do we have to allocate this memory each time?
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
<https://reviews.apache.org/r/3831/#comment11032>
Although 0 is very likely the correct offset, shouldn't we be using pkt.getOffset()?
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
<https://reviews.apache.org/r/3831/#comment11034>
As stated earlier, extractEvent could return null?
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
<https://reviews.apache.org/r/3831/#comment11035>
PollableSourceRunner expects the process method to return non-null. Should BACKOFF be returned or should the IOException be wrapped and rethrown?
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
<https://reviews.apache.org/r/3831/#comment11037>
From looking at PollableSourceRunner it looks like the proccess method will be called if the start method returns non-error. At this point the process method will NPE correct?
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
<https://reviews.apache.org/r/3831/#comment11033>
It looks like this method will not throw an IOException?
- Brock
On 2012-02-10 00:43:59, Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3831/
> -----------------------------------------------------------
>
> (Updated 2012-02-10 00:43:59)
>
>
> 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/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java PRE-CREATION
> 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
>
> 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
>
>
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/#review5656
-----------------------------------------------------------
Ship it!
- Arvind
On 2012-03-06 20:15:22, Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3831/
> -----------------------------------------------------------
>
> (Updated 2012-03-06 20:15:22)
>
>
> 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
>
>
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/
-----------------------------------------------------------
(Updated 2012-03-06 20:15:22.977780)
Review request for Flume.
Changes
-------
Rebase with context changes
Summary
-------
syslog source support for Flume 1.x
This addresses bug FLUME-892.
https://issues.apache.org/jira/browse/FLUME-892
Diffs (updated)
-----
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
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/
-----------------------------------------------------------
(Updated 2012-03-06 19:36:00.950734)
Review request for Flume.
Changes
-------
rebased patch
Summary
-------
syslog source support for Flume 1.x
This addresses bug FLUME-892.
https://issues.apache.org/jira/browse/FLUME-892
Diffs (updated)
-----
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
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/#review5646
-----------------------------------------------------------
Thanks for the update Prasad. Looks like you rebased it to one version prior to the latest changes to context which is causing problems. Can you please refresh and rebase again?
Thanks
- Arvind
On 2012-02-22 18:26:46, Prasad Mujumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3831/
> -----------------------------------------------------------
>
> (Updated 2012-02-22 18:26:46)
>
>
> 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
>
>
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/
-----------------------------------------------------------
(Updated 2012-02-22 18:26:46.629023)
Review request for Flume.
Changes
-------
Re-factored the TCP and UPD sources to use netty package instead of raw socket. Also made the sources to event driven.
Summary
-------
syslog source support for Flume 1.x
This addresses bug FLUME-892.
https://issues.apache.org/jira/browse/FLUME-892
Diffs (updated)
-----
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
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/#review5168
-----------------------------------------------------------
Ship it!
- Brock
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
>
>
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-892: Support for SysLog as source
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3831/
-----------------------------------------------------------
(Updated 2012-02-16 21:49:51.169691)
Review request for Flume.
Changes
-------
Updates per review comments.
Summary
-------
syslog source support for Flume 1.x
This addresses bug FLUME-892.
https://issues.apache.org/jira/browse/FLUME-892
Diffs (updated)
-----
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