Bug 7533 - Add template_cache_dir to the koha-conf.xml file
Summary: Add template_cache_dir to the koha-conf.xml file
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: master
Hardware: All All
: P3 enhancement (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 7511
Blocks: 17819 17951
  Show dependency treegraph
 
Reported: 2012-02-13 10:03 UTC by Paul Poulain
Modified: 2017-06-14 22:17 UTC (History)
7 users (show)

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


Attachments
Bug 7533: Add the template_cache_dir entry to koha-conf.xml (1.86 KB, patch)
2016-12-28 13:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7533: Add a warning to the about page if template_cache_dir is not set (3.87 KB, patch)
2016-12-28 13:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7533: Add the template_cache_dir entry to koha-conf.xml (1.92 KB, patch)
2016-12-29 08:06 UTC, Magnus Enger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7533: Add a warning to the about page if template_cache_dir is not set (4.13 KB, patch)
2016-12-29 08:08 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 7533: Add the template_cache_dir entry to koha-conf.xml (1.96 KB, patch)
2017-01-17 14:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 7533: Add a warning to the about page if template_cache_dir is not set (4.17 KB, patch)
2017-01-17 14:11 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2012-02-13 10:03:32 UTC
Add an option to the command-line installer that will let the user choose to activate T::T during installation.
Comment 1 Mason James 2016-08-30 22:19:22 UTC
(In reply to Paul Poulain from comment #0)
> Add an option to the command-line installer that will let the user choose to
> activate T::T during installation.

Hi Paul, do you think it would be better to enable this feature by default?

It seems to good to just have it as an 'optional' choice :)
Comment 2 Magnus Enger 2016-12-28 13:02:08 UTC
I always enable TT caching for new instances.
Comment 3 Jonathan Druart 2016-12-28 13:53:13 UTC
Created attachment 58467 [details] [review]
Bug 7533: Add the template_cache_dir entry to koha-conf.xml

And comment it, as we don't know what are the sysop's preferences
Comment 4 Jonathan Druart 2016-12-28 13:53:17 UTC
Created attachment 58468 [details] [review]
Bug 7533: Add a warning to the about page if template_cache_dir is not set

We need to tell the administrators that it would be great for them to
set this config entry.

Test plan:
- Do not set template_cache_dir and confirm that you see the warning
- Set template_cache_dir and confirm that you do not see the warning
Comment 5 Jonathan Druart 2016-12-28 13:56:20 UTC
I don't think it's a good idea to just ask during the install process if the administrators want this option.
It's better to let them enable it if they want it and configure it following their preferences.
Comment 6 Magnus Enger 2016-12-29 08:06:57 UTC
Created attachment 58476 [details] [review]
[SIGNED-OFF] Bug 7533: Add the template_cache_dir entry to koha-conf.xml

And comment it, as we don't know what are the sysop's preferences

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 7 Magnus Enger 2016-12-29 08:08:07 UTC
Created attachment 58477 [details] [review]
[SIGNED-OFF] Bug 7533: Add a warning to the about page if template_cache_dir is not set

We need to tell the administrators that it would be great for them to
set this config entry.

Test plan:
- Do not set template_cache_dir and confirm that you see the warning
- Set template_cache_dir and confirm that you do not see the warning

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Both templates for koha-conf.xml are updated. After applying the
patach a warning was correctly displayed. After adding
template_cache_dir to koha-conf.xml and restarting memcached it
went away.
Comment 8 Marcel de Rooy 2016-12-31 09:37:53 UTC
I would prefer not to use /tmp as the template cache dir, but a subdir of /tmp in order to keep more control there.
Also I am wondering if we should give each instance a separate cache dir or share it between instances. I have a slight preference for separating them, although it does not seem strictly necessary.
Comment 9 Jonathan Druart 2016-12-31 11:16:24 UTC
(In reply to Marcel de Rooy from comment #8)
> I would prefer not to use /tmp as the template cache dir, but a subdir of
> /tmp in order to keep more control there.
> Also I am wondering if we should give each instance a separate cache dir or
> share it between instances. I have a slight preference for separating them,
> although it does not seem strictly necessary.

The line is commented by default, I think it's the duty of the sys admin to configure it correctly.
IIRC if the subdir does not exist, it will explode.
Comment 10 Nick Clemens 2017-01-17 14:11:18 UTC
Created attachment 59091 [details] [review]
Bug 7533: Add the template_cache_dir entry to koha-conf.xml

And comment it, as we don't know what are the sysop's preferences

Signed-off-by: Magnus Enger <magnus@libriotech.no>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens 2017-01-17 14:11:29 UTC
Created attachment 59092 [details] [review]
Bug 7533: Add a warning to the about page if template_cache_dir is not set

We need to tell the administrators that it would be great for them to
set this config entry.

Test plan:
- Do not set template_cache_dir and confirm that you see the warning
- Set template_cache_dir and confirm that you do not see the warning

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Both templates for koha-conf.xml are updated. After applying the
patach a warning was correctly displayed. After adding
template_cache_dir to koha-conf.xml and restarting memcached it
went away.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Tomás Cohen Arazi 2017-01-18 12:47:49 UTC
I think we should fall back to /tmp, and take care of creating a per-instance directory in /var/somewhere for each instance.

I think the patchset is incomplete!
Comment 13 Marcel de Rooy 2017-01-18 12:48:33 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> I think we should fall back to /tmp, and take care of creating a
> per-instance directory in /var/somewhere for each instance.
> 
> I think the patchset is incomplete!

I agree, but did not want to block this.
Comment 14 Kyle M Hall 2017-01-20 14:13:32 UTC
Pushed to master for 17.05, thanks Jonathan! Would you be able to add a followup bug to store the cache in /var/lib/koha/$site/template_cache or something like that for packages?
Comment 15 Katrin Fischer 2017-01-22 20:29:46 UTC
This won't get ported back to 16.11.x as it is an enhancement.
Comment 16 Jonathan Druart 2017-01-23 07:58:42 UTC
(In reply to Kyle M Hall from comment #14)
> Pushed to master for 17.05, thanks Jonathan! Would you be able to add a
> followup bug to store the cache in /var/lib/koha/$site/template_cache or
> something like that for packages?

Tomas opened bug 17951.
Comment 17 Jonathan Druart 2017-01-23 08:00:16 UTC
(In reply to Katrin Fischer from comment #15)
> This won't get ported back to 16.11.x as it is an enhancement.

I think this one should be backported to the stable branches, it will not hurt and could highlight performance boost.
Comment 18 Paul Poulain 2017-01-23 08:30:43 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Katrin Fischer from comment #15)
> > This won't get ported back to 16.11.x as it is an enhancement.
> 
> I think this one should be backported to the stable branches, it will not
> hurt and could highlight performance boost.

I agree with Jonathan
Comment 19 Katrin Fischer 2017-01-23 10:18:00 UTC
After looking at the patches they seem safe, so I will probably push in the next few days.
Comment 20 Katrin Fischer 2017-01-23 10:19:16 UTC
Wow, it's hard to get from RESOLVED FIXED back to "Pushed to master"!
Comment 21 Katrin Fischer 2017-02-07 07:05:37 UTC
I've decided to make an exception here and backport this enh as it might be helpful for stable releases and it's quite safe not to break something.

These patches have been pushed to 16.11.x and will be in 16.11.04.
Comment 22 Mason James 2017-02-22 23:06:46 UTC
Pushed to 16.05.x, for 16.05.10 release