Fixed #11331 -- Removed disconnect_all() from pylibmc backend

[lericson]

This in reference to #11331. libmemcached manages its own connections fairly well.

[MarkusH]

Patch needs a test, I suppose and it might be worth mentioning in the release notes for 1.9

[lericson]

It's hard to test failure behavior, I suppose it'd be possible but it seems like too big an undertaking for me. I'll be running with this modification in a production system before the end of the week, will let you know about any catastrophic failures. 😜 As for the release notes, do you want to write something or should I?

[timgraham]

Any update on this?

[lericson]

Well I don't have the time anymore, it's up to you guys. I for one think a test for this is nigh impossible to construct. Cheerio! 🍒

[timgraham]

Okay, we'll see if anyone comes along to finish it, thanks.

[edmorley]

Okay, we'll see if anyone comes along to finish it, thanks.

If a test is hard/not possible, could we not just merge this - and land a changelog entry along with it? :-)

[lericson]

I agree with @edmorley , seems really stupid to throw away useful code like this, but that's not my job I guess.

[carljm]

The code's not thrown away, it's sitting right here. Anyone who cares about it is free to finish the pull request by writing a test (or present actual evidence that a test is not feasible). We don't merge code in Django without tests just because the original contributor didn't feel like writing one.

[timgraham]

If a normal test isn't feasible, it might be possible to use mock to verify that disconnect_all is called for MemcachedCache but not PyLibMCCache.

[lericson]

Well, the code is tested by the usual caching tests so I don't understand what needs to be tested.

I also think you should refrain from formulations such as "We don't merge code in Django without tests just because the original contributor didn't feel like writing one", IMHO it creates a hostile environment.

[carljm]

@lericson Sorry for that formulation. ISTM the hostile environment was created by "seems really stupid to..."

[lericson]

Well, you are burying a fix I made that others would almost certainly appreciate to have as well, so yes I guess I'm feeling a little pissy about it.

The code is run by tests, not specifically testing disconnecting behavior but at least exercising the code path. You're free to reject my contributions on purely bureaucratic grounds, of course, but I think that's a slippery slope no maintainer should ever go down.

• Ludvig

On 1 sep. 2015, at 17:00, Carl Meyer notifications@github.com wrote:

@lericson Sorry for that formulation. ISTM the hostile environment was created by "seems really stupid to..."

—
Reply to this email directly or view it on GitHub.

[timgraham]

From the perspective of a maintainer, accepting contributions without tests is a slippery slope. :-)

If you have time to continue this discussion, why not spend a few minutes trying to write a test instead. This is how we keep Django high-quality and help to prevent regressions.

[lericson]

I’ve said it three and a half times now, so I won’t bore you with further discussion but let it be known without doubt that THE CODE IS EXERCISED IN TESTS.

Have a good one.

On 1 sep. 2015, at 18:16, Tim Graham notifications@github.com wrote:

From the perspective of a maintainer, accepting contributions without tests is a slippery slope. :-)

If you have time to continue this discussion, why not spend a few minutes trying to write a test instead. This is how we keep Django high-quality and help to prevent regressions.

—
Reply to this email directly or view it on GitHub.

[alevy]

@lericson I'm happy to take over this pull request. We've been promoting a monkey patch to our customers that basically does this for several years and it seems like the best solution without other pretty significant change to the Django caching infrastructure.

@timgraham Do you think using mock as you suggested to verify disconnect_all isn't called would be a sufficient?

[timgraham]

Yes, something like that -- I haven't looked at the details. As long as the test fails before patch and passes afterward it should be a good start at least.

[edmorley]

I'm going to open a new PR for this, but have a query as to whether third party backends should stop using disconnect_all() after every request too - see:
https://code.djangoproject.com/ticket/11331#comment:13

@jaysonsantos / @alevy , would you happen to know the answer for jaysonsantos/python-binary-memcached?
@nichochar / @cgordon , would you know for pinterest/pymemcache?

Many thanks :-)

[jaysonsantos]

@edmorley On jaysonsantos/python-binary-memcached we don't have a connection pool, in this case if you have a broken connection it would sit there forever. We have a issue that needs to be implemented that is the consistent hashing (when I created it I was mimicking django abstraction to replicate all the keys) and with it we would have a connection pool and we would manage it and reconnect if needed.

[nichochar]

I am inheriting the project, and didn't write the original code, so I could be wrong, but I believe that pymemcache doesn't close on every connection, but whenever one of the exceptions we have created gets raised.
When that happens we destroy the connection, and a new one will get reinstated on the next method call to the pymemcache client. This happens both in the Single client and in the pool client. As described here

[cgordon]

@nichochar is correct about the behavior of pymemcache. The documentation for the close method says the same: https://github.com/pinterest/pymemcache/blob/master/pymemcache/client/base.py#L241

[edmorley]

So it sounds like it's a mixed bag - and as such the Django base memcached class should probably assume the client doesn't manage its own connections until proven otherwise. (And I'll just special-case the pylibmc backend for now).

Many thanks!