Skip to content

enable some asynchronous logging (revised)#49

Open
cosmo0920 wants to merge 20 commits intofluent:masterfrom
cosmo0920:asynchronous-logging
Open

enable some asynchronous logging (revised)#49
cosmo0920 wants to merge 20 commits intofluent:masterfrom
cosmo0920:asynchronous-logging

Conversation

@cosmo0920
Copy link
Contributor

@cosmo0920 cosmo0920 commented Sep 7, 2015

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 Outdated
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

jondoe1337 commented Mar 8, 2016

Why does this feature make no progress anymore?

@cosmo0920
Copy link
Contributor Author

cosmo0920 commented Mar 8, 2016

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

komamitsu commented Mar 9, 2016

@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 Outdated

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

@komamitsu komamitsu Mar 9, 2016

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

komamitsu commented Mar 9, 2016

@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

cosmo0920 commented Mar 9, 2016

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

komamitsu commented May 2, 2016

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

cosmo0920 commented May 2, 2016

Rebased!

komamitsu
komamitsu reviewed May 2, 2016
View reviewed changes
src/main/java/org/fluentd/logger/FluentLoggerFactory.java Outdated

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

@komamitsu komamitsu May 2, 2016

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

komamitsu commented May 3, 2016

@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

cosmo0920 commented May 9, 2016

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 Outdated
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

@komamitsu komamitsu May 9, 2016

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

komamitsu commented May 31, 2016

@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