[
https://issues.apache.org/jira/browse/FLUME-1126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13261244#comment-13261244
]
[email protected] commented on FLUME-1126:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4809/#review7200
-----------------------------------------------------------
Thanks for the patch Prasad. I have a few nits/requests below, along with a
refactoring request noted below.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
<https://reviews.apache.org/r/4809/#comment15902>
While you are incorporating the feedback, it will be great if you could
change this class name to be convention compliant (SyslogTcpHandler instead of
syslogTcpHandler).
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
<https://reviews.apache.org/r/4809/#comment15901>
Instead of doing this, please add a constructor that takes these flags as
arguments and sets it up on a newly created syslogUtils object.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
<https://reviews.apache.org/r/4809/#comment15903>
instead it will be something like:
handler = new SyslogTcpHandler(eventSiz,e hasVersion, useRFC5424);
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
<https://reviews.apache.org/r/4809/#comment15898>
typo: Boolean instead of boolean
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
<https://reviews.apache.org/r/4809/#comment15904>
Class name SyslogUdpHandler please.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
<https://reviews.apache.org/r/4809/#comment15896>
Prefer to use the constructor that has event size, udp flag and rfc5424
flag.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
<https://reviews.apache.org/r/4809/#comment15905>
Suggest removing this constructor and the other one that takes the (isUdp,
useRFC5424) arguments.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
<https://reviews.apache.org/r/4809/#comment15897>
Suggest not modifying the constructor argument list for consistency.
Instead have the caller use the setter method as is being used for hasVersion
flag.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
<https://reviews.apache.org/r/4809/#comment15907>
This adhoc parsing has become significantly complex and hard to follow. It
will be better to instead have a standard solution that parses the event body
into subcomponents as necessary. For example it could be done by:
* using a table driven parser based on standard grammar
* using an externally specified format to parse the body of the event
* using a externally specified regular expression that parses the body of
the event.
Minimally, I request that this part be refactored so that the parsing of
the body is done in the buildEvent() method by a secondary routine instead.
This will ensure that the current implementation continues to work and the
secondary parsing can be done defensively and thus default to old semantics if
it is unable to parse the expected fields correctly.
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
<https://reviews.apache.org/r/4809/#comment15900>
Please remove this setter and instead force the use of the constructor
instead.
- Arvind
On 2012-04-20 03:19:50, Prasad Mujumdar wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4809/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-04-20 03:19:50)
bq.
bq.
bq. Review request for Flume, Arvind Prabhakar and Mike Percy.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Support for timestamp and hostname parsing.
bq.
bq.
bq. This addresses bug FLUME-1126.
bq. https://issues.apache.org/jira/browse/FLUME-1126
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java
b0485b1
bq. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java
732cce5
bq. flume-ng-core/src/main/java/org/apache/flume/source/SyslogUtils.java
653f5eb
bq.
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java
3a7c486
bq. flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUtils.java
8b1f7c5
bq.
bq. Diff: https://reviews.apache.org/r/4809/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated syslog tests to cover timestamp and hostname handling.
bq. Manually tested using syslog4j
bq.
bq.
bq. Thanks,
bq.
bq. Prasad
bq.
bq.
> Support RFC 3164 and 5424 syslog format timestamp parsing
> ---------------------------------------------------------
>
> Key: FLUME-1126
> URL: https://issues.apache.org/jira/browse/FLUME-1126
> Project: Flume
> Issue Type: Improvement
> Components: Sinks+Sources
> Affects Versions: v1.2.0
> Reporter: Prasad Mujumdar
> Assignee: Prasad Mujumdar
> Fix For: v1.2.0
>
>
> Support RFC 3164 and 5424 syslog format timestamp parsing for syslog source
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
