search

openshift / openshift-restclient-java

Other
78 stars 112 forks source link

corrected logging for inaccessible legacy apis to info (#462) #465

Closed adietish closed 4 years ago

adietish commented 4 years ago

fixes #462

adietish commented 4 years ago

Hi @mhagnumdw

Can I please have your +1?

mhagnumdw commented 4 years ago

Hi @adietish, the only slightly different thing I saw is that you use the exception toString(). When we usually use getMessage() or even delegate the exception instance to the logger, something like logger.info(String msg, Throwable t) (but it's strange INFO prints an exception).

If this was not by mistake, that's fine by me! Tks!!

adietish commented 4 years ago

Hi @adietish, the only slightly different thing I saw is that you use the exception toString(). When we usually use getMessage() or even delegate the exception instance to the logger, something like logger.info(String msg, Throwable t) (but it's strange INFO prints an exception).

Hmm, weird, "e" is actually the endpoint, not the exception. The exception is "ex". The slf4j FAQ actually specifies that the last parameter is treated as the exception if identified as such: http://www.slf4j.org/faq.html#paramException Where did you see the #toString?

mhagnumdw commented 4 years ago

Ohh.. Sorry! My mistake! I got confused when reading the code.

mhagnumdw commented 4 years ago

I tested it locally... Before the exception was not put in the log. Now, the exception is placed in the log and as INFO. An exception printed as INFO is strange. Well... feel free to approve the MR and thanks again!

I hope I didn't read the code wrong again 😅

adietish commented 4 years ago

I tested it locally... Before the exception was not put in the log. Now, the exception is placed in the log and as INFO. An exception printed as INFO is strange.

Yes I agree, but then it's quite useless to get a message that the oapi could not be reached without knowing the reason for it.

adietish commented 4 years ago

merged.