Skip to content
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

Consumer lib #44

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Consumer lib #44

wants to merge 4 commits into from

Conversation

plazmoid
Copy link
Contributor

No description provided.

Copy link
Contributor

@a-kordys a-kordys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Резюмируя:

  1. Кажется, что описываемый скоуп слишком широкий для одной библиотеки. Как минимум, задачи парсинга blockchain updates, работы с общей частью базы и чтения конфигов - это три совершенно разные и непересекающиеся задачи для трёх разных библиотек.
  2. Задача работы с общей частью базы кажется наиболее сложной из этих трёх, в рамках существующих подходов (т.е. совместно с библиотеками diesel и diesel-migrations). Альтернатива в виде sqlx тоже не лишена фатальных недостатков.

Comment on lines +6 to +10
- значительно сократить объёмы повторяющегося кода, а, значит, и время на его поддержку
- унифицировать интерфейсы и шаблоны проектирования консьюмеров
- внедрять новые фичи и исправлять баги быстрее и лучше (в конечных консьюмерах потребуется лишь
переход на новую версию библиотеки с возможными незначительными правками кода)
- избавиться от legacy-кода
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Общие слова, бездоказательные утверждения.

По опыту использования библиотеки api-clients, стало крайне неудобно добавлять новый функционал, а делать это приходится довольно часто (чаще чем хотелось бы). Весь legacy код как был так и остался, просто переехал в другое место. Внедрять новые фичи и исправлять баги стало менее удобно. Переход на новую версию не всегда бесшовный (часто рушатся существующие интерфейсы, т.к. все плевали на semver, также вспоминается проблема сборки из-за конфликта версий зависимости, вроде qs_serde или что-то в этом роде было, фиксили больше дня насколько помню).

В общем, хочется побольше конкретики, как именно оно станет лучше, и желательно опираясь на существующий опыт текущих библиотек.


# Фичи/Фиксы
## Общая логика консьюмера
Получение сырых данных из blockchain updates, их парсинг и сохранение в БД.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Про сохранение в БД не раскрыто. Кажется, что это будет наибольшая из проблем. Я не вижу, как можно Diesel и его миграции натянуть на два проекта (библиотека+приложение), чтобы всё работало. Может, какой-то игрушечный proof-of-concept проект запилить сначала? Где будут лежать миграции? В библиотеке? В приложении? И там и там? Если последнее - то как их смержить? Как задавать опциональные части в общих таблицах? Как здесь написано ниже, "Из blockchain updates приходит множество разных структур данных: блоки, ассеты, тикеры, транзакции и т.д.. В большинстве случаев необходима только часть из них" - как это утверждение ляжет на миграции для общих таблиц? Всегда создавать по максимуму? Генерить sql-миграции на лету и исполнять их в обход Diesel Migrations? Пока больше вопросов чем ответов.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я бы тоже начал с решения вопроса про базу. Без этого ничего не получится.

Может, какой-то игрушечный proof-of-concept проект запилить сначала
Хорошая идея. Надо собраться, описать скоуп для этого прототипа — что он должен уметь.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Консьюмер без Service почти не нужен вопрос про модели не раскрыт часто модели лежат и в сервисе и консьюмере они похожи, но не одинаковые иногда нужно переносить и/или дублировать и т.п. или наоборот соединять в одну и непонятно где её держать.



## Возможность выбора только нужных структур данных
Из blockchain updates приходит множество разных структур данных: блоки, ассеты, тикеры, транзакции и т.д.. В большинстве случаев необходима только часть из них, поэтому предлагается механизм экстракции и обработки нужных из всех возможных, представленных в либе.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Непонятно, как создавать/исполнять миграции для базы, основываясь на потребностях собираемого приложения.



Дефолтная логика обработки блоков в либе не всегда подходит конечному консьюмеру. Если требуется дополнительный код, можно написать свой экстрактор/хендлер для нужного типа, оверрайдящий дефолтное поведение.
Пример: экстракторы в пулах.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А что за проблема в пулах? Хотелось бы чуть подробнее, не все наверно в теме.

Пример: экстракторы в пулах.

## Взаимодействие с БД
Каждый консьюмер имеет трейт `Repo` (`RepoOperations`), содержащий методы для работы с моделями, нужными конкретно этому консьюмеру. Вместо создания в либе одного `RepoOperations`, содержащего всевозможные методы для всех моделей (имплементация которого невозможна только для части моделей в конечном консьюмере), предлагается api из нескольких трейтов, содержащих методы только для конкретного типа. Пример:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кажется, что написать универсальные трейты, учитывающие все потребности, не реально.

### Предлагаемая реализация
Два варианта интерфейса:
- отдельный бинарь
- `--rollback-to=12345` в аргументах бинаря консьюмера
Copy link
Contributor

@a-kordys a-kordys Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наверно, это не самый лучший вариант.

Аргументы против:

  1. Сейчас мы нигде вообще не парсим командную строку. Выглядит как новая сущность в наших проектах.
  2. Код запуска консюмера станет сложнее и запутаннее, с постоянными вызовами в библиотеку.
  3. При запуске из CI без разницы как выглядит командная строка, длинно или коротко.
  4. При запуске руками из консоли отдельный бинарь - короче.
  5. Миграции сделаны отдельным бинарём, почему бы и тут не сделать так же.

Аргументы за:

  1. На стороне консюмера меньше телодвижений в исходниках (не нужно прописывать отдельный бинарник).

- `--rollback-to=12345` в аргументах бинаря консьюмера

## Чтение конфигов
Опциональный модуль для чтения переменных окружения.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кажется, что это вообще отдельная тема. Отдельная вообще библиотека может быть, которая вообще никак не пересекается с перечисленным выше.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это отличный вопрос, но надо его вынести куда-то в отдельное место. Например, в задачу в JIRA.

```

## Хуки
Консьюмеры, помимо основных задач, могут иметь дополнительный код, отрабатывающий в главном цикле. Для его регистрации предлагаются следующие виды хуков:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для чего это может быть полезно? Есть примеры текущих проектов, где это необходимо? Проблема с консюмеров в пулах, упомянутая выше - это из этой области или нет?

- `post-rollback`

# sqlx
Возможна замена дизеля на sqlx, поскольку с усложнением sql-запросов в ORM возрастает когнитивная нагрузка, сообщения об ошибках типов дизеля становятся километровыми и малопонятными. Sqlx же позволяет писать на обычном sql, синтаксис которого проверяется в компайл-тайме с помощью макросов, а также десериализовывать результаты запроса сразу в нужные структуры вместо кортежей.
Copy link
Contributor

@a-kordys a-kordys Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Про sqlx выше написал. В двух словах - на бумаге он выглядит гораздо лучше, чем на деле.
Да, Diesel не идеален, но кажется что именно sqlx не даёт чего-то большего, не привнося своих серьёзных проблем.
Конкретно:

  1. Проверка sql-запросов в compile-time с помощью макросов работает исключительно в одном самом простом случае: если запрос представляет собой строковый литерал. Любая конкатенация, опциональные части и прочее подобное - всё, проверки нет.
  2. Даже такая простая и часто используемая вещь, как sql-оператор IN (...) в sqlx не реализована - у них об этом даже в readme написано, типа не удалось сделать кросс-платформенно. Поэтому тут либо динамический запрос, либо костыли с массивами, либо ещё что-то. Проверки в compile-time не будет.
  3. Десериализуются результаты запросов в анонимные структуры, генерируемые внутри макроса. Структуры анонимные, со всеми вытекающими проблемами: нет имени типа, значит нельзя объявить переменную/поле/параметр, передать в функцию для обработки нельзя, только на месте разбирать.
  4. Поддержки со стороны IDE нет. Имена полей этой структуры - только вслепую, у меня в IDE поддержка этого не завелась даже если все экспериментальные фичи включить (фичи cargo.build.scripts и cargo.proc.macros в IDE), причём для процедурных макросов из prost/tonic да и того же дизеля это работает, а тут - нет).

Резюмируя, я теперь уже довольно скептически смотрю на sqlx, по сравнению с тем, как это было до того как я попробовал.

# sqlx
Возможна замена дизеля на sqlx, поскольку с усложнением sql-запросов в ORM возрастает когнитивная нагрузка, сообщения об ошибках типов дизеля становятся километровыми и малопонятными. Sqlx же позволяет писать на обычном sql, синтаксис которого проверяется в компайл-тайме с помощью макросов, а также десериализовывать результаты запроса сразу в нужные структуры вместо кортежей.

Для использования таких макросов в IDE или в `.env` должна быть поднята БД с миграциями и установлена энва `DATABASE_URL`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

И это, кстати, тоже серьёзное неудобство sqlx.

Везде, где запускается сборка проекта, везде где надо cargo build или cargo check - нужна полноценная база, чтобы до неё был доступ. Это и в CI-среде (у нас из build-окружения есть доступ хотя бы к dev-инфраструктуре?), и локально при разработке.

На данный момент нет возможности роллбэкнуться до нужной высоты иначе, кроме как через вызов sql-процедуры.

### Предлагаемая реализация
Два варианта интерфейса:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Есть ещё вариант — делать роллбэк по сетевому запросу на админский эндпоинт в какой-нибудь наш инфраструктурный порт 9090.

Запрос должен gracefully запаузить консьюмер, откатить, и продолжить. Параллельные запросы надо, конечно, запретить.

Этот вариант имеет ряд удобств для devops-инженеров. Я бы обсудил.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тогда было бы неплохо для начала отрефакторить то что у нас есть и поменять нейминг на более подходящий.

То что сейчас называется MetricsWarpBuilder и также всякие METRICS_PORT и прочее должно уже называться как-то более общо, InfrastructureWebService или что-то такое в этом духе, должен появиться интерфейс добавления не-метриковых эндпоинтов к этому инстансу варпа.

Но в целом да, идея вполне здравая. Я бы обсудил тоже.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants