Passing transport_options to conn_opts in _extract_failover_opts

[sschiessl-bcp]

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]

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

[sschiessl-bcp]

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

done

[matusvalo]

@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.