Unexpected JavaLogger behavior fixed

[finnetrolle]

Investigating #985 task I've created solution. I'm not sure this solution is good, but I tried to explain my thoughts and solution inside code. Also, I copy explanation here.
Also I'm not sure it's a good idea to create log files during test run. But I don't really know how to remove them after tests...

This constructor can be used to create just one logger. In this way nothing will change.
Example = Logger.JavaLogger().appendToFile("logs/first.log")

But if this constructor is used several times in different places with different files we will have bad situation
We call constructor and that means for client that we created new instance, that will write logs to given file
But in fact we created only wrapper. Our worker class java.util.logging.Logger instance will be same for all
JavaLoggers because or getLogger() method logic. And each time we create and apply file to logger -
we apply file handler to same logger.

Correct way to change this - is to ask client for unique logger name to give loggerManager change to differ files
But some users already using this method for single call without any class name. So, I decided to make workaround

I'll create logger with Logger.class name for first instance.
But each other calls of constructor will check if someone already created first logger. If so - I'll create
logger with name Logger.class[1-..]. We will have Logger, Logger1, Logger2, etc. This looks not very good, but
provides backward compatibility. But I mark this constructor as Deprecated and recommend other constructor with
the name to provide inner logger for.

[kdavisk6]

@finnetrolle

I think you may gone a little too far in this change. I agree with deprecating the default constructor, but by attempting to provide an internal workaround, you are adding complexity that we don't need. I think that by simply marking the default constructor as deprecated and add the new constructor will suffice.

We will remove the default constructor in Feign 11.

[kdavisk6]

See my comments

[finnetrolle]

@kdavisk6 I absolutely agree with you about workaround. But I decided that it's better to make proposal and this is the best I can provide to keep default constructor and change its logic. I'll fix it now

[kdavisk6]

I understand and appreciate the effort. We can add additional documentation to let user's know of the limitation and our suggested workaround.

[finnetrolle]

@kdavisk6 I'm going to change javadoc for deprecated method to warn users

[velo]

Makes sense

…

On Mon, Aug 26, 2019, 21:33 Max Syachin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/main/java/feign/Logger.java <#1047 (comment)>: > + * + * That's why this constructor will be removed in future. + */ + @deprecated + public JavaLogger() { + logger = java.util.logging.Logger.getLogger(Logger.class.getName()); + } + + /** + * Constructor for JavaLogger class + * + * @param loggerName - A name for the logger. This should be a dot-separated name and should + * normally be based on the package name or class name of the subsystem, such as java.net + * or javax.swing + */ + public JavaLogger(String loggerName) { maybe both? I think just a class could be not enough. For example if I have 2 same clients such SomeService client2 = ...; And in some reason I want to write them to different files. It will force developers to make special class. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1047?email_source=notifications&email_token=AABBLDSQ2QRXEPG7P5OC373QGOPPFA5CNFSM4IPFK4I2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCULZCI#discussion_r317527168>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABBLDS4A7PTQH44SL7AJ5DQGOPPFANCNFSM4IPFK4IQ> .

[finnetrolle]

Done =)