-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Pass socket_keepalive_options to redis client for result_backend #8297
base: main
Are you sure you want to change the base?
Pass socket_keepalive_options to redis client for result_backend #8297
Conversation
7b6df98
to
1253610
Compare
what this change will let to achieve? |
f6f9940
to
8d0b6f7
Compare
You will have possibility to manipulate sending tcp keepalive packet to redis from celery client. |
2103b0d
to
667278c
Compare
d4c063f
to
b743579
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not fully sure why we need this
876d212
to
784981a
Compare
117783b
to
063bd57
Compare
…support setup 0 max_connection in it as well
08ae1d6
to
9b540d3
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8297 +/- ##
==========================================
+ Coverage 81.25% 81.26% +0.01%
==========================================
Files 149 149
Lines 18553 18560 +7
Branches 3166 3169 +3
==========================================
+ Hits 15075 15083 +8
+ Misses 3191 3190 -1
Partials 287 287
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6a799e4
to
5f9c1d5
Compare
752ab5f
to
ce4580d
Compare
@@ -453,7 +458,7 @@ def apply_chord(self, header_result_args, body, **kwargs): | |||
def _chord_zset(self): | |||
return self._transport_options.get('result_chord_ordered', True) | |||
|
|||
@cached_property | |||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure about this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I have this cached_property 4 test are doing to failure:
======================================================================= short test summary info ========================================================================
FAILED t/unit/backends/test_redis.py::test_RedisBackend_chords_simple::test_on_chord_part_return__unordered - AssertionError: assert 0
FAILED t/unit/backends/test_redis.py::test_RedisBackend_chords_simple::test_on_chord_part_return_no_expiry__unordered - AssertionError: assert 0
FAILED t/unit/backends/test_redis.py::test_RedisBackend_chords_simple::test_on_chord_part_return__ChordError__unordered - TypeError: '_ContextMock' object is not subscriptable
FAILED t/unit/backends/test_redis.py::test_RedisBackend_chords_simple::test_on_chord_part_return__other_error__unordered - TypeError: '_ContextMock' object is not subscriptable
=============================================================== 4 failed, 82 passed in 63.34s (0:01:03) ================================================================
Its looks like _transport_options is setup once and every test use it (doesn't update their custom setup).
Also I don't see reason that should be cached if:
- It is in settings object so all worker have their own copy.
- Get something form dict don't cost much for computation.
Maybe something I have missed or I haven't know and any advice how to fix will be welcome and I change if back :)
@pawl since you use redis, can you please try / review this PR too? |
@auvipy I was unable to start using redis as a broker in production due to this issue: #7276 (reply in thread) |
Which version you are using? |
@auvipy here are the versions I'm using:
I wasn't seeing any fixes related to celery workers using Redis as a broker getting stuck in the 2 more recent releases. I'm thinking the issues could be related to the I need to find some time to make progress on celery/kombu#1734 However, Kombu's redis transport code is very difficult to navigate without types and debug logging. I'm thinking it may be easiest to add types before proceeding. At the moment, I'm not sure I'd recommend using Redis as a celery broker. |
This change doesn't fix reset connection but possibility to configure keep_alive connection to results pub/sub redis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rebase please to test with python 3.12 in CI?
self._ConnectionPool = connection_pool | ||
|
||
socket_timeout = _get('redis_socket_timeout') | ||
socket_connect_timeout = _get('redis_socket_connect_timeout') | ||
retry_on_timeout = _get('redis_retry_on_timeout') | ||
socket_keepalive = _get('redis_socket_keepalive') | ||
socket_keepalive_options = self._transport_options.get('socket_keepalive_options', {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we document this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have this description for those parameters and the rest parameters are in code they aren't described either.
So I haven't described it as well. So the question is: should I be highlighted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least New in version 5.4. annotation?
58c6da7
to
1e2cf86
Compare
statefilename
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file should be removed from the PR git commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Thanks for the clue!
================================== FAILURES =================================== self = <t.unit.worker.test_worker.test_WorkController object at 0x0000019DC8CCD280>
t\unit\worker\test_worker.py:1027: t\unit\worker\test_worker.py:717: in create_worker file = 'statefilename', flag = 'c', mode = 438
C:\hostedtoolcache\windows\Python\3.12.0\x64\Lib\dbm_init_.py:91: error |
f8c356b
to
e6764e4
Compare
8be4339
to
dc71d74
Compare
@auvipy can you rerun this one integration test. It looks like is non-deterministic. |
I rerun several time and it is passing this time! |
self._ConnectionPool = connection_pool | ||
|
||
socket_timeout = _get('redis_socket_timeout') | ||
socket_connect_timeout = _get('redis_socket_connect_timeout') | ||
retry_on_timeout = _get('redis_retry_on_timeout') | ||
socket_keepalive = _get('redis_socket_keepalive') | ||
socket_keepalive_options = self._transport_options.get('socket_keepalive_options', {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least New in version 5.4. annotation?
@awmackowiak Hey there - is it possible to finalize this PR? Thank you 🙏 |
Note: Before submitting this pull request, please review our contributing
guidelines.
Description
In result_backend_transport_options documentation celery have description:
so after this change we will have possibility to pass socket_keepalive_options defined in result_backend_transport_options to result backend redis client. Also set up max_connection to 0 with is responsible to close conenction to redis after sending task to it.
This options are supported in redis-py client.