FluentLogFactory: Why use WeakHashMap to store loggers? about fluent-logger-java HOT 10 CLOSED

@todesking Thanks for your report.

@muga Why did you change the implementation of Map from HashMap to WeakHashMap at d5efd4d?

from fluent-logger-java.

@muga Any updates?

from fluent-logger-java.

@todesking We've used WeakHashMap and finalize method for reducing the heap space consumption. I thought and considered the possibility that some users create a lot of FluentLogger objects on his JVM. But the implementation is not good in some cases, you know.

@komamitsu We should remove FluentLoggerFactory (and the WeakHashMap impl.) from external API. FluentLogger objects and the object creations should be managed by users.

from fluent-logger-java.

@todesking good catch, thanks for your report.
We have 2 options:

1. Reverting the store of FluentLoggerFactory to HashMap.
2. Removing FluentLoggerFactory.

I prefer to use 1. for the backward compatibility. Also, users can choose whether they manage the object by themselves or by FluentLoggerFactory. For handling small number of the objects, it's enough.

from fluent-logger-java.

@muga @oza Thanks for your ideas.

For option 1, I'm concerned about memory consumption because all FluentLoggers stored in HashMap are never released. I think option 2 is simple and isn't confusable. Of course, backward compatibility is important, though as you said.

Also, users can choose whether they manage the object by themselves or by FluentLoggerFactory.

FluentLogger internally depends on FluentLogFactory. https://github.com/fluent/fluent-logger-java/blob/master/src/main/java/org/fluentd/logger/FluentLogger.java#L32. So we need to remove the dependency from FluentLogger if we choose this option.

from fluent-logger-java.

@komamitsu Rethinking of this - how about having the key of WeakHashMap at FluentLogger via FluentLogger#setFactoryKey or something to prevent the GC? When a instance of FluentLogger#close()
is called, resource management can be done by setting key as null.

from fluent-logger-java.

@oza Make sense.
But adding another attribute into FluentLogger may make it more complicated. Also, factoryKey doesn't sound an essential information for FluentLogger...

I think the cause of this problem is the way of holding a reference of FluentLogger in FluentLoggerFactory. I created a PR (#38) for it. The PR has the following little downsides. But I think they are ignorable.

• FluentLoggerFactory#getLogger linearly searches for a FluentLogger which has the same key consists of tagPrefix, host, port and so on. But the number of FluentLogger entries won't be large actually.
• FluentLoggerFactory#getLoggers broke the compatibility. But this method is package private and only for test.

What do you think?

from fluent-logger-java.

@komamitsu I agree with your idea. @muga what do you think?

from fluent-logger-java.

@komamitsu @oza thank you for driving this issue and fixing. #38 looks good to me to fix this issue. compatibility is important:-)

from fluent-logger-java.

@muga @komamitsu Merged #38.

@todesking Thanks for your reporting!

from fluent-logger-java.