Stage 1, Андрей Чешев, Политех #16
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
There was a problem hiding this comment.
В решении есть серьёзные недочёты, такие как отсутствие обработки исключений и некорректная остановка сервиса. И, похоже, профилировалось приложение с "холодной" jvm (прошу учесть в следующих этапах)
Тем не менее, судя по отчёту, по результатам профилирования первого решения были выполнены оптимизации, направленные на уменьшение генерации мусора. Хотя, флеймграфы неинтерактивные, проверить отдельные выводы нельзя (прошу учесть в следующих этапах, чтобы не терять баллы)
Поэтому 8 баллов
И перед мержем необходимо откатить лишние изменения
| import java.util.NoSuchElementException; | ||
| import java.util.PriorityQueue; | ||
| import java.util.Queue; | ||
| import java.util.*; |
There was a problem hiding this comment.
Необходимо откатить изменения в несвязанных с этапом файлах. Здесь и в еще одном классе reference/dao
| private static final String ID = "id="; | ||
| private final ReferenceDao dao; | ||
|
|
||
| public ServerImpl(ServiceConfig config) throws IOException { |
There was a problem hiding this comment.
Думаю, не очень хорошо, что конфиг сервиса просочился на уровень сервера. Было бы лучше, если бы сервис разобрал данный конфиг, на его основе сформировал бы конфиги зависимостей и инстанцировал их с этими конфигами
| return serverConfig; | ||
| } | ||
|
|
||
| public Response get(final Request request) { |
There was a problem hiding this comment.
Методы get,put,delete и createServerConfig являются публичными, хотя этого не требуется. Нужно ужесточить доступность/видимость
|
|
||
| int method = request.getMethod(); | ||
|
|
||
| Response response = switch (method) { |
There was a problem hiding this comment.
Для всех методов отсутствует обработка исключений от DAO
Что, кстати, будет при выбросе исключения?) Автоматом 500 ответ или, может, отсутствие ответа?)
| dao.close(); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| super.stop(); |
There was a problem hiding this comment.
Сначала закрывается dao, затем сервер перестаёт слушать порт
Получается, что будет попытка обработать часть запросов уже с закрытым dao
Поэтому, нужно сначала прекратить принимать http запросы, а уже потом закрывать dao
| * Результаты работы wrk2 при стабильной нагрузкe на протяжении 2х минут:  | ||
|
|
||
| ### Профилирование запросов типа PUT: | ||
| * Профилирование CPU:  |
There was a problem hiding this comment.
Высокая доля сэмплов от работы JVM может свидетельствовать о том, что JVM не была "прогрета" перед началом профилирования. Начинать снимать профили имеет смысл, когда уже было выполнено какое-то количество вставок в базу, прошло, скажем, секунд 30. Тогда горячий код успеет скомпилироваться
| * Убрана двойная аллокация через getPath() | ||
|
|
||
| Попытка оптимизировать возвращение результата с кодом OK и body: | ||
| * Было замечено, что в используемом конструкторе Response() происходит двойная аллокация строки "Content-Length: " + body.length (на FlameGraph видно вызов метода StringConcatHelper.newString()). |
There was a problem hiding this comment.
Зря профили были экспортированы в SVG. Потерялась интерактивность, у ревьюера нет возможности их внимательно изучить
в частности, хотелось полученный вывод о двойной аллокации проверить
|
|
||
| public class ServiceImpl implements Service { | ||
| private final ServiceConfig serviceConfig; | ||
| private ServerImpl server; |
There was a problem hiding this comment.
Вероятно, здесь будет достаточно HttpServer/Server
| acceptorConfig.port = config.selfPort(); | ||
| acceptorConfig.reusePort = true; | ||
|
|
||
| HttpServerConfig serverConfig = new HttpServerConfig(); |
There was a problem hiding this comment.
Rename "serverConfig" which hides the field declared at line 18.
|
Исправил замечания. |
| public ServerImpl(HttpServerConfig config, Config daoConfig) throws IOException { | ||
| super(config); | ||
|
|
||
| this.dao = new PersistentReferenceDao(daoConfig); |
There was a problem hiding this comment.
Dao, в принципе, тоже можно было бы инстанцировать в сервисе и в сервер просто инжектить
* rename package * 8/14 tests * 12/14 tests * 14/14 tests * fixes * fixes * fixes * + report * codestyle fixes * codestyle fixes * add lua scripts * report fix * HTML to SVG * add THRESHOLD_BYTES const * code review fixes * code climate fixes * style fixes --------- Co-authored-by: Anton Lamtev <lamtev@users.noreply.github.com>
* rename package * 8/14 tests * 12/14 tests * 14/14 tests * fixes * fixes * fixes * + report * codestyle fixes * codestyle fixes * add lua scripts * report fix * HTML to SVG * add THRESHOLD_BYTES const * code review fixes * code climate fixes * style fixes --------- Co-authored-by: Anton Lamtev <lamtev@users.noreply.github.com>
No description provided.