-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature-#34 parallel logging #42
Conversation
…nder implementations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defaultHostname uses always the value ""
, it should be either configurable or just a default fallback value in the function, passing always same literal string in the constructor doesn't look nice
src/main/java/com/teragrep/jla_01/syslog/hostname/Hostname.java
Outdated
Show resolved
Hide resolved
src/main/java/com/teragrep/jla_01/syslog/hostname/Hostname.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
||
import com.teragrep.rlp_01.pool.Stubable; | ||
|
||
public interface RelpAppender<E> extends Stubable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name ending in -er
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the naming is to reflect the UnsynchronizedAppenderBase naming
* Unsynchronized version of RELP Appender for Logback. It is configured in a Bean manner using setters by Logback. | ||
* @param <E> | ||
*/ | ||
public final class RlpLogbackAppender<E> extends UnsynchronizedAppenderBase<E> implements IRelpAppenderConfig<E> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another issue to make immutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, Bean type classes unfortunately can't be immutable due to the way they are configured via the setters and reflection.
import com.teragrep.rlo_14.SyslogMessage; | ||
|
||
public interface SyslogRecord { | ||
SyslogMessage getRecord(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be named record()
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it could be. I will just leave it as it is for now because #44 should change it
SyslogRecord syslogRecord = new SyslogRecordConfigured(hostname, appName); | ||
syslogRecord = new SyslogRecordTimestamp(syslogRecord); | ||
syslogRecord = new SyslogRecordOrigin(syslogRecord, originalHostname); | ||
if (enableEventId48577) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enableEventId48577 boolean seems to configure the behaviour of this method here, so there seems to be use cases where sometimes you want the eventId, sometimes you don't.
Should this be separated into a different object, so that you have RelpAppenders that include the eventId and RelpAppenders that don't include it, to make it more obvious when the eventId will be appended, and when it's omitted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please create an issue about this
eventIdSDE.addSDParam("uuid", uuid); | ||
eventIdSDE.addSDParam("source", "source"); | ||
|
||
long unixtime = System.currentTimeMillis(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SyslogRecordTimeStamp the timestamps are generated using the Instant object, and here we use another method for the same end result. Not sure if it matters, but should we use Instant here as well to be consistent with other objects on what method we use to measure time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good point. please create an issue about this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left two minor comments, otherwise this looks good to me!
No description provided.