-
Notifications
You must be signed in to change notification settings - Fork 0
spring-cloud #1
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
spring-cloud #1
Conversation
EugeneLenkevich
left a comment
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.
Привет, Вячеслав! Спасибо за PR.
Отличная работа! :)
Сервисы в эврике регистрируются, супер, все вроде на месте, отметил
пару небольших моментов, посмотри комментарии, пожалуйста :)
В первую очередь логирование бы поправить, хотелось бы какие то идентификаторы в логах увидеть чтобы запросы идентифицировать. Иначе потом не разобраться с тем что произошло на проде в случае какого либо инцидента
| private final CategoryAdminService categoryAdminService; | ||
|
|
||
| @PostMapping | ||
| public ResponseEntity<CategoryDto> addCategory( |
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.
А во эти обертки, ResponseEntity, нужны ли, может лучше аннотациями задать нужные коды, нужен ли нам этот лишний код :)
| @Service | ||
| @RequiredArgsConstructor | ||
| @Slf4j | ||
| @Transactional |
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.
Давай все таки транзакции повесим на тех методах, которые меняют данные, а томы селекты гоняем в транзакциях только ресурсы лишние расходуем. То есть для операций чтения в общем случае транзакции не нужны :)
|
|
||
| @Override | ||
| public CommentDto approveComment(Long comId) { | ||
| log.info("approveComment - invoked"); |
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.
Добавить бы в логи идентификаторы, comId в данном случае. Потом же в логе не будет понятно какая строка к какому запросу или объекту относится и не будет возможности собрать всю цепочку вызовов по конкретному объекту. В реальном приложении у нас будет множество запросов одновременно :)
EugeneLenkevich
left a comment
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.
Теперь все отлично, спасибо :)
No description provided.