Tencent / TubeMQ

TubeMQ has been donated to the Apache Software Foundation and renamed to InLong, please visit the new Apache repository: https://github.com/apache/incubator-inlong
https://inlong.apache.org/
2.02k stars 391 forks source link

Remove redundant exception information log print and amend LOG level bug #82

Closed aloyszhang closed 4 years ago

aloyszhang commented 4 years ago

Currently ,there are some redundant exception information print like :

catch (Exception e) {
            e.printStackTrace();
            logger.info("[loadTopicConfUnits error] ", e);
            throw e;
        }

And, some LOG level error like :

catch (Throwable t) {
            logger.info("Dispatcher start failed!", t);
            throw new ServletException(t);
        }
gosonzhang commented 4 years ago

These are really places that needs to be aligned and modified. Can you mention a pr to see the specific modification point?

tisonkun commented 4 years ago

Thanks for reporting this issue. In my opinion

catch (Exception e) {
            e.printStackTrace();
            logger.info("[loadTopicConfUnits error] ", e);
            throw e;
        }

we might be able to reduce e.printStackTrace() because stack trace is already logged by logger. The subtle difference is that e.printStackTrace() print stack trace in STDOUT. @gosonzhang please check whether our users rely on STDOUT output.

catch (Throwable t) {
            logger.info("Dispatcher start failed!", t);
            throw new ServletException(t);
        }

though usually we log exception in debug/warn/error level, Logger#info can also log exception as other level does. Here the concern is that change to warn might flood user log monitor if these logs are too much.

However, if you open a pull request for this kind of changes, we can deal with them case by case.

aloyszhang commented 4 years ago

@TisonKun Thanks for you reply.
I agree with that Logger#info can also log exception as other level does. However , these several places of Logger#info means some error when loading master meta data from BDB StoreUnit. So I think use Logger#error is the better way.

tisonkun commented 4 years ago

Thanks for your explanation @aloyszhang ! Good contribution :-)