Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1234#discussion_r224555245
--- Diff:
metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/syslog/Syslog5424Parser.java
---
@@ -61,16 +67,37 @@ public void configure(Map<String, Object> config) {
public void init() {
}
- @Override
@SuppressWarnings("unchecked")
+ @Override
public List<JSONObject> parse(byte[] rawMessage) {
+ Optional<MessageParserResult<JSONObject>> resultOptional =
parseOptionalResult(rawMessage);
--- End diff --
In MessageParser, you have default implementations that have a call chain
as follows:
parseOptionalResult(byte[] parseMessage) -> parseOptional(parseMessage) ->
return Optional.ofNullable(parse(parseMessage));
https://github.com/apache/metron/pull/1184/files#diff-11a0634729ded3406160d17bf725c9bbR49
I'm now seeing that GrokParser and this syslog parser have that inverted so
that parse(byte[] rawMessage) calls parseOptionalResult(byte[] rawMessage). So
this is basically because the original parse needs to flatten out the parser
result along with the default implementation for parseOptional calling
parse(...). I think in the spirit of backwards compatibility what you have is
good - ie I have no idea if folks are using the parsers.parse(message) or
parsers.parseOptional(message) methods in other ways, whether or not that's
something we'd want to support in the first place. I was thinking we could add
a note about this in the interface, however we don't want an interface to be
pinned to the implementation details of the consumers of that interface. In
lieu of that, I think your README illustrates the point just fine. The only
thing I might suggest is adding a note in the README that for the purposes of
Metron parsers, if you implement parseOptionalResult then you can simply thro
w a NotImplementedException because it will not be called.
---
