search

celery / kombu

Messaging library for Python.
http://kombu.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
2.82k stars 920 forks source link

Passing transport_options to conn_opts in _extract_failover_opts #1223

Open sschiessl-bcp opened 3 years ago

sschiessl-bcp commented 3 years ago

Is there a reason I am missing why timeout option is not passed forward in _extract_failover_opts?

https://github.com/celery/kombu/blob/75027490c71b83342f92b5293461c679233dc25e/kombu/connection.py#L835-L847

Reference: https://github.com/celery/celery/issues/5067#issuecomment-656523289

thedrow commented 3 years ago

I don't see why it shouldn't. Feel free to submit a PR and we'll test it out.

sschiessl-bcp commented 3 years ago

I don't see why it shouldn't. Feel free to submit a PR and we'll test it out.

done

matusvalo commented 3 years ago

@sschiessl-bcp thank you for your issue. I have few notes connected to this issue:

  1. default_channel property has special semantics different from connect() and channel() methods - it blocks until connection is made by default (channel()/connect() fails with exception)
  2. this blocking until connection is created is done by method _ensure_connection() which is used only in default_channel and hence point 1.
  3. point 1. and 2. were causing in celery https://github.com/celery/celery/issues/5067
  4. Issue https://github.com/celery/celery/issues/5067 was fixed by adding transport_options to _ensure_connection(): https://github.com/celery/kombu/blob/56e88dc275831196ca396fce81275ac687d1207b/kombu/connection.py#L827-L839 My personal view on this is that it is good to introduce mechanism for controlling default_channel semantics but transport_options are not the right place. They are supposed to inject parameters directly to transport subsystem (Redis, py-amqp lib etc.). Even because it is not clear that it is just parameters for default_channel property.
  5. timeout parameter is timeout for overall retrying (e.g. you have multiple brokers in connection string and the timeout covers total time of waiting for new connection). This is different to connect_timeout parameter which is timeout for creating connection to single broker: https://github.com/celery/kombu/blob/56e88dc275831196ca396fce81275ac687d1207b/kombu/connection.py#L81 Hence, as mentioned in PR I prefer to rename this parameter to something more describing - e.g. total_timeout, retry_timeout, default_channel_timeout etc.
FrankK-1234 commented 2 weeks ago

Hi @thedrow, @Nusnus, @auvipy ,

To me is seems that #1599 already sets the timeout parameter (though based on connect_retry_timeout). Merging the PR #1229 would create a double configuration or conflicting one. Perhaps this can be closed or perhaps #1599 does not use the correct naming (problem it is already released).

Also see #1458 and #1298 which seem related.

With kind regards,

Frank

sschiessl-bcp commented 1 week ago

Please have a look here https://github.com/celery/kombu/pull/1229#issuecomment-2188830306

Considering that, I do think the timeout option should be renamed at this point, and the wording as well as documentation streamlined with the other ways on how this option can be set.

What do you think?