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.

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

Add separate transport option for retry loop timeout #1599

Merged
merged 7 commits into from
Oct 12, 2022
Merged

Add separate transport option for retry loop timeout #1599

merged 7 commits into from
Oct 12, 2022

Conversation

intgr
Copy link
Contributor

@intgr intgr commented Sep 27, 2022

Fixes #1594. This only applies when using Connect.default_channel.

Before this change, the retry loop timeout was set equal to TCP connect timeout (connect_timeout), meaning when first connection attempt timeouted, no retry would be attempted.

Now, if a new transport option connect_total_timeout is provided, this overrides connect_timeout for the retry loop (but not for TCP connect).

@intgr intgr mentioned this pull request Sep 27, 2022
Closed
@intgr
Copy link
Contributor Author

intgr commented Sep 27, 2022

Connection._ensure_connection() gets called from many different places with different sets of arguments, but for some reason transport_options only apply when using Connect.default_channel. So I didn't touch other call paths.

I tested that this solves my issue when using Celery.

auvipy
auvipy requested changes Sep 27, 2022
View reviewed changes
kombu/connection.py Outdated Show resolved Hide resolved
@intgr
Copy link
Contributor Author

intgr commented Sep 27, 2022

Also I'm not sure about the naming of connect_total_timeout transport option, ideas welcome.

Alternatives I could come up with: connect_retry_timeout, connect_retry_total_timeout, connect_retry_loop_timeout, total_connect_timeout...

auvipy
auvipy reviewed Sep 29, 2022
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

how does connect_retry_timeouts sound?

@auvipy auvipy added this to the 5.3 milestone Sep 29, 2022
@intgr intgr force-pushed the add-separate-timeout-for-connect-retry-loop branch from d6f12ed to e874d06 Compare October 3, 2022 15:49
@intgr
Copy link
Contributor Author

intgr commented Oct 3, 2022

Sorry I had missed your comment. I have renamed the setting to connect_retry_timeout.

Or did you mean connect_retry_timeouts in plural? Having "timeouts" in plural seems weird to me, there is one timeout for multiple retries. Maybe connect_retries_timeout then?

@intgr intgr requested a review from auvipy October 12, 2022 07:27
auvipy
auvipy requested changes Oct 12, 2022
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

Maybe connect_retries_timeout then seems ok. but the tests are failing. can you recheck please?

@intgr
Add separate transport option for retry loop timeout
68d1400 
This only applies when using `Connect.default_channel`.

Before this change, the retry loop timeout was set equal to TCP connect timeout (`connect_timeout`), meaning when first connection attempt timeouted, no retry would be attempted.

Now if a new transport option `connect_total_timeout` is provided, this overrides `connect_timeout` for the retry loop (but not for TCP connect).
@intgr
Add tests
fa5d129
@intgr
Fix isort
32d2e1d
@intgr
Rename to connect_retry_timeout
0f3adf6
@intgr
Reformat
c16aacb
@intgr
connect_retry_timeout -> connect_retries_timeout
f46918e
@intgr intgr force-pushed the add-separate-timeout-for-connect-retry-loop branch from e874d06 to f46918e Compare October 12, 2022 08:37
@intgr
Copy link
Contributor Author

intgr commented Oct 12, 2022

Maybe connect_retries_timeout then seems ok.

Renamed to connect_retries_timeout

but the tests are failing. can you recheck please?

The failures don't seem to be related to my changes. Let's see if it gets resolved with a rebase.

@intgr
Copy link
Contributor Author

intgr commented Oct 12, 2022

@auvipy Can you approve the workflows so we can see if the test issue is fixed?

@auvipy auvipy merged commit 3a49533 into celery:master Oct 12, 2022
@intgr intgr deleted the add-separate-timeout-for-connect-retry-loop branch June 26, 2023 08:18
This was referenced Jun 19, 2024
Open
Closed
@sschiessl-bcp sschiessl-bcp mentioned this pull request Jun 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auvipy auvipy auvipy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.

Unable to configure connect_timeout and failover at the same time
2 participants
@intgr @auvipy