-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HH-204314 add error log on datasource creation error with db name #559
base: master
Are you sure you want to change the base?
Conversation
|
||
FileSettings healthCheckSettings = dataSourceSettings.getSubSettings(HEALTHCHECK_SETTINGS_PREFIX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а зачем порядок поменял?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
я прост переменную dataSourceName выше поднял, и все)
ab76668
to
b900fad
Compare
|
||
LOGGER.error(errorMessage, e); | ||
RuntimeException dataSourceCreationException = new RuntimeException(errorMessage); | ||
dataSourceCreationException.addSuppressed(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не варик передавать ориганальное исключение в качестве cause, так как вот тут мы мы пишем лог в stderr и берем сообщение из рутового исключения. То есть если передать оригинальное исключение как cause, то в stderr не будет писаться название датасорса, пользак и хост. Поэтому добавляю оригинальное исключение как suppressed, чтоб оно фигурировало в стек трейсе, который мы вот тут пишем
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
выглядит костыльненько конечно.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
я бы предложил переписать https://github.com/hhru/nuts-and-bolts/blob/master/nab-starter/src/main/java/ru/hh/nab/starter/NabApplication.java#L177
доставать по умолчанию эксепшен сам, если нет допов.
А мб вообще все логировать. Так еще лучше. есессенно не в этой задаче.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
я бы логировал с cause, мы не должны зависеть от того как кто-то читает наши логи
|
||
LOGGER.error(errorMessage, e); | ||
RuntimeException dataSourceCreationException = new RuntimeException(errorMessage); | ||
dataSourceCreationException.addSuppressed(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
выглядит костыльненько конечно.
|
||
LOGGER.error(errorMessage, e); | ||
RuntimeException dataSourceCreationException = new RuntimeException(errorMessage); | ||
dataSourceCreationException.addSuppressed(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
я бы предложил переписать https://github.com/hhru/nuts-and-bolts/blob/master/nab-starter/src/main/java/ru/hh/nab/starter/NabApplication.java#L177
доставать по умолчанию эксепшен сам, если нет допов.
А мб вообще все логировать. Так еще лучше. есессенно не в этой задаче.
return hikariDataSource; | ||
} catch (RuntimeException e) { | ||
String url = hikariConfig.getJdbcUrl(); | ||
String host = url.split("\\?")[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Предлагаю сделать так
String jdbcUrl = "jdbc:postgresql://localhost:5432/testDB";
URI uri = URI.create(jdbcUrl.substring(5));
String host = uri.getHost();
String user = Optional | ||
.ofNullable(hikariConfig.getUsername()) | ||
.orElseGet(() -> { | ||
Matcher matcher = Pattern.compile("user=([^&]+)").matcher(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не понимаю откуда взялась эта часть. В документации этих переменных нет
https://jdbc.postgresql.org/documentation/use/
P.S. разобрался, это query параметры
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Предлагаю сделать так
String url = "jdbc:postgresql://localhost:5432/testDB?user=myCooluser&password=myCoolPassword";
URI uri = URI.create(url.substring(5));
String userFromConfig = null /*magic*/;
String userFromQuery = null;
Map<String, List<String>> queryParams = UriComponent.decodeQuery(uri.getQuery(), false, false); /* наш класс из nab */
userFromQuery = queryParams.get(USER).stream().findFirst().orElseGet(() -> "unknown");
String user = userFromConfig != null ? userFromConfig : userFromQuery;
Important
Добавь в описание PR changelog в формате: