Bug 17833 - _initilize_memcached() warns if errors
Summary: _initilize_memcached() warns if errors
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-02 11:13 UTC by Olli-Antti Kivilahti
Modified: 2018-01-16 12:24 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17833 - Memcached silently fails to _initilize_memcached() on the second time it is invoked (2.04 KB, patch)
2017-01-02 11:28 UTC, Olli-Antti Kivilahti
Details | Splinter Review
Bug 17833 - _initilize_memcached() warns if errors (1.16 KB, patch)
2017-01-02 12:22 UTC, Olli-Antti Kivilahti
Details | Splinter Review
Bug 17833 - _initilize_memcached() warns if errors (1.21 KB, patch)
2017-10-15 10:52 UTC, Katrin Fischer
Details | Splinter Review
Bug 17833 - _initilize_memcached() warns if errors (1.31 KB, patch)
2018-01-05 07:55 UTC, Marcel de Rooy
Details | Splinter Review
Bug 17833: Make sure this warning will not be ignored (1.18 KB, patch)
2018-01-05 17:03 UTC, Jonathan Druart
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2017-01-02 11:13:37 UTC
When _initilize_memcached() is called the first time, it sets the 'ismemcached'-key and returns 1.

When _initilize_memcached() is called the second time, the 'ismemcached'-key already exists and '' is returned (this default to False in Perl).
Koha thinks that memcached is not in use and decides to use some other caching mechanism instead.

Cache::Memcached::Fast returns undef on server error:
https://metacpan.org/pod/Cache::Memcached::Fast#add

Koha MUST listen to undefined, instead of False to define whether or not memcached is properly working.


Also added simple helpful diagnostics to help identify and log when this memcached server connection problem arises.


Here is a small test run to verify this behaviour:
root@kohadev:~# echo "fresh memcached start"
fresh memcached start
root@kohadev:~# perl l.pl
127.0.0.1:4545 1.4.14 (Ubuntu)
def  : '1'
getkey: 'text'
root@kohadev:~# echo "val already cached"
val already cached
root@kohadev:~# perl l.pl
127.0.0.1:4545 1.4.14 (Ubuntu)
def  : ''
getkey: 'text'
root@kohadev:~# echo "memcached killed and not responding"
memcached killed and not responding
root@kohadev:~# perl l.pl
undef
getkey: ''
root@kohadev:~# echo "The return value is undefined"
The return value is undefined
root@kohadev:~#



Here is the code snippet used to test the Memcached-server in the previous example output:

kivilahtio@koha_ci_1 /h/k/Koha> cat t.pl
use Cache::Memcached::Fast;
use Modern::Perl;

my $memd = new Cache::Memcached::Fast({
    servers => [ '/lxcBindMount/tmp/memcached.sock' ],
    namespace => 'koha_ci_1:',
    connect_timeout => 0.2,
    io_timeout => 0.5,
    close_on_error => 1,
    max_failures => 3,
    failure_timeout => 2,
    ketama_points => 150,
    nowait => 1,
    hash_namespace => 1,
});


# Get server versions.
my $versions = $memd->server_versions;
while (my ($server, $version) = each %$versions) {
    print "$server $version\n";
}

# Store scalars.
my $rv = $memd->add('skey', 'text');
print "undef\n" if not defined $rv;
print "def  : '$rv'\n" if defined $rv;
print "getkey: '".$memd->get('skey')."'\n";
Comment 1 Olli-Antti Kivilahti 2017-01-02 11:28:12 UTC
Created attachment 58554 [details] [review]
Bug 17833 - Memcached silently fails to _initilize_memcached() on the second time it is invoked

When _initilize_memcached() is called the first time, it sets the
'ismemcached'-key and returns 1.

When _initilize_memcached() is called the second time, the 'ismemcached'-key
already exists and '' is returned (this default to False in Perl).
Koha thinks that memcached is not in use and decides to use some other caching
mechanism instead.

Cache::Memcached::Fast returns undef on server error:
https://metacpan.org/pod/Cache::Memcached::Fast#add

Koha MUST listen to undefined, instead of False to define whether or not memcached
is properly working.

Also added simple helpful diagnostics to help identify and log when this memcached
server connection problem arises.
Comment 2 Jonathan Druart 2017-01-02 12:01:44 UTC
You are mixing add and set, replace ->add with ->set in your snippet and it will work as expected.
Set will return 1 even if the key already exists.

It seems that this is invalid, waiting for Olli's confirmation.
Comment 3 Olli-Antti Kivilahti 2017-01-02 12:21:58 UTC
Thanks for digging into this Jonathan!

You are correct. The culprit was misconfigured socket permission. Same behaviour is expected on any connection issue to the memcached server.

I changed the bug title and content to only add warnings when a server/connection problem occurs.
Comment 4 Olli-Antti Kivilahti 2017-01-02 12:22:38 UTC
Created attachment 58556 [details] [review]
Bug 17833 - _initilize_memcached() warns if errors

If memcached or the connection to it is misconfigured, show simple warnings to help
identify the problem.
Comment 5 Katrin Fischer 2017-10-15 10:52:34 UTC
Created attachment 68162 [details] [review]
Bug 17833 - _initilize_memcached() warns if errors

If memcached or the connection to it is misconfigured, show simple warnings to help
identify the problem.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2017-10-15 10:52:58 UTC
Small change, didn't appear to break something in my testing.
Comment 7 Marcel de Rooy 2018-01-05 07:55:32 UTC
Created attachment 70281 [details] [review]
Bug 17833 - _initilize_memcached() warns if errors

If memcached or the connection to it is misconfigured, show simple warnings to help
identify the problem.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Jonathan Druart 2018-01-05 17:03:10 UTC
Created attachment 70307 [details] [review]
Bug 17833: Make sure this warning will not be ignored
Comment 9 Jonathan Druart 2018-01-09 20:27:47 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 10 Nick Clemens 2018-01-16 12:24:37 UTC
Enhancement, skipping for 17.11.x.
Awesome work everybody!