feat: controller cash#173
Conversation
KonstantinKondratenko
left a comment
KonstantinKondratenko
left a comment
There was a problem hiding this comment.
Нужно исправлять, что я описал + не забывать про метрики -- задачку нужно будет на себя взять (я думаю, что идейно в другом PR лучше делать)
| req.WorkerId, req.Type, req.Target) | ||
|
|
||
| categoryIDs, err := s.classifier.Check(req.Target, req.Type) | ||
| cashRequest, err := s.storage.GetRequestHash(ctx, req.Target) |
There was a problem hiding this comment.
почему кэш контракт нарушается, у нас же
message ClassifyRequest {
uint64 worker_id = 1;
string type = 2;
string target = 3;
}
тип и таргет -- а у вас только таргет
Причем ниже же categoryIDs, err = s.classifier.Check(req.Target, req.Type)
сделайте отдельную функцию по типу makeClassifyCacheKey, которая будет использоваться и в Save, и в Get
| return err | ||
| } | ||
|
|
||
| return c.db.Set(ctx, key, data, 0).Err() |
There was a problem hiding this comment.
почему нет TTL, у нас же нет концепции вечного кэша
| TrustLevel: 0, | ||
| }, nil | ||
| categoryIDs, err = s.classifier.Check(req.Target, req.Type) | ||
| if err != nil { |
There was a problem hiding this comment.
если я правильно понял логику, то все ошибки редиса классифицируются как промах кэша
правильно было бы разделить на
- реальный промах
- ошибка редиса
иначе мы будем некорректно интерпретировать результаты
| }, nil | ||
| } | ||
|
|
||
| err = s.storage.SaveRequestHash(ctx, storage.RequestCash{ |
There was a problem hiding this comment.
если внешний провайдер временно будет недоступен, то мы в classifier.Check не кидаем ошибку наверх, а просто логируем ее и продолжаем -> что мы сохраняем в кэш?
Нам нужно разбирать ошибки по типам:
- если ошибка внешних сервисов, то ничего не кэшируем
- если результат unknown, то можно кэшировать на меньший TTL например
- если нормальная классификация прошла, то кэшируем на нормальный ttl
| SERVER_PORT=50051 | ||
| MAX_MESSAGE_SIZE=4194304 No newline at end of file | ||
| MAX_MESSAGE_SIZE=4194304 | ||
| REDIS_PASSWORD=mysecretpassword |
There was a problem hiding this comment.
почему контроллер при обращаениях к БД не использует креды?
давайте их или уберем или будет нормальный запросы делать с кредами смысл делать креды а потом default on nopass
There was a problem hiding this comment.
я подумал, что убрать креды -- плохая мысль -- работаем по паролю но честно по паролю
| echo "requirepass $REDIS_PASSWORD" >> /usr/local/etc/redis/redis.conf && | ||
| echo "appendonly yes" >> /usr/local/etc/redis/redis.conf && | ||
| echo "appendfsync everysec" >> /usr/local/etc/redis/redis.conf && | ||
| echo "user default on nopass ~* +@all" > /usr/local/etc/redis/users.acl && |
There was a problem hiding this comment.
почему?
Это опасно, если мы хотим, чтобы разработкой пользовались, нужно ее аккуратно писать, а не оставлять такие дыры.
и сразу нормальный контракт взаимодействия описать
| build: | ||
| dockerfile: Dockerfile.controller | ||
| environment: | ||
| - REDIS_ADDR=redis:6379 |
There was a problem hiding this comment.
а почему оно не в .env???
вынесите в .env файл
| func init() { | ||
| err := gotenv.Load() | ||
| if err != nil { | ||
| log.Fatalf("Error to load env: %v", err) |
There was a problem hiding this comment.
почему фатальная ошибка?
А если я передам переменные руками без .env файле
или какой-то хитрый оркестратор подсунет переменные и без env файла? Кажется что ломаться нам не стоит на данном этапе -- мб имеет смысл грохнуться с ошибкой когда какая-то жизненно важная переменная не задана, но тут излищне
There was a problem hiding this comment.
а вообще нужн ли тут init?
| redis, err := storage.NewRedisClient(ctx, config) | ||
|
|
||
| if err != nil { | ||
| log.Fatalf("Error to create redis storage: %v", err) |
There was a problem hiding this comment.
почему фатально опять?
У вас же функция может прокинуть ошибку через error -- и должна по логике именно так и поступить, а сверху ее уже как-то обрабатывать
| data = [ | ||
| "//internal/service/config", | ||
| ], |
2 participants
No description provided.