Skip to content

AMQP keepalive support #372

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

AMQP keepalive support #372

wants to merge 5 commits into from

Conversation

vyachin
Copy link

@vyachin vyachin commented Mar 20, 2020 •
edited
Loading

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues

Если перед кластером rabbitmq стоит балансер (у нас это yandex cloud load balancer) то периодически consumers завершались с ошибкой "Connection timed out". Активация heartbeat режима, без переписывания обработчиков не поможет php-amqplib/RabbitMqBundle#301

Нам помогло включение режима keepalive на стороне consumers.

Для возможности включения режима keepalive через конфиг и был создан данный PR

@samdark samdark added this to the 2.3.1 milestone Mar 20, 2020
@samdark samdark added type:enhancement Enhancement status:code review The pull request needs review. labels Mar 20, 2020
@samdark
Copy link
Member

samdark commented Mar 20, 2020 •
edited
Loading

https://travis-ci.com/github/yiisoft/yii2-queue/jobs/300286412 that's interesting... likely not related to pull request but can't merge without this error fixed :(

samdark
samdark requested changes Mar 20, 2020
View reviewed changes
@samdark samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Mar 20, 2020
@vyachin vyachin force-pushed the ampq-keepalive-support branch 4 times, most recently from 3853dde to b8950e4 Compare March 21, 2020 01:01
@samdark
Copy link
Member

samdark commented Mar 23, 2020

Any idea about these random failures?

@vyachin vyachin force-pushed the ampq-keepalive-support branch 3 times, most recently from acaca1d to 14a79d5 Compare April 29, 2020 11:53
@samdark
Copy link
Member

samdark commented Apr 29, 2020

Works? Ready for review?

@vyachin
Copy link
Author

vyachin commented Apr 29, 2020 •
edited
Loading

Тесты рандомно падают на проверке работы приоритетов для драйвера rabbitmq, если rabbitmq запущен через докер.

Если запустить rabbitmq через раздел services, возникает другая проблема - rabbitmq не установлен в образе ubuntu xenial и поддержка travis говорит, что его нужно устанавливать вручную в этом образе.

php 5.5 нет в образе ubuntu xenial, придется использовать для тестирования образ trusty.

Раздел before_install для xenial и trusty получается разный. Путем проб и ошибок самый простой вариант - это использовать для всех тестов дистрибутив trusty

плюсы:

  • единый раздел before_install
  • поддержка php 5.5, 5.6, 7.0, 7.1, 7.2, 7.3
  • rabbitmq подключается через раздел services
  • все тесты работают (или не работают) стабильно

минусы:

  • старый дистрибутив, возможны проблемы с поддержкой
  • непонятно как он себя поведет с php 7.4

@vyachin
Copy link
Author

vyachin commented Apr 29, 2020

готово

@vyachin vyachin force-pushed the ampq-keepalive-support branch from 14a79d5 to 14330ac Compare April 29, 2020 15:06
@samdark samdark added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Apr 29, 2020
@samdark samdark requested a review from a team April 29, 2020 15:56
samdark
samdark requested changes May 1, 2020
View reviewed changes
composer.json Outdated
@@ -23,7 +23,7 @@
"require-dev": {
"yiisoft/yii2-redis": "*",
"php-amqplib/php-amqplib": "*",
"enqueue/amqp-lib": "^0.8||^0.9.10",
"enqueue/amqp-lib": "*",
Copy link
Member

Choose a reason for hiding this comment

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

That's kind of unsafe default since it will update to major versions without us knowing that. Is it possible to specify a range explicitly?

Copy link
Author

Choose a reason for hiding this comment

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

Нормально встают все зависимости только при наличии enqueue/amqp-lib версии 0.8.* Версии enqueue/amqp-lib 0.9.* или 0.10.* начинают конфликтовать с ограничениями для "enqueue/stomp": "^0.8.39"

Copy link
Member

Choose a reason for hiding this comment

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

Конфликт на уровне composer или прямо в коде?

@samdark
Copy link
Member

samdark commented May 1, 2020

Overall everything's well done 👍 Let's fix minor phpdoc issues and version constraint. Then I'll merge it.

@samdark samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels May 1, 2020
@samdark samdark added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels May 7, 2020
@samdark samdark modified the milestones: 2.3.1, 2.3.2 Dec 23, 2020
@samdark samdark removed this from the 2.3.2 milestone Oct 23, 2021
@iglooom
Copy link

iglooom commented Sep 16, 2022

How about merge it? =)

@arogachev
Copy link
Contributor

arogachev commented Sep 21, 2022 •
edited
Loading

Need to resolve conflicts first. Travis was replaced by Github Actions so adaptation for these changes is required.

@s1lver
Copy link
Member

s1lver commented Jan 11, 2023

Came across the same problem. Can this PR be updated?

s1lver added a commit to s1lver/yii2-queue that referenced this pull request Jan 12, 2023
@s1lver
(yiisoft#372) Add ability to configure keepalive and heartbeat for AM…
fcfd126 
…QP and AMQP interop
@s1lver s1lver mentioned this pull request Jan 12, 2023
Merged
@samdark samdark closed this Jan 13, 2023
samdark pushed a commit that referenced this pull request Jan 13, 2023
@s1lver
Fix #372: Add ability to configure keepalive and heartbeat for AMQP a…
cd7c266 
…nd AMQP interop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samdark samdark samdark approved these changes

Assignees
No one assigned
Labels
status:code review The pull request needs review. type:enhancement Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vyachin @samdark @iglooom @arogachev @s1lver