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.

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

Move dead_retry and socket_timeout into the MemcachedBackend class #228

Closed
wants to merge 1 commit into from

Conversation

4383
Copy link
Collaborator

@4383 4383 commented Jul 7, 2022

In my previous patch [1] I proposed to add the dead_retry and socket_timeoutparams to the MemcacheArgs. I was wrong. My goal was to pass these parameters to the client during its initialization to set the memcached client dead_retry and socket_timeout arguments.

By using the MemcacheArgs they are passed to the method calls which is not what it was requested in the feature request [2]. I misunderstood the goal of this class (MemcacheArgs).

My previous patch led to issues [3][4] that I'm able to reproduce locally by using oslo.cache's functional test. These changes fix these issues.

[1] 1de93aa
[2] #223
[3] https://bugzilla.redhat.com/show_bug.cgi?id=2103117
[4] https://review.opendev.org/c/openstack/requirements/+/848827

@4383 4383 requested a review from sqla-tester July 7, 2022 14:36
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of 4383 to try to get revision b4dc017 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change b4dc017: https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of 4383 to try to get revision 07f91ce of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 07f91ce added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

hi -

this is a little bit of a debacle. Can you point me to the documentaiton for these parameters so that I can review what they do, who accepts them and where? thanks. also we can't just yank the params away unless it is 1000% clear they cannot be used in the way that we implemented them

:param dead_retry: Number of seconds memcached server is considered dead
before it is tried again..

.. versionadded:: 1.1.7
Copy link
Member

Choose a reason for hiding this comment

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

OK this can't be removed unless it is absolutely clear that this argument can never work on any backend, ever. this has been released

before it is tried again. Will be passed to ``memcache.Client``
as the ``dead_retry`` parameter.

.. versionadded:: 1.1.8
Copy link
Member

Choose a reason for hiding this comment

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

.. verisionchanged:: 1.1.8  Moved the ``dead_retry`` argument which was erroneously added to the "set_parameters", where it has no effect, to be part of the Memcached connection arguments

@sqla-tester
Copy link
Collaborator

Hervé Beraud (4383) wrote:

Patch Set 2:

(2 comments)

zzzeek@github wrote:

hi -

this is a little bit of a debacle. Can you point me to the documentaiton for these parameters so that I can review what they do, who accepts them and where? thanks. also we can't just yank the params away unless it is 1000% clear they cannot be used in the way that we implemented them

Hey,

These parameters are consumed by the python-memcached Client's init class:

By passing them through the MemcacheArgs they are passed to all the method calls like min_compress_len and memcached_expire_time (time) but those there are present in all the method signatures (e.g https://github.com/linsomniac/python-memcached/blob/master/memcache.py#L695) so this is not issue for these. That's not the case of dead_retry and socket_timeout, those are only configurable through the constructor. dead_retry and socket_timeout are not present in method signatures other than within the client's constructor.

We just want to set dead_retry and socket_timeout when we init our client.

The current behavior introduced by my previous changes will lead oslo.cache to use dead_retry and socket_timeout as kwargs passed to the calls of client's methods (e.g set_multi, https://github.com/linsomniac/python-memcached/blob/master/memcache.py#L833)(https://opendev.org/openstack/oslo.cache/src/branch/master/oslo_cache/backends/memcache_pool.py#L32) which is not what we want.

Let me know if you need more details.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

i need to know that the feature we implemeneted in 1.7 is 100% not-usable, that is, if one uses these parameters as implemented in 1.7, then the program will crash. that way we can safely remove them as you are doing here.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

@sqla-tester
Copy link
Collaborator

Hervé Beraud (4383) wrote:

All the elements that I found for now seems to indicate that 1.7 is not usable.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of 4383 to try to get revision e8431ba of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset e8431ba added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

@4383 4383 requested a review from sqla-tester July 7, 2022 18:04
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of 4383 to try to get revision e8431ba of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset e8431ba added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

this need a changelog file in docs/build/unreleased the same way we did for #223.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

Moves the ``dead_retry`` argument and the ``socket_timeout`` argument which
were erroneously added to the "set_parameters", where they have no effect, to
be part of the Memcached connection arguments.

Indeed, In my previous patch [1] I proposed to add the ``dead_retry`` and
``socket_timeout`` params to the ``MemcacheArgs`` class. I was wrong. My goal
was to pass these parameters to the client during its initialization to set
the memcached client dead_retry and socket_timeout arguments [2].

By using the MemcacheArgs they are passed to the method calls which is not what
it was requested in the feature request [3]. I misunderstood the goal of
this class (MemcacheArgs).

The ``MemcacheArgs`` class is only inherited by the ``MemcachedBackend``
class and the ``PylibmcBackend`` class. Both libraries doesn't support
``dead_retry`` and ``socket_timeout`` in their methods related to the
memcache API commands (set, get, set_multi, etc), so for this reason I
think we can move those parameters safely.

My previous patch led to issues [4][5] that I'm able to reproduce locally by
using oslo.cache's functional test. These changes fix these issues.

[1] sqlalchemy@1de93aa
[2] https://github.com/linsomniac/python-memcached/blob/7942465eba2009927e5d14b4b6dbd48b75780d80/memcache.py#L165
[3] sqlalchemy#223
[4] https://bugzilla.redhat.com/show_bug.cgi?id=2103117
[5] https://review.opendev.org/c/openstack/requirements/+/848827
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of 4383 to try to get revision dcef04b of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset dcef04b added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

i know you are in a hurry, is this set for me to finalize and release ?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

@sqla-tester
Copy link
Collaborator

Hervé Beraud (4383) wrote:

Yes, sure. Thanks Mike.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978 has been merged. Congratulations! :)

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

OK we release again..... are you SURE?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

sqlalchemy-bot pushed a commit that referenced this pull request Jul 8, 2022
Change-Id: Ic9de2099134e8bd9c20a802ff604599ef511cfd9
@sqla-tester
Copy link
Collaborator

Hervé Beraud (4383) wrote:

Sorry for my late reply I was AFK.
Thanks for 1.1.8 I proposed a patch to move openstack directly to this version and ignore 1.1.7 https://review.opendev.org/c/openstack/requirements/+/849138

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/3978

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants