Code review (sprint 10)

BigDeepBlue commented 1 year ago

Здравствуйте ! Отлично поработали ! Есть несколько рекомендаций и замечаний:

Так как сервис оповещений достаточно обширный нужно нарисовать схему взаимоотношений сервисов внутри него, подобно как Вы делали для UGC сервиса. И внести его на общую схему сервисов входящих в кинотеатр.
Хорошо бы добавить отдельный подробный README в сам сервис нотификаций с описанием как все запустить (и возможно пояснения о взаимоотношениях сервисов).
Тут не нашел сервиса который джанго админка. Получается он не входит в число микросервисов относящихся к notification ?
Для редактирования темплитов в джанго админке могу предложить вот такую штуку https://github.com/summernote/django-summernote WYSIWYG редактор (легко достаточно интегрируется) и получается пользователь может создавать темплейт сообщения комфортно (не просто скучное текстовое поле :)) (но это необязательно, но возможно Вам понравится и подключите)
Тут думаю нужно подготовить джанго приложение подобно тому как вы делали в начальных спринтах - чтоб оно конфигурировалось для прод окружения и запускалось в стеке сервисов. (так как по сути это все же часть notification сервиса). Также БД можно взять не sqlite, ну или позаботиться о хранении на постоянном томе (чтоб при перезапуске контейнера БД не терялась).
Тут получается работа над этой штукой еще не окончена ?
Тут и тут пустые файлики можно убрать.
Тут здесь неиспользуемые импорты.
Тут не думаю что нужно так жестко определять порт, ну пусть приезжает из настроек ) Можно определить ему дефолтное значение например, зачем так выносить в константу ? )
Тут импорт requests - но здесь в файле зависимостей библиотечки нет - https://github.com/AndreiUkladchikov/YandexPracticumTeam/blob/79faaf51c4416156cb28d99c9432511cd4411b2a/notification/etl_worker/requirements.txt#L1
Тут вполне возможно нужно приводить к bytes так как в документации https://pika.readthedocs.io/en/stable/modules/channel.html#pika.channel.Channel.basic_publish body ожидается bytes.
Тут немного странное использование конструктора экземпляра. Работать это конечно будет, но я бы советовал сделать какой то говорящий метод вроде run(), если не нужна предварительная инициализация параметрами - можно сделать метод статическим и вызывать BaseMessageRender.run(). По сути у вас все методы класса могут быть статическими, у вас ведь нет обращений через self к атрибутам экземпляра. Еще момент часто слово Base используют чтоб обозначить некий базовый класс от которого что то будет наследоваться, а в данном случае это может слегка сбивать тех кто будет исследовать Ваш код, так как класс по сути не является ни для чего базовым :)
Тут если возникнет исключение выше, то поток выполнения не дойдет до вызова закрытия соединения. Используйте try-except-finally и закрывайте гарантированно в finally блоке.
Интересно почему выбрали синхронную модель для ETL worker сервиса (и sender) ? (можно в коментах к тикету отписать, интересно ведь асинхронная модель легла бы сюда неплохо)
Тут вы создаете подключение к SMTP cервису но нигде не закрываете его, очевидно оно должно закрываться как в случае ошибки так и по завершению работы.
Тут так как в принципе в перехватчиках исключений происходит одно и тоже - можно их объединить вроде этого (документация)
```
except (smtplib.SMTPRecipientsRefused, smtplib.SMTPSenderRefused):
  logger.exception("Error during send message.")
  rabbit_producer(settings.death_queue, self.email)
```
Не нашел тестов относящихся к сервисам нотификаций, а ведь есть API сервис в котором можно покрыть ручки тестами. И остальные сервисы etl и sender должны иметь тесты.

AndreiUkladchikov commented 1 year ago

Добрый день! Спасибо за подробное ревью!

Исправили: https://github.com/AndreiUkladchikov/YandexPracticumTeam/pull/152 - пункт 6 https://github.com/AndreiUkladchikov/YandexPracticumTeam/pull/154 - пункты: 1 - дополнена uml схема и добавлена отдельная для сервиса уведомлений (также можно посмотреть в виде картинок в out папке) 2 - дополнили общий Readme и добавили отдельный для сервиса уведомлений 3 - упаковали сервис (пока оставили sqlite - но указали volume в compose файле) 5 - через split_settings разделили конфиг файл 7, 8, 9 - исправили

rostsul commented 1 year ago

Добрый день!

14 - Выбрали синхронную модель, так как воркер относительно универсальный и масштабировать планировали горизонтально в дальнейшем разделяя сообщения на тематические очереди и подымая нужное количество реплик воркера под очередь. В целом в перспективе с ростом нагрузки да необходимо будет перейти на асинхронную модель. 10, 11, 12, 13 - исправили

AndreiUkladchikov commented 1 year ago

https://github.com/AndreiUkladchikov/YandexPracticumTeam/pull/155 - 15, 16, 17

BigDeepBlue commented 1 year ago

Молодцы ! Много подправили ! Вот еще немного рекомендаций:

Тут переменную можно принимать из переменных окружения.
Тут не нужен отступ, это свойство "верхнего уровня" :)
```
volumes:
static_volume:
```

AndreiUkladchikov / YandexPracticumTeam

Code review (sprint 10) #151