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
Allow to pass pymemcache socket keepalive and allow to instantiate a RetryingClient #205
Conversation
|
These changes implement two new features:
I made two separated commits to allow isolated reviews. |
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 8a7ea5d of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change 8a7ea5d: https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 |
dogpile/cache/backends/memcached.py
Outdated
| ) | ||
| if self.retry_client: |
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.
do we want to raise an argument error if they passed any of the "Retry" options without passing "retry_client" ?
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.
Yes from a UX point of view that's make sense.
dogpile/cache/backends/memcached.py
Outdated
| @@ -509,15 +555,32 @@ def __init__(self, arguments): | |||
| self.serde = arguments.get("serde", pymemcache.serde.pickle_serde) | |||
| self.default_noreply = arguments.get("default_noreply", False) | |||
| self.tls_context = arguments.get("tls_context", None) | |||
| self.socket_keepalive = arguments.get("socket_keepalive", None) | |||
| self.retry_client = arguments.get("retry_client", False) | |||
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.
how about "enable_retry_client" ?
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.
Sold
|
I was thinking that your gerrit will saw my changes as separated commits (keepalive and retries), unfortunately they have been merged into a single one commit on the gerrit side, so, I squashed them here too. |
you dont need to squash them here, the transfer to gerrit does the squash regardless. either way. the two changes are not that big so it's fine w/ squash. |
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 637c562 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 637c562 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 |
Ok thanks for the details. |
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision ca14bf7 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset ca14bf7 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 |
|
is this dependent on an unreleased version of pymemcache? |
My bad. It should be |
|
I'll be on PTO and mostly off-line during the next two weeks so I'll take long time replying to you. |
enjoy your PTO, no rush on this end |
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 914f215 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 914f215 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 |
|
Since this is passing in the kwargs directly to the pymemcache constructors, there should probably be a docs note about the minimum compatible pymemcache version as this could (probably?) cause exceptions on incompatible versions. |
Done. However the TLS support is in the same case and it require at least pymemcache 3.1.0. Do we want to add notes about the TLS feature and minimal pymemcache compatible version too? |
Though that could be done with a follow up patch. |
|
I think that would make sense. I'm just stepping through possible deployment/upgrade scenarios for existing applications, and I see this raising an exception without much helpful information. I don't think it needs to be accurate, but just having something like "the minimum supported/tested pymemcache version is 3.5" in the README or docs would be enough of a hint. |
|
Failed to create a gerrit review, git squash against branch 'master' failed |
OK, this is sqla-tester setting up my work on behalf of 4383 to try to get revision 03aee4b of this pull request into gerrit so we can run tests and reviews and stuff
|
Failed to create a gerrit review, git squash against branch 'master' failed |
OK, this is sqla-tester setting up my work on behalf of 4383 to try to get revision 9ad5a92 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 9ad5a92 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 |
|
Changes rebased over the HEAD of master |
Socket keepalive capabilities have been introduced [1][2][3] with pymemcache 3.5.0 [4][5]. These changes allow to pass keepalive configuration to the pymemcache client. Retry mechanisms have been implemented [6][7] with pymemcache 3.5.0 [5][8]. These changes allow to instantiate a ``RetryingClient`` by using dogpile.cache. [1] pinterest/pymemcache@b289c87 [2] pinterest/pymemcache@c782de1 [3] pinterest/pymemcache@4d46f5a [4] pinterest/pymemcache@07b5ecc [5] https://pypi.org/project/pymemcache/3.5.0/ [6] pinterest/pymemcache@75fe5c8 [7] https://pymemcache.readthedocs.io/en/latest/getting_started.html#using-the-built-in-retrying-mechanism [8] pinterest/pymemcache@07b5ecc Change-Id: Ia2e76475943bc8ec86b5974217d1c92110be5b43
types-redis and types-decorator seems missing requirements, these changes add them to the mypy tox environment. Change-Id: Iee34ed090895728ae4e46c45eb4ef56ba6a43a4b
|
The latest patch set fix a typo in the commit message |
OK, this is sqla-tester setting up my work on behalf of 4383 to try to get revision f898fe4 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset f898fe4 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 |
mike bayer (zzzeek) wrote:
better, can you go through the other change requests i made on patch set 6, changelog file, imports, versionadded directives, thanks!
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965
| @@ -17,6 +18,8 @@ | |||
| from ..api import NO_VALUE | |||
| from ... import util | |||
|
|
|||
| log = logging.getLogger(__name__) | |||
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.
mike bayer (zzzeek) wrote:
this should be removed
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965
| @@ -488,6 +533,22 @@ class PyMemcacheBackend(GenericMemcachedBackend): | |||
| :param serde: optional "serde". Defaults to | |||
| ``pymemcache.serde.pickle_serde`` | |||
| :param default_noreply: Defaults to False | |||
| :param socket_keepalive: optional socket keepalive, will be used for | |||
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.
mike bayer (zzzeek) wrote:
these params all need "..versionadded:: 1.1.5"
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965
|
mike bayer (zzzeek) wrote: also Hervé you have access to gerrit directly here, so it might be easier if you just work with this gerrit rather than with the PR, that way we can both collaborate on pushes. View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 |
|
Hervé Beraud (4383) wrote: PS16 LGTM. View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/2965 has been merged. Congratulations! :) |
|
Thanks. @zzzeek Can I request a new release to allow us to use these changes in openstack oslo. Thanks by advance. |
* Update oslo.cache from branch 'master'
to f4fa6aa6fa2aca23a8f4f9e63c5a57dbcd2d1166
- Add socket keepalive options to oslo.cache
This patch specifies a set of options required to setup the
socket keepalive of the dogpile.cache's pymemcache
backend [1][2]. This setup from those options can later on
be passed to this backend.
This patch also sets up the socket keepalive object based on
the configuration options passed via oslo.config and adds it
as an argument to be passed to the selected oslo.cache backend.
Dogpile.cache will be used as an interface between oslo.cache and
pymemcache [3].
[1] sqlalchemy/dogpile.cache#205
[2]
pinterest/pymemcache@b289c87
[3]
https://dogpilecache.sqlalchemy.org/en/latest/api.html?highlight=keepalive#dogpile.cache.backends.memcached.PyMemcacheBackend.params.socket_keepalive
Change-Id: I501100e1a48cdd4e094c08046e2150405dcf371e
This patch specifies a set of options required to setup the socket keepalive of the dogpile.cache's pymemcache backend [1][2]. This setup from those options can later on be passed to this backend. This patch also sets up the socket keepalive object based on the configuration options passed via oslo.config and adds it as an argument to be passed to the selected oslo.cache backend. Dogpile.cache will be used as an interface between oslo.cache and pymemcache [3]. [1] sqlalchemy/dogpile.cache#205 [2] pinterest/pymemcache@b289c87 [3] https://dogpilecache.sqlalchemy.org/en/latest/api.html?highlight=keepalive#dogpile.cache.backends.memcached.PyMemcacheBackend.params.socket_keepalive Change-Id: I501100e1a48cdd4e094c08046e2150405dcf371e
This patch specifies a set of options required to setup the socket keepalive of the dogpile.cache's pymemcache backend [1][2]. This setup from those options can later on be passed to this backend. This patch also sets up the socket keepalive object based on the configuration options passed via oslo.config and adds it as an argument to be passed to the selected oslo.cache backend. Dogpile.cache will be used as an interface between oslo.cache and pymemcache [3]. This patch is needed to fix a TLS issue on stable branches introduced by pymemcache (since train), where if a cluster node disappear the client will fail without retrying to reconnect or to switch to an other node of the cluster. [1] sqlalchemy/dogpile.cache#205 [2] pinterest/pymemcache@b289c87 [3] https://dogpilecache.sqlalchemy.org/en/latest/api.html?highlight=keepalive#dogpile.cache.backends.memcached.PyMemcacheBackend.params.socket_keepalive Partial-Bug: #1959562 Change-Id: I501100e1a48cdd4e094c08046e2150405dcf371e (cherry picked from commit f4fa6aa)
This patch specifies a set of options required to setup the socket keepalive of the dogpile.cache's pymemcache backend [1][2]. This setup from those options can later on be passed to this backend. This patch also sets up the socket keepalive object based on the configuration options passed via oslo.config and adds it as an argument to be passed to the selected oslo.cache backend. Dogpile.cache will be used as an interface between oslo.cache and pymemcache [3]. This patch is needed to fix a TLS issue on stable branches introduced by pymemcache (since train), where if a cluster node disappear the client will fail without retrying to reconnect or to switch to an other node of the cluster. [1] sqlalchemy/dogpile.cache#205 [2] pinterest/pymemcache@b289c87 [3] https://dogpilecache.sqlalchemy.org/en/latest/api.html?highlight=keepalive#dogpile.cache.backends.memcached.PyMemcacheBackend.params.socket_keepalive Partial-Bug: #1959562 Change-Id: I501100e1a48cdd4e094c08046e2150405dcf371e (cherry picked from commit f4fa6aa) (cherry picked from commit 2ad2d52)
This patch specifies a set of options required to setup the
socket keepalive of the dogpile.cache's pymemcache
backend [1][2]. This setup from those options can later on
be passed to this backend.
This patch also sets up the socket keepalive object based on
the configuration options passed via oslo.config and adds it
as an argument to be passed to the selected oslo.cache backend.
Dogpile.cache will be used as an interface between oslo.cache and
pymemcache [3].
This patch is needed to fix a TLS issue on stable branches introduced by
pymemcache (since train), where if a cluster node disappear the client
will fail without retrying to reconnect or to switch to an other node of
the cluster.
Conflicts:
- requirements.txt
- doc/requirements.txt
- oslo_cache/tests/unit/test_cache_basics.py
NOTE(hberaud): Conflicts are related to requirements that have been
updated in the youngest branches, however, I removed these reqs updated
to follow the same logic that have been applied by Moisès in his patches
related to TLS, namely, appending the dogpile.cache.pymemcache backend
only if the version of dogpile.cache is higher to a specific version [4].
TLS changes are at the origin of the bug fixed here.
[1] sqlalchemy/dogpile.cache#205
[2]
pinterest/pymemcache@b289c87
[3]
https://dogpilecache.sqlalchemy.org/en/latest/api.html?highlight=keepalive#dogpile.cache.backends.memcached.PyMemcacheBackend.params.socket_keepalive
[4] https://opendev.org/openstack/oslo.cache/commit/3e3037886e8021911e10f492c0502951bff5104e
Partial-Bug: #1959562
Change-Id: I501100e1a48cdd4e094c08046e2150405dcf371e
(cherry picked from commit f4fa6aa)
(cherry picked from commit 2ad2d52)
(cherry picked from commit 7b00cb3)
This patch specifies a set of options required to setup the
socket keepalive of the dogpile.cache's pymemcache
backend [1][2]. This setup from those options can later on
be passed to this backend.
This patch also sets up the socket keepalive object based on
the configuration options passed via oslo.config and adds it
as an argument to be passed to the selected oslo.cache backend.
Dogpile.cache will be used as an interface between oslo.cache and
pymemcache [3].
This patch is needed to fix a TLS issue on stable branches introduced by
pymemcache (since train), where if a cluster node disappear the client
will fail without retrying to reconnect or to switch to an other node of
the cluster.
Conflicts:
- requirements.txt
- doc/requirements.txt
- oslo_cache/tests/unit/test_cache_basics.py
NOTE(hberaud): Conflicts are related to requirements that have been
updated in the youngest branches, however, I removed these reqs updated
to follow the same logic that have been applied by Moisès in his patches
related to TLS, namely, appending the dogpile.cache.pymemcache backend
only if the version of dogpile.cache is higher to a specific version [4].
TLS changes are at the origin of the bug fixed here.
[1] sqlalchemy/dogpile.cache#205
[2]
pinterest/pymemcache@b289c87
[3]
https://dogpilecache.sqlalchemy.org/en/latest/api.html?highlight=keepalive#dogpile.cache.backends.memcached.PyMemcacheBackend.params.socket_keepalive
[4] https://opendev.org/openstack/oslo.cache/commit/3e3037886e8021911e10f492c0502951bff5104e
This patch also squash parts of the fix submitted with:
https://review.opendev.org/c/openstack/oslo.cache/+/834267
Partial-Bug: #1959562
Change-Id: I501100e1a48cdd4e094c08046e2150405dcf371e
(cherry picked from commit f4fa6aa)
(cherry picked from commit 2ad2d52)
(cherry picked from commit 7b00cb3)
(cherry picked from commit f34ed73)
This patch specifies a set of options required to setup the
socket keepalive of the dogpile.cache's pymemcache
backend [1][2]. This setup from those options can later on
be passed to this backend.
This patch also sets up the socket keepalive object based on
the configuration options passed via oslo.config and adds it
as an argument to be passed to the selected oslo.cache backend.
Dogpile.cache will be used as an interface between oslo.cache and
pymemcache [3].
This patch is needed to fix a TLS issue on stable branches introduced by
pymemcache (since train), where if a cluster node disappear the client
will fail without retrying to reconnect or to switch to an other node of
the cluster.
Conflicts:
- requirements.txt
- doc/requirements.txt
- oslo_cache/tests/unit/test_cache_basics.py
NOTE(hberaud): Conflicts are related to requirements that have been
updated in the youngest branches, however, I removed these reqs updated
to follow the same logic that have been applied by Moisès in his patches
related to TLS, namely, appending the dogpile.cache.pymemcache backend
only if the version of dogpile.cache is higher to a specific version [4].
TLS changes are at the origin of the bug fixed here.
[1] sqlalchemy/dogpile.cache#205
[2]
pinterest/pymemcache@b289c87
[3]
https://dogpilecache.sqlalchemy.org/en/latest/api.html?highlight=keepalive#dogpile.cache.backends.memcached.PyMemcacheBackend.params.socket_keepalive
[4] https://opendev.org/openstack/oslo.cache/commit/3e3037886e8021911e10f492c0502951bff5104e
This patch also squash parts of the fix submitted with:
https://review.opendev.org/c/openstack/oslo.cache/+/834267
Conflits:
CONFLICT (content): Merge conflict in oslo_cache/tests/test_cache.py
Partial-Bug: #1959562
Change-Id: I501100e1a48cdd4e094c08046e2150405dcf371e
(cherry picked from commit f4fa6aa)
(cherry picked from commit 2ad2d52)
(cherry picked from commit 7b00cb3)
(cherry picked from commit f34ed73)
(cherry picked from commit 6194133)
This is the combinaison of two commits to implement and reflect new pymemcache features on the dogpile.cache side.
The first one is, the socket keepalive. These capabilities have been introduced [1][2][3] with pymemcache 3.5.0 [4][5]. These changes allow to pass keepalive configuration to the pymemcache client.
The second one is the retry mechanisms. Retry mechanisms have been implemented [6][7] with pymemcache 3.5.0 [8]. These changes allow to instantiate a
RetryingClientby using dogpile.cache.[1] pinterest/pymemcache@b289c87
[2] pinterest/pymemcache@c782de1
[3] pinterest/pymemcache@4d46f5a
[4] pinterest/pymemcache@07b5ecc
[5] https://pypi.org/project/pymemcache/3.5.0/
[6] pinterest/pymemcache@75fe5c8
[7] https://pymemcache.readthedocs.io/en/latest/getting_started.html#using-the-built-in-retrying-mechanism
[8] pinterest/pymemcache@07b5ecc
Change-Id: Ia2e76475943bc8ec86b5974217d1c92110be5b43