Support from Acquia helps fund testing for Drupal Acquia logo

Comments

peximo created an issue. See original summary.

peximo’s picture

peximo’s picture

Title: Add Memcached ascii authentication support. » Add Memcached ascii authentication support
peximo’s picture

Added a comment to the README.

Jeremy’s picture

This is great! Please find an updated patch after more testing.

- renamed all related functions and variables to include ascii_auth (to differentiate from sasl auth)
- added additional documentation
- perform additional tests on admin Status Report

Jeremy’s picture

FileSize
18.98 KB
Jeremy’s picture

FileSize
19.01 KB

Uploading a new version of the patch, as the previous wasn't properly checking the memcached version.

Jeremy’s picture

I ran some load tests on this patch. Enabling ASCII protocol authentication on the server and in the module results in ~1.25% performance overhead. This seems primarily due to an ~11.25% increase in memcache GET requests. Very acceptable overhead.

Fabianx’s picture

  1. +++ b/README.txt
    @@ -686,6 +690,31 @@ memcache_sasl_username and memcache_sasl_password in settings.php. For example:
    +ASCII protocol authentication requires Memcached version 1.5.15 or greater
    

    I suggest to add that at least 1.6.4 is recommended to avoid that one bug.

  2. +++ b/dmemcache.inc
    @@ -1265,3 +1322,221 @@ function _dmemcache_write_debug($action, $bin, $key, $rc) {
    +  // Login to the server.
    +  $rc = $mc->setByKey($full_key, DRUPAL_MEMCACHE_ASCII_AUTH_KEY, $memcache_ascii_auth);
    +  if (!$rc) {
    

    You are missing an essential interdiff - I will post it again.

  3. +++ b/dmemcache.inc
    @@ -1265,3 +1322,221 @@ function _dmemcache_write_debug($action, $bin, $key, $rc) {
    +  // Set the lifetime key.
    +  $mc->setByKey($full_key, DRUPAL_MEMCACHE_ASCII_AUTH_LIFETIME_KEY, TRUE);
    

    I still don't get why setByKey returns success when it fails to set the key. [or maybe it can fail to set the key even though the connection is authorized, because any set on an unauthorized connection should give back error 9 CLIENT ERROR].

  4. +++ b/memcache.install
    @@ -113,6 +114,40 @@ function memcache_requirements($phase) {
    +        // Confirm ASCII authentication works on all memcached servers.
    +        $memcache_bins = variable_get('memcache_bins', array('cache' => 'default'));
    +        foreach ($memcache_bins as $bin => $_) {
    +          if ($mc = dmemcache_object($bin)) {
    +            $ascii_auth =  _dmemcache_reset_ascii_auth($mc, TRUE);
    +            if ($ascii_auth !== TRUE) {
    

    IIRC - you also wanted to check if ascii auth is even enabled.

    The easiest is to create a new Memcached() instance for each connection in the getServerList() and see that a simple set on a dummy key fails.

Fabianx’s picture

Jeremy’s picture

I suggest to add that at least 1.6.4 is recommended to avoid that one bug.

I've updated the README, and I also added a Warning on the Status Page if <1.6.4 is used.

You are missing an essential interdiff - I will post it again.

Merged in.

I still don't get why setByKey returns success when it fails to set the key. [or maybe it can fail to set the key even though the connection is authorized, because any set on an unauthorized connection should give back error 9 CLIENT ERROR].

It likely is specific to have 1 failed among 2+ servers in a given bin?

I did not address 4, but additional checks are certainly welcome as silent errors are very difficult to debug.

Jeremy’s picture

Note, this patch was load tested with Goose using this example:
https://github.com/tag1consulting/goose/blob/master/examples/drupal_load...

The raw command used was:
cargo run --release --example drupal_loadtest -- --host http://local.dev/ -c10 -t15m -v --only-summary

And the collected results:
https://docs.google.com/spreadsheets/d/1aRns-fiXepYhz-lA2y2aJNgFuWjy67OS...

Jeremy’s picture

Issue summary: View changes
FileSize
22.44 KB

  • Jeremy committed a7a183d on 7.x-1.x authored by Fabianx
    Issue #3150893 by Jeremy, peximo, Fabianx: Add Memcached ascii...
Jeremy’s picture

Version: 7.x-1.x-dev » 8.x-2.x-dev
Status: Active » Patch (to be ported)

This has been committed. Marking as to-be-ported to the 8.x/9.x version.