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.

Sign up for GitHub

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

Jump to bottom

enable some asynchronous logging (revised) #49

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

enable some asynchronous logging (revised) #49

wants to merge 20 commits into from

Conversation

cosmo0920
Copy link
Contributor

Revised #30.

And I modified setErrorHandler/removeErrorHandler in AsyncRawSocketSender.

Should I add tests for AsyncRawSocketSender?

cosmo0920
cosmo0920 reviewed Sep 7, 2015
View reviewed changes
src/main/java/org/fluentd/logger/FluentLoggerFactory.java
@@ -80,6 +81,30 @@ public synchronized FluentLogger getLogger(String tagPrefix, String host, int po
return logger;
}

public FluentLogger getLogger(String tagPrefix, String host, int port, int timeout, int bufferCapacity,
Copy link
Contributor Author

@cosmo0920 cosmo0920 Sep 7, 2015 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure that this method worthiness, for now....

@jondoe1337
Copy link

Why does this feature make no progress anymore?

@cosmo0920
Copy link
Contributor Author

This feature is implemented in Fluency.
I think that this library's owners do not want to add this feature because they want to keep simple implementation.

@komamitsu
Copy link
Member

@cosmo0920 Oh really sorry, I failed to notice the notifications of this project. Let me see the changes.

komamitsu
komamitsu reviewed Mar 9, 2016
View reviewed changes
src/main/java/org/fluentd/logger/FluentLoggerFactory.java
@@ -33,7 +34,7 @@
private final Map<FluentLogger, String> loggers;

public FluentLoggerFactory() {
loggers = new WeakHashMap<FluentLogger, String>();
loggers = Collections.synchronizedMap(new WeakHashMap<FluentLogger, String>());
Copy link
Member

Choose a reason for hiding this comment

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

This change doesn't seem to be related to asynchronous logging.

@komamitsu
Copy link
Member

@cosmo0920 @mxk1235 Thanks for the PRs! I left some comments. Also, it needs unit tests in principle.

BTW, as I commented earlier, AsyncRawSocketSender creates a thread for each invoking emit() method and I'm concerned about the performance.

@cosmo0920
Copy link
Contributor Author

Thanks for your review! I'll try to resolve issues which are pointed out.

@cosmo0920 cosmo0920 force-pushed the asynchronous-logging branch 2 times, most recently from f63cc3f to 7cef979 Compare April 22, 2016 09:20
@komamitsu
Copy link
Member

I fixed some test failures at 969b88d.

Could you try git rebase master to it?

@cosmo0920 cosmo0920 force-pushed the asynchronous-logging branch from 2d6c3ea to 10d57db Compare May 2, 2016 08:38
@cosmo0920
Copy link
Contributor Author

Rebased!

komamitsu
komamitsu reviewed May 2, 2016
View reviewed changes
src/main/java/org/fluentd/logger/FluentLoggerFactory.java
@@ -19,6 +19,7 @@

import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.util.Collections;
Copy link
Member

Choose a reason for hiding this comment

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

Unused import?

@komamitsu
Copy link
Member

@cosmo0920 Thanks! I left some comments on the diff. Especially, https://github.com/fluent/fluent-logger-java/pull/49/files#r61833474 is an issue, I think.

BTW, will you actually use AsyncRawSocketSender? If so, what do you think about creating an EmitRunnable for each emit() call in terms of performance. If it's just experimental, why don't you add a comment like this on the class? This feature is highly experimental

@cosmo0920 cosmo0920 force-pushed the asynchronous-logging branch from 10d57db to 7392010 Compare May 9, 2016 07:23
@cosmo0920
Copy link
Contributor Author

BTW, will you actually use AsyncRawSocketSender?

No, I won't.
It's just an experimental.

why don't you add a comment like this on the class? This feature is highly experimental

I've added this one in javadoc comment.

komamitsu
komamitsu reviewed May 9, 2016
View reviewed changes
src/main/java/org/fluentd/logger/sender/AsyncRawSocketSender.java
return this.isConnected() || reconnector.enableReconnection(System.currentTimeMillis());
try {
Future<Boolean> result = senderTask.submit(new EmitRunnable(tag, data, sender, timestamp));
return result.get();
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, Future#get will wait the result here. So this method doesn't work asynchronously...
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Future.html#get--

@komamitsu
Copy link
Member

@cosmo0920 Sorry for late reply again. Could you check this comment? 1bf11d7#r65201214

@cosmo0920 cosmo0920 force-pushed the asynchronous-logging branch from 1bf11d7 to 046b798 Compare June 1, 2016 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cosmo0920 @jondoe1337 @komamitsu