Review Board 1.7.22


FLUME-1126: Support RFC 3164 and 5424 syslog format timestamp parsing

Review Request #4809 - Created April 19, 2012 and updated

Prasad Mujumdar
trunk
FLUME-1126
Reviewers
Flume
aprabhakar, mpercy
flume-git
Support for timestamp and hostname parsing.
Updated syslog tests to cover timestamp and hostname handling.
Manually tested using syslog4j
Review request changed
Updated (April 27, 2012, 2:58 p.m.)
Refactored to use regex for syslog header format. Also additional config to specify add-hoc format 
Ship it!
Posted (April 27, 2012, 4:11 p.m.)
+1

Changes look good Prasad. Some nits noted below. Once you are done with the revision, please update the patch on the Jira.
convention: rename to messageFormats instead of using underscore.
convention: please use class name with CamelCase (SyslogFormater)
nit: please rename it to initHeaderFormats() instead.
nit: please remove extraneous whitespace.
nit:trailing whitespace.
Posted (April 27, 2012, 4:27 p.m.)

   

  
Its not used anymore, removed.
Done
Done
Done
Done
Posted (April 27, 2012, 9:27 p.m.)
Interesting approach, Prasad. Looks pretty nice. I just have a couple of comments.
The RFC5424 version field should be a digit followed by a space. So I think this pattern should be: "(?:\\d\\s)?"; i.e. no [] character class around in. Worth adding a unit test w/ the example messages from RFC5424 to be sure it's compliant.
  1. Actually, I just checked and it works as-is. So, nevermind this comment :)
Why would the Scanner be closed? Shouldn't this exception be logged or re-thrown?
  1. Looks like it's not a closed Scanner but it will throw if match() is called when there are no matches. So, please disregard this too.