Bug 11921

I agree totally, the config file is a much more logical place to have this. It would allow command line scripts and cron jobs to also make use of caching. Not only plack.

With Koha::Cache we are able to make use of a lot of caching mechanism, not just memcached, having examples of these commented out in the koha-conf.xml would make it much easier for others to use them.

See bug 16579 for more information on these patches.

in C4/Contex.pm ->new():

251: unless ( %$self ) {
         $self = Koha::Config->read_from_file($conf_fname);
     }

$self is always an empty hash at this point, so config file would be rereaded and reparsed no matter what?

C4/Auth.pm, in get_session():

1731:    elsif ( $storage_method eq 'memcached' && Koha::Caches->get_instance->memcached_cache ) {
         $session = new CGI::Session( "driver:memcached;serializer:yaml;id:md5", $sessionID, { Memcached => C4::Context->memcached } );

looks like ->memcached method doesn't exist in C4::Context anymore

Trap for young testers

Bug 17189 updates plack.psgi .. make sure you are running that version before you test (you will probably need to manually copy it)

It takes a bit to get set up to test, the main thing is make sure you are using the latest plack.psgi.

But once it is set up it seems to work well, plack is now running using memcached using the variables from the config file.

I will sign off.

Created attachment 55310 [details] [review]
Bug 11921: Restore memcached infos to koha-conf

MEMCACHED_SERVERS and MEMCACHED_NAMESPACE should be moved back to
koha-conf instead of an ENV var to let command line scripts use
the cache mechanism.

TODO:
- Add the entries to koha-conf.xml
- Remove occurrences of MEMCACHED_SERVERS in package, apache and plack files

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 55311 [details] [review]
Bug 11921: Restore memcached infos to koha-conf - Koha::Config

Move C4::Context->read_config_file to Koha::Config
That permits to reuse it from Koha::Cache without needing C4::Context
(and introduce a circular deps).

TODO: Add decent POD to Koha::Config
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 55312 [details] [review]
Bug 11921: Update remaining occurrences of ->ismemcached

TODO: Would be better to provide a better way to know if memcache is correctly set
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 55313 [details] [review]
Bug 11921: C4::Context->memcached does no longer exist

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 55314 [details] [review]
Bug 11921: Fix logic in C4::Context->new to init the config

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 55315 [details] [review]
Bug 11921: (QA followup) Remove remaining unused var

This patch removes an occurence of an unused var in Context.pm and
also prevents a warning if memcached_servers is not set:

Use of uninitialized value in split at /home/vagrant/kohaclone/Koha/Cache.pm line 91.

t also tidies small things.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Works as expected, backwards compatible (ENV overloads koha-conf.xml)

Pushed to master for 16.11, thanks Jonathan!

Created attachment 55441 [details] [review]
Bug 11921: (followup) Don't die on non-existent koha-conf.xml

This patch wraps the XMLin call on an eval block. It also adds unit
tests (mocked) for Koha::Config->read_from_file()

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

To work in (so-so) reliable way, this feature (using memcached for storing CGI sessions)

    $session = new CGI::Session( "driver:memcached;serializer:yaml;id:md5", $sessionID, { Memcached => $memcached } );

desperately needs a possibility to use it's own separately configured memcached server instance for that task. Otherwise the risk for user sessions being occasionally kicked out in semi-random fashion is pretty substantial.

(In reply to Jacek Ablewicz from comment #29)
> To work in (so-so) reliable way, this feature (using memcached for storing
> CGI sessions)
> 
>     $session = new CGI::Session( "driver:memcached;serializer:yaml;id:md5",
> $sessionID, { Memcached => $memcached } );
> 
> desperately needs a possibility to use it's own separately configured
> memcached server instance for that task. Otherwise the risk for user
> sessions being occasionally kicked out in semi-random fashion is pretty
> substantial.

Jacek, are you implying there's a bug on master now this has been pushed?

(In reply to Tomás Cohen Arazi from comment #30)
> (In reply to Jacek Ablewicz from comment #29)
> > To work in (so-so) reliable way, this feature (using memcached for storing
> > CGI sessions)
> > 
> >     $session = new CGI::Session( "driver:memcached;serializer:yaml;id:md5",
> > $sessionID, { Memcached => $memcached } );
> > 
> > desperately needs a possibility to use it's own separately configured
> > memcached server instance for that task. Otherwise the risk for user
> > sessions being occasionally kicked out in semi-random fashion is pretty
> > substantial.
> 
> Jacek, are you implying there's a bug on master now this has been pushed?

Sorry for the confusing comment - what I meant is that storing sessions in the same memcached server instance which is used as L2 caching system for everything else (& their dogs) has potential reliability problems (extra fun / unpredictability involved if the same memcached instance is used by the multiple tenants) - but it's not the fault of this patch set, more like a side note.

*** Bug 17224 has been marked as a duplicate of this bug. ***

Jonathan, don't you think pushing those patches to 16.05 would be highly
risky? I can see the issue in 16.05 concerning the way memcached server is
retrieved. With Plack enabled, memcached server is just not used whether you
don't export MEMCACHED_SERVERS in shell environment before starting Plack. But
isn't the patch for bug 17262 more secure for 16.05? because it doesn't affect
that much code in Koha inner modules?

(In reply to Frédéric Demians from comment #33)
> Jonathan, don't you think pushing those patches to 16.05 would be highly
> risky? I can see the issue in 16.05 concerning the way memcached server is
> retrieved. With Plack enabled, memcached server is just not used whether you
> don't export MEMCACHED_SERVERS in shell environment before starting Plack.
> But
> isn't the patch for bug 17262 more secure for 16.05? because it doesn't
> affect
> that much code in Koha inner modules?

Yes, I don't think these patches should be backported.

enhancement, passing for 16.05