init commit#1
Conversation
avhitrov
left a comment
avhitrov
left a comment
There was a problem hiding this comment.
Почти все хорошо, кроме нескольких недочетов и одного архитектурного косяка в виде синхронной работы с черным ящиком. Все отметил в комментариях к ПР.
Попытка сделать списание и начисление транзакционно безопасными будет похвальна. Если не получится - акцентировать свое внимание на DATA RACE не буду.
Работу с черным ящиком необходимо переделать.
| if errAcc := gm.calcAccrualForOrder(r, customer, order); errAcc != nil { | ||
| gm.logger.Warn(errAcc.Error()) | ||
| } |
There was a problem hiding this comment.
Ставить быстродействие и надежность работы сервера в зависимость от "черного ящика" - идея глубоко порочная.
- Данные о заказах в черный ящик могут поступать с опозданием
- Черный ящик может лежать
- Черный ящик может очень долго отвечать, таймаут ее работы может пагубно влиять на ответы сервера
- Черный ящик может отвечать 429 ошибкой (приходите позже), и этот ответ сервер не обрабатывает вообще никак. Точнее, во всех случаях посылает пользователя с ошибкой.
Требуется fail-safe решение в виде отдельного процесса (горутины), который в асинхронном формате будет переопрашивать ящик и скармливать ему еще не обработанные заказы (статус NEW и PROCESSING) вплоть до получения определенности по заказу (подтвержден, отклонен).
Остальные функции должны уметь работать со статусами и не списывать деньги с неподтвержденного заказа. Соответственно, пересчет пользовательского баланса тоже уходит в асинхронный процесс.
There was a problem hiding this comment.
- Писать полноценный шедулер с очередью не описано в ТЗ, ТЗ говорит, что загруженный заказ должен загрузиться и по нему должен в сервисе accrual сделаться рассчёт, а клиенту возвращён ответ, принцип взаимодействия не описан, условия тоже не описаны, наверное оно подразумевалось, но в ТЗ не попало.
- Там передаётся контекст
- Я пытался делать через горутину, но возникает вопрос в автотестах, которые вряд ли учитывают асинхронность работы с чёрным ящиком, поэтому тут два варианта, либо писать что-то полноценное либо подгонять результат под автотесты, я выбрал подгонять под автотесты, другого не надо ибо код-ревью только при условии прохождения этих автотестов.
There was a problem hiding this comment.
Что же, опустимся до уровня ТЗ. В соответствии с ним
Заказ может быть взят в расчёт в любой момент после его совершения. Время выполнения расчёта системой не регламентировано. Статусы INVALID и PROCESSED являются окончательными.
Соответственно, статусы REGISTERED и PROCESSING являются временными статусами. Не вижу никакой реакции на получение этого статуса.
Кроме того, из ТЗ следует, что списание можно проводить только для заказов, имеющих статус PROCESSED. Не вижу в коде реализации этого условия.
Кроме того, черный ящик может ответить статусом 429, формат ответа:
429 Too Many Requests HTTP/1.1
Content-Type: text/plain
Retry-After: 60
No more than N requests per minute allowed
Статус предлагает вернуться к системе расчета через 60 секунд (заголовок Retry-After). Этот отлуп, равно как и 500, не является отлупом пользователю, отправимшему заказ, а лишь техническое предложение прийти за ответом попозже.
Мне, в общем, все равно, как будут реализованы требования ТЗ - полноценным ли шедулером, простой горутиной, перелопачивающей базу в одно сопло, либо же каким-то еще способом. Могу сказать, что автотесты вполне расчитаны на асинхронную работу с черным ящиком.
поэтому тут два варианта, либо писать что-то полноценное либо подгонять результат под автотесты, я выбрал подгонять под автотесты, другого не надо ибо код-ревью только при условии прохождения этих автотестов.
Никто не требует продового решения. А условие прохождения автотестов является лишь допуском до ревью, но совершенно не обязывает ревьюера принимать работу, не реализующую ТЗ.
avhitrov
left a comment
avhitrov
left a comment
There was a problem hiding this comment.
По гонкам в данных все принимается. Осталось реализовать недоделанные требования пунктов ТЗ.
avhitrov
left a comment
avhitrov
left a comment
There was a problem hiding this comment.
Хорошая реализация. Мои поздравления с завершением первой дипломной работы.
| acc.logger.Error(err.Error()) | ||
| return nil, err | ||
| } | ||
| defer w.Body.Close() |
There was a problem hiding this comment.
| defer w.Body.Close() | |
| w.Body.Close() |
defer имеет смысл только при наличии еще каких-то вычислений (с возможным экстренным выходом по ошибке) между его определением и концом функции. Поставленный перед концом функции он лишь добавляет временное пенальти для создания отложенной функции.
2 participants
No description provided.