(1.x) STORM-2854 Expose IEventLogger to make event logging pluggable #2458
Conversation
|
Tested manually with FileBasedEventLogger. |
HeartSaVioR
force-pushed
the
STORM-2854-1.x
branch
2 times, most recently
from
f6b9152 to
a0d297d
Compare
conf/storm.yaml.example
Outdated
| # topology.event.logger.register: | ||
| # - class: "org.apache.storm.metric.FileBasedEventLogger" | ||
| # - class: "org.mycompany.MyEventLogger" | ||
| # argument: |
There was a problem hiding this comment.
Nit: May want to make this "arguments" if it is a list.
There was a problem hiding this comment.
I'm following the name for metrics consumer, but I think it is worth to fix. Moreover we're deprecating metrics consumer so it looks OK. I'll fix it.
There was a problem hiding this comment.
Ah also it is a Map<String,Object>. I think I should remove -. Will remove.
| @Override | ||
| public String toString() { | ||
| return new Date(Long.parseLong(ts)).toString() + "," + component + "," + task + "," + messageId + "," + values; | ||
| return new Date(ts).toString() + "," + component + "," + String.valueOf(task) + "," |
There was a problem hiding this comment.
Do we want to support configurable date formats or default to Date.toString()?
There was a problem hiding this comment.
Instead of doing that, I just open possibility to extend FileBasedEventLogger easily. Users can simply extend FileBasedEventLogger and override buildLogMessage(EventInfo) to provide the log line.
There was a problem hiding this comment.
Fair enough. I think it would be helpful to document that.
There was a problem hiding this comment.
Yeah good suggestion. I'll address.
|
|
||
| # Event Logger | ||
| # topology.event.logger.register: | ||
| # - class: "org.apache.storm.metric.FileBasedEventLogger" |
There was a problem hiding this comment.
nit: remove the metric package name reference
There was a problem hiding this comment.
Eventlogging classes are in org.apache.storm.metric package. I don't know why, but if we can't move them to another package, I think it should be this value since this requires full class name.
|
+1, once the minor nits are addressed. |
|
@ptgoetz @arunmahadevan |
|
+1 |
HeartSaVioR
force-pushed
the
STORM-2854-1.x
branch
from
cf62f74 to
91d3cae
Compare
* expose option to register IEventLogger similar to metrics consumer
* change the interface of IEventLogger slightly
* allow argument
* the change is technically not backward compatible but in real we can treat it's OK
* cause we don't provide a chance to implement custom IEventLogger and plug to topology
* Fix EventInfo to contain origin type of values instead of String-converted values
* open possibility to extend FileBasedEventLogger and provide different format of log message
* document the change
* address review comments
HeartSaVioR
force-pushed
the
STORM-2854-1.x
branch
from
91d3cae to
78e7077
Compare
4 participants
For master branch: #2457