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

added-rebbitMQ Fixtures Logger #1

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

added-rebbitMQ Fixtures Logger #1

wants to merge 1 commit into from

Conversation

AlexSolonin
Copy link
Owner

Добрый день, Александр! Вроде удалось Вас пригласить в проект :)
Как будет время, посмотрите пожалуйста код, мне важно Ваше мнение.
К сожалению, на этих выходных много работы и заняться проектом было мало времени, но кое что всё же сделал.
Добавил фикстуры, чтоб вручную не заполнять таблицы, добавил логгер в нескольких местах, но пока не научился разделять логи по файлам.
Подскажите, правильная идея: реализовать очереди для выбора типа и количества приза? Я сделал их на RabbitMQ и даже работает ))). Теперь все запросы будут выполняться друг за другом и гонки не произойдёт. Это правильное утверждение?
Транзакции: правильно я понимаю, что в одну транзакцию нужно добавлять сеттер em из выбранного блока case (GiftService: строка 97, 134) и запись данных в таблицы Winning и Transfer (GiftService: строки 143-156) и после этого выполнять её? Если что-то пойдёт не так - всё вернётся как и было.
Паттерн "стратегия" пока не реализован, но я нашёл кучу статей и обязательно разберусь с этим ))).
Заранее спасибо за rewiev! )

Copy link
Collaborator

@actofgod actofgod left a comment

Choose a reason for hiding this comment

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

В общем то, если хотелось измежать состояния гонки, то не получилось, так как воркеров может быть не один, а несколько, и тогда снова встанет та же проблема, если два воркера получат одну и туже задачу почти одновременно :) Тут именно транзакция нужна или лок какого-то ресурса, но так как используется БД, то логичнее всё-таки транзакция.
Ну и я не совсем понимаю, зачем тут очередь, она отлично бы подошла для отправки призов из консоли, а вот для генерации приза непонятно, плюс, если воркер подвис, каким образом нотифицировать пользователя о том, какой приз ему был сгенерирован? Ну то-есть пользователь нажал кнопку сейчас, а воркер отработал через 15 секунд, что будет?

@@ -23,5 +23,11 @@ services:
resource: '../src/Controller'
tags: ['controller.service_arguments']

app.consumer.gift_select:
class: App\Service\ConsumerService
autowire: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Если services._default.autowire = true, то тут указывать флаг не нужно.
В случае, если дефолтное значение поменяется, то скорее всего большая часть проекта упадёт, и переживать об этом не нужно :)

@@ -127,7 +129,7 @@ public function personal(Request $request, GiftService $giftService, ConvertServ
);

if (isset($_POST['user_id']) && isset($_POST['start'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут непонятно, раз используется Request, то стоило бы обратиться к корзине $request->request а не к глобальному массиву.
Я бы вообще, на месте разработчиков симфони, делал бы $_GET = $_POST = $_FILES = []; после инициализации объекта запроса :)

@@ -127,7 +129,7 @@ public function personal(Request $request, GiftService $giftService, ConvertServ
);

if (isset($_POST['user_id']) && isset($_POST['start'])) {
$giftService->selectGift($queryUser->getId());
$producer->publish(json_encode($queryUser->getId()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Это должно быть в сервисе (как и вытаскивание пользователя из репозитория ;-) )
Вообще, можно было второй сервис замутить, даже возможно просто унаследовав его от текущего гифт сервиса, и в нём в брокера подсунуть сообщение. Тогда можно было бы просто поменяв одну строчку в конфиге (выбрав сервис) изменить логику работы выдачи призов, либо на лету, либо через кролика

'name' => 'bank_api_key',
'value' => 2598756215644
],
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

указание ключей тут не нужно


public function __construct(
ParameterBagInterface $params,
LoggerInterface $logger)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

по PSR-12

When the argument list is split across multiple lines, the closing parenthesis and opening brace MUST be placed together on their own line with one space between them.

'amount' => 10,
'description' => 'Alfa Romeo'
],
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

не нужны ключи

@@ -53,7 +60,7 @@ public function sendPost(array $params)
]
);
} catch (GuzzleException $e) {
echo $e->getMessage();
$this->logger->info('bank api info: ' . $e->getMessage());
Copy link
Collaborator

Choose a reason for hiding this comment

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

отлично, только это не info 100%, а либо предупреждение, либо ошибка, я бы error отправлял

return true;
}

}
Copy link
Collaborator

@actofgod actofgod Nov 17, 2019

Choose a reason for hiding this comment

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

Открытие/закрытие соединения с БД не совсем хорошая идея.
Вообще в чём смысл брокеров очередей - что-то нужно обработать в бэкграунде, ставим на обработку каким-то образом в очередь задачу на обработку. С другой стороны очереди висит обработчик, возможно не один, который при возникновении в очереди события его вытаскивает, понимает что нужно сделать, делает, и снова слушает очередь. Тоесть, по хорошему, у воркера все коннекты, сервисы и т.д. должны быть инициализированы всегда, чтобы максимально быстро обработать задачу и приняться за новую.

public function __construct(EntityManagerInterface $em,ParameterBagInterface $params)
public function __construct(
EntityManagerInterface $em,
ParameterBagInterface $params)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

не по PSR-12 )

@AlexSolonin
Copy link
Owner Author

Александр, добрый вечер! Спасибо за замечательное описание! Эт очень важно для меня. Как раз таких ревью и не хватает. А тут не только все ошибки описаны но и объяснения есть. Буду учиться дальше!!! :)

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.

2 participants