Skip to content
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

Introduce a pulsar log4j2 appender #1316

Merged
merged 3 commits into from
Mar 2, 2018
Merged

Conversation

sijie
Copy link
Member

@sijie sijie commented Mar 2, 2018

Motivation

when developing pulsar-functions, it would be good to log the log messages generated by functions to pulsar, so the user can easily tail the log messages topic to get all the messages per functions.

Modifications

This PR is to add a pulsar based appender.

Result

We are able to use pulsar log4j2 appender to log messages.

@sijie sijie added the type/feature The PR added a new feature or issue requested a new feature label Mar 2, 2018
@sijie sijie requested review from merlimat and rdhabalia March 2, 2018 04:21
@sijie
Copy link
Member Author

sijie commented Mar 2, 2018

NOTE:

Ideally I would like to contribute this module as part of log4j2 - https://github.com/apache/logging-log4j2 . However we would like to use this module for pulsar-functions in #1314 , so make the initial contribution here. At the same time, I would send a PR to log4j2 as well.

@sijie
Copy link
Member Author

sijie commented Mar 2, 2018

filed a jira issue to log4j2 to contribute this appender back to log4j2 : https://issues.apache.org/jira/browse/LOG4J2-2280

@sijie
Copy link
Member Author

sijie commented Mar 2, 2018

retest this please

1 similar comment
@sijie
Copy link
Member Author

sijie commented Mar 2, 2018

retest this please

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just few comments

<scope>test</scope>
</dependency>
<!-- Required for AsyncLoggers -->
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this in top level dependency management to fix the version there.

public boolean releaseSub(final long timeout, final TimeUnit timeUnit) {
if (producer != null) {
try {
producer.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use closeAsync().get(timeout, timeUnit) to respect the contract?

producer = client.newProducer()
.topic(topic)
.producerName("pulsar-log4j2-appender-" + topic)
.enableBatching(true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the mode is syncSend, batching would just increase the latency (due to waiting for time budget) and decrease the throughput, since we only have 1 in-flight message.

.topic(topic)
.producerName("pulsar-log4j2-appender-" + topic)
.enableBatching(true)
.batchingMaxPublishDelay(1, TimeUnit.MILLISECONDS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use batching (with sendAsync()) we could increase this delay, since most likely it won't be latency sensitive.

@sijie
Copy link
Member Author

sijie commented Mar 2, 2018

retest this please

@sijie
Copy link
Member Author

sijie commented Mar 2, 2018

@merlimat I have addressed all your comments.

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sijie sijie merged commit 973fa3a into apache:master Mar 2, 2018
@sijie sijie deleted the log4j_appender branch March 2, 2018 21:07
@merlimat merlimat added this to the 2.0.0-incubating milestone Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants