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

add config to make memcached binary protocol optional #54

Merged
merged 1 commit into from Nov 6, 2019

Conversation

@romainmenke
Copy link
Contributor

romainmenke commented Sep 25, 2019

w3-total-cache fails to connect to memcached servers with binary protocol turned on.

The cause of this issue is the \Memcached::OPT_BINARY_PROTOCOL constant which is defined if there is support for this feature on the server running the Wordpress instance. It is however possible that the actual memcached server instance, or a proxy in between does not support it.

This change adds a config option to disable the binary protocol for memcached.

I'm not a 100% sure I updated all the relevant parts to support this new config. The core functionality is tested and works for us. This setting appears in the Admin interface and is editable there. Feedback is welcome!

@maxicus

This comment has been minimized.

Copy link
Contributor

maxicus commented Sep 25, 2019

Thanks. We wanted to avoid one more config option (too much already) but seems there are still a lot legacy systems...

How about moving OPT_TCP_NODELAY there too? In that case disabling binary protocol would switch all those new features.
Does it work well for you without OPT_TCP_NODELAY?

@romainmenke

This comment has been minimized.

Copy link
Contributor Author

romainmenke commented Sep 25, 2019

As far as we can tell it works just fine with or without OPT_TCP_NODELAY (we did not run our benchmarks). It only breaks with OPT_BINARY_PROTOCOL on.

Maybe keep these features separate for now? We understand wanting to keep the config options to a minimum but throwing in OPT_TCP_NODELAY behind a config options binary_protocol seems confusing and counter intuitive to us.

@romainmenke

This comment has been minimized.

Copy link
Contributor Author

romainmenke commented Oct 2, 2019

@maxicus What is your preferred fix for this issue? I don't mind putting more work into this PR to make sure you are happy with it.

@romainmenke

This comment has been minimized.

Copy link
Contributor Author

romainmenke commented Oct 16, 2019

@maxicus ping

@maxicus

This comment has been minimized.

Copy link
Contributor

maxicus commented Oct 16, 2019

@romainmenke Thank you for your help.
We just giving a chance for other people to report something related to this area.
The primary question if anyone has problems with NO_DELAY mode.

So far your fix is fine, and we are going to merge it to master shortly. In any case before the next release.

@maxicus maxicus merged commit b2c09a8 into W3EDGE:master Nov 6, 2019
@romainmenke romainmenke deleted the romainmenke:make-memcached-binary-protocol-optional branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.