Moodle
  1. Moodle
  2. MDL-39448

MUC: Memcache should use configurable key names

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.3
    • Fix Version/s: 2.4.4
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide

      1. In admin settings, go to Plugins/Caching/Configuration.
      2. Under 'memcache', choose 'Add instance'.

      • There should be a 'Key prefix' field with a help button, limited to 5 characters. The default value for this field is 'mdl_'.

      3. Type an arbitrary name for the store, type a working memcache server into the 'Servers' box, and type 'xxx_' into the Prefix box, then create the store.

      • Store should be created OK.

      4. Click 'Edit store' next to the new store from the list.

      • Form should appear again, with 'xxx_' as prefix.

      5. Change prefix to 'yyy_' and save. Edit it again.

      • Should now show 'yyy_' to verify that edit worked.

      6. Cancel edit.

      7. Under 'Database meta information', edit settings and set the store to the newly-created memcache store.

      8. Browse a few pages to check that the cache appears to be working.

      9. If necessary, turn on 'performance info' in server settings (it's under debugging). Look at bottom of page.

      • Check that the 'core/databasemeta' cache information includes a line about cachestore_memcache and that it is mostly getting cache hits if you reload a page. (This is just to check it's really being used!)

      10. To simulate what would happen if you already had a memcache installed before the upgrade, manually edit the config file in muc/config.php inside your dataroot. Scroll down to find the one you added and manually delete the line: " 'prefix' => 'yyy_',".

      11. Reload the Moodle caching settings page.

      • Check the page manages to load.

      12. Edit the store you added.

      • Check it says mdl_ on the settings page.
      Show
      1. In admin settings, go to Plugins/Caching/Configuration. 2. Under 'memcache', choose 'Add instance'. There should be a 'Key prefix' field with a help button, limited to 5 characters. The default value for this field is 'mdl_'. 3. Type an arbitrary name for the store, type a working memcache server into the 'Servers' box, and type 'xxx_' into the Prefix box, then create the store. Store should be created OK. 4. Click 'Edit store' next to the new store from the list. Form should appear again, with 'xxx_' as prefix. 5. Change prefix to 'yyy_' and save. Edit it again. Should now show 'yyy_' to verify that edit worked. 6. Cancel edit. 7. Under 'Database meta information', edit settings and set the store to the newly-created memcache store. 8. Browse a few pages to check that the cache appears to be working. 9. If necessary, turn on 'performance info' in server settings (it's under debugging). Look at bottom of page. Check that the 'core/databasemeta' cache information includes a line about cachestore_memcache and that it is mostly getting cache hits if you reload a page. (This is just to check it's really being used!) 10. To simulate what would happen if you already had a memcache installed before the upgrade, manually edit the config file in muc/config.php inside your dataroot. Scroll down to find the one you added and manually delete the line: " 'prefix' => 'yyy_',". 11. Reload the Moodle caching settings page. Check the page manages to load. 12. Edit the store you added. Check it says mdl_ on the settings page.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-39448-m24
    • Pull Master Branch:
      MDL-39448-master
    • Rank:
      50108

      Description

      If you want to use different Moodle instances with the same memcache server, it is necessary for keys to have different names.

      At present, all keys are prefixed with 'mdl_' (see MDL-38205). I propose to make this configurable when adding a memcache instance.

        Activity

        Hide
        Sam Marshall added a comment -

        Just writing a test script for my new code.

        Show
        Sam Marshall added a comment - Just writing a test script for my new code.
        Hide
        Sam Marshall added a comment -

        Change was simple and cherry-picks cleanly to 2.4 branch. (Note: I'd like this change in 2.4 because it seems to be pretty vital for users with multiple Moodle instances on same infrastructure, so it's kind of like a bug that you couldn't do it already...)

        Show
        Sam Marshall added a comment - Change was simple and cherry-picks cleanly to 2.4 branch. (Note: I'd like this change in 2.4 because it seems to be pretty vital for users with multiple Moodle instances on same infrastructure, so it's kind of like a bug that you couldn't do it already...)
        Hide
        Sam Marshall added a comment -

        Added Sam Hemelryk to watchers, hoping you might do the peer review here

        This change seems quite straightforward.

        Note about one detail: I restricted prefix length to 5 characters. Based on the comments from MDL-38205, it seems like the safe key length limit is 250 characters; the existing code with 'mdl_' prefix (4 chars) would never have been more than 249, so it should be safe to use up to 5 chars.

        Show
        Sam Marshall added a comment - Added Sam Hemelryk to watchers, hoping you might do the peer review here This change seems quite straightforward. Note about one detail: I restricted prefix length to 5 characters. Based on the comments from MDL-38205 , it seems like the safe key length limit is 250 characters; the existing code with 'mdl_' prefix (4 chars) would never have been more than 249, so it should be safe to use up to 5 chars.
        Hide
        Sam Hemelryk added a comment -

        Spot on thanks Sam, definitely a feature worth having.
        I think someone had mentioned it to me in the past but I'd never got around to doing anything about it.

        You code looks perfect, so straight up for integration.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Spot on thanks Sam, definitely a feature worth having. I think someone had mentioned it to me in the past but I'd never got around to doing anything about it. You code looks perfect, so straight up for integration. Many thanks Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Side note: Reviewing this leaded to remember that MDL-37500 has not landed yet, grrr and it was supposed to be the solution for any shared/unique configuration (2.5). We missed that completely, super-grrr^3.

        Show
        Eloy Lafuente (stronk7) added a comment - Side note: Reviewing this leaded to remember that MDL-37500 has not landed yet, grrr and it was supposed to be the solution for any shared/unique configuration (2.5). We missed that completely, super-grrr^3.
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        Urm.. I was just about to complain to you about adding this to the memcached plugin too, but I notice this already has it!?

        So, just double checking that there isn't something we can do with these two plugins to get better code shairng?

        Show
        Dan Poltawski added a comment - Urm.. I was just about to complain to you about adding this to the memcached plugin too, but I notice this already has it!? So, just double checking that there isn't something we can do with these two plugins to get better code shairng?
        Hide
        Sam Hemelryk added a comment -

        Hi Dan,

        The Memcached extension has a specific prefix option that gets provided when constructing the connection to Memcache service.
        The Memcache extension on the other hand provides no such option so we must "prefix" it to the key before every interaction with the Memcache service.

        While the two have nearly identical interfaces the backend code is different.

        When writing the two plugins I considered whether there would be a good way to share code between the two plugins.
        However as time was pressing to get it in and they're pretty simple plugins to write I choose to just create them entirely separate.
        The actual idea is a good one, the plugins do share quite a bit of logic however I'm not overly sure how practical it would be.
        I don't think having one depend on the other is a good idea. They both use the Memcache service but are both entirely individual extensions. They share no dependency and I hate the idea of introducing one.
        However there would perhaps be merit in having an abstract class that extends base and interacts with fully self managing extension.
        Each plugin would have to override the action methods anyway (get,set,delete) as they'll no doubt have their own calls to make but perhaps we could abstract some of the preceeding logic to that middle layer between the cache_store class and the plugin class. Perhaps a cache_store_managed_extension class or something.
        Either way I think that would definitely be better discussed and tackled as a separate issue.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hi Dan, The Memcached extension has a specific prefix option that gets provided when constructing the connection to Memcache service. The Memcache extension on the other hand provides no such option so we must "prefix" it to the key before every interaction with the Memcache service. While the two have nearly identical interfaces the backend code is different. When writing the two plugins I considered whether there would be a good way to share code between the two plugins. However as time was pressing to get it in and they're pretty simple plugins to write I choose to just create them entirely separate. The actual idea is a good one, the plugins do share quite a bit of logic however I'm not overly sure how practical it would be. I don't think having one depend on the other is a good idea. They both use the Memcache service but are both entirely individual extensions. They share no dependency and I hate the idea of introducing one. However there would perhaps be merit in having an abstract class that extends base and interacts with fully self managing extension. Each plugin would have to override the action methods anyway (get,set,delete) as they'll no doubt have their own calls to make but perhaps we could abstract some of the preceeding logic to that middle layer between the cache_store class and the plugin class. Perhaps a cache_store_managed_extension class or something. Either way I think that would definitely be better discussed and tackled as a separate issue. Many thanks Sam
        Hide
        Dan Poltawski added a comment -

        Thanks Sam.

        Integrated to master and 24.

        Note that I changed the version number on 24 so that it did not increment the date part (its possible that version updates on master might be skipped by doing that). I didn't check and in practice it probably didn't matter, but i think its better practice that we branch version numbers on branches too.

        Show
        Dan Poltawski added a comment - Thanks Sam. Integrated to master and 24. Note that I changed the version number on 24 so that it did not increment the date part (its possible that version updates on master might be skipped by doing that). I didn't check and in practice it probably didn't matter, but i think its better practice that we branch version numbers on branches too.
        Hide
        Sam Marshall added a comment -

        Thanks Dan - sorry you are right about the version number, I should have remembered that.

        Show
        Sam Marshall added a comment - Thanks Dan - sorry you are right about the version number, I should have remembered that.
        Hide
        Rajesh Taneja added a comment -

        Thanks for fixing that Sam, works Grt.

        Few observations which might be of interest:

        1. Special chars in prefix get stripped, and if prefix is created with special chars, then no validation is done to notify user. End result is default prefix (mdl_)
        2. While Editing mapping for Databasemeta there is no way to save final store as default_application, not sure why default_application option is visible. Also, Store used when no mapping is present is always default_application.

        Passing this as above observations are not related to this issue.

        Show
        Rajesh Taneja added a comment - Thanks for fixing that Sam, works Grt. Few observations which might be of interest: Special chars in prefix get stripped, and if prefix is created with special chars, then no validation is done to notify user. End result is default prefix (mdl_) While Editing mapping for Databasemeta there is no way to save final store as default_application, not sure why default_application option is visible. Also, Store used when no mapping is present is always default_application. Passing this as above observations are not related to this issue.
        Hide
        Sam Marshall added a comment -

        Thanks Rajesh.

        Re 1 this is because I used PARAM_ALPHAEXT in the form field; there are probably plenty of Moodle forms that have similar behaviour. I agree it's a bit questionable - I wonder if, as a general change to Moodle forms, any of the 'setParam' stuff should actually result in automatic validation (i.e. showing an actual error by the form field if it doesn't match the param) rather than only using the PARAM_xx constant when getting values to automatically strip them. Hmm. Well, will leave that for others.

        Show
        Sam Marshall added a comment - Thanks Rajesh. Re 1 this is because I used PARAM_ALPHAEXT in the form field; there are probably plenty of Moodle forms that have similar behaviour. I agree it's a bit questionable - I wonder if, as a general change to Moodle forms, any of the 'setParam' stuff should actually result in automatic validation (i.e. showing an actual error by the form field if it doesn't match the param) rather than only using the PARAM_xx constant when getting values to automatically strip them. Hmm. Well, will leave that for others.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Did you think this day was not going to arrive ever?

        Your patience has been rewarded, yay, sent upstream, thanks!

        Closing...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: