Moodle
  1. Moodle
  2. MDL-36512

Purging a file store actually purges nothing

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide
      1. Open Site administration ► Plugins ► Caching ► Configuration
      2. Go to the dataroot filesystem and take note of the time of the folders/files representing the cache e.g. # cd $CFG->cachedir/cache/cachestore_file/default_application && ls -lh */
      3. Purge the Default file store for application caches
      4. Check the file time: it should return the current time
      5. Create a new file store (Add instance at the end of the row identified by column Plugin = File cache) in a Cache path really different from the default one e.g. /tmp/moodle-master-cache-file1
      6. Edit the Language string cache mapping and point it to the new file store
      7. Again: take note of the time of the cached files, wait at least 1 minute (to take advantage of reading the time with ls -lh), purge the new file store and re-check the time of the files
      Show
      Open Site administration ► Plugins ► Caching ► Configuration Go to the dataroot filesystem and take note of the time of the folders/files representing the cache e.g. # cd $CFG->cachedir/cache/cachestore_file/default_application && ls -lh */ Purge the Default file store for application caches Check the file time: it should return the current time Create a new file store ( Add instance at the end of the row identified by column Plugin = File cache ) in a Cache path really different from the default one e.g. /tmp/moodle-master-cache-file1 Edit the Language string cache mapping and point it to the new file store Again: take note of the time of the cached files, wait at least 1 minute (to take advantage of reading the time with ls -lh ), purge the new file store and re-check the time of the files
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      m24_MDL-36512_Purging_file_cachestore_does_not_purge_anything
    • Rank:
      45974

      Description

      This has been discovered while working on MDL-36363

        Issue Links

          Activity

          Hide
          Matteo Scaramuccia added a comment -

          My patch proposal moves the definition of a store to be a real one and not an adhoc one: more, it gets all the definitions working on top of that file store.
          This will help cachestore_file classes to work as expected after having being {initialise}}d: I'm just learning MUC but I feel confident that this could be the right solution.

          Show
          Matteo Scaramuccia added a comment - My patch proposal moves the definition of a store to be a real one and not an adhoc one: more, it gets all the definitions working on top of that file store. This will help cachestore_file classes to work as expected after having being {initialise}}d: I'm just learning MUC but I feel confident that this could be the right solution.
          Hide
          Sam Hemelryk added a comment -

          Good spotting! Thanks for reporting the issue and providing a fix Matteo, I'll look at it right now.

          Show
          Sam Hemelryk added a comment - Good spotting! Thanks for reporting the issue and providing a fix Matteo, I'll look at it right now.
          Hide
          Sam Hemelryk added a comment -

          Thanks Matteo, in general I think the approach you've taken here is the correct one and certainly the code is nearly flawless.

          It took me a bit to get through this one, I think that after release I/we/someone is going to have to spend a bit of time looking at how purging is happening and whether we can clean up what is going on presently.
          There are a couple of things about the current approach that arn't ideal (not your work, but the purging of caches as a whole).

          From this work I noted only a couple of really minor things:

          1. We don't use underscores in variable names (as per our coding style guidelines) so $assoc_definition_mappings and $definition_mappings should be renamed.
          2. Typo in there: $istance
          3. The code would be a little more readable if that first foreach in purge_store were removed in favor something like the following:
            $stores = $config->get_all_stores();
            if (!array_key_exists($storename, $stores)) {
                // The store does not exist.
                return false;
            }
            $store = $stores[$storename];
            

          Other than that spot on!
          I thought about just making those changes and putting it up but I saw you mention you had based one of your other branches on this branch so thought it best I let you make those changes so I don't break things for you.

          BTW I believe Michael de Raadt added you to the Moodle Developers group in tracker. Congratulations
          You should now be able to make yourself and assignee, put things up for peer-review when you are ready for them to be reviewed.

          Many thanks once more
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Matteo, in general I think the approach you've taken here is the correct one and certainly the code is nearly flawless. It took me a bit to get through this one, I think that after release I/we/someone is going to have to spend a bit of time looking at how purging is happening and whether we can clean up what is going on presently. There are a couple of things about the current approach that arn't ideal (not your work, but the purging of caches as a whole). From this work I noted only a couple of really minor things: We don't use underscores in variable names (as per our coding style guidelines) so $assoc_definition_mappings and $definition_mappings should be renamed. Typo in there: $istance The code would be a little more readable if that first foreach in purge_store were removed in favor something like the following: $stores = $config->get_all_stores(); if (!array_key_exists($storename, $stores)) { // The store does not exist. return false ; } $store = $stores[$storename]; Other than that spot on! I thought about just making those changes and putting it up but I saw you mention you had based one of your other branches on this branch so thought it best I let you make those changes so I don't break things for you. BTW I believe Michael de Raadt added you to the Moodle Developers group in tracker. Congratulations You should now be able to make yourself and assignee, put things up for peer-review when you are ready for them to be reviewed. Many thanks once more Sam
          Hide
          Matteo Scaramuccia added a comment -

          Hi Sam,
          in these days I'm at home with the flu so spare time has been increased even though I'm quite slow for reasoning. I've just pushed a proposal which should address your points and go futher looking at purge_by_* too.
          The last commit, is something new and I guess it should be addressed in another issue: shortly, $aggregate is missing in purge_by_definition - BC breaking, another reason for having a separate issue - and it is required since it contributes in defining the id of the definition which can be used in a cache store - cachestore_file uses it to create the main per-definition subfolders!
          Besides there is potentially - I'm supposing and frankly really not sure of what should be correct - lack of consistency around $aggregate: in cache_factory->create_cache_from_definition $definitionname doesn't contribute to define the id for cachesfromdefinitions while create_definition uses it.

          Summary:

          1. Commit 1-2-3: they should implement the fix, covered by the testing instructions
          2. Commit 4: it is consistent with the first 3 commits, required if the event acts over a cachestore_file
          3. Commit 5: it should be like Commit 4 but $aggregate gives me more headache than flu

          I'm using single commits: I think it will help you in looking at the differences. At the end of the peer review cycles it would be probably better to squash everything before integrating.

          BTW, TNX Michael, really appreciated!

          Show
          Matteo Scaramuccia added a comment - Hi Sam, in these days I'm at home with the flu so spare time has been increased even though I'm quite slow for reasoning. I've just pushed a proposal which should address your points and go futher looking at purge_by_* too. The last commit , is something new and I guess it should be addressed in another issue: shortly, $aggregate is missing in purge_by_definition - BC breaking, another reason for having a separate issue - and it is required since it contributes in defining the id of the definition which can be used in a cache store - cachestore_file uses it to create the main per-definition subfolders! Besides there is potentially - I'm supposing and frankly really not sure of what should be correct - lack of consistency around $aggregate : in cache_factory->create_cache_from_definition $definitionname doesn't contribute to define the id for cachesfromdefinitions while create_definition uses it. Summary: Commit 1-2-3 : they should implement the fix, covered by the testing instructions Commit 4 : it is consistent with the first 3 commits, required if the event acts over a cachestore_file Commit 5 : it should be like Commit 4 but $aggregate gives me more headache than flu I'm using single commits: I think it will help you in looking at the differences. At the end of the peer review cycles it would be probably better to squash everything before integrating. BTW, TNX Michael, really appreciated!
          Hide
          Sam Hemelryk added a comment -

          Hi Matteo,

          Sorry to hear about the flu!

          The changes look spot on thanks.
          Hehe there is one thing that has been raised, coding style guidelines again. If we use a TODO in code we are required to create an issue in tracker and refer to it within the todo.
          This way we can be sure that things aren't forgotten (and that we don't need to fix it now).

          // TODO MDL-XXXXX: Change the signature: $aggregate must be added.
          

          I really like the abstract of the code calculating the definitions using a store to a method of the config object, thats a good abstraction!

          In regards to the commits that is up to you.
          Generally we let the developer decide as to whether they want to squash it or not providing there are a reasonable number of commits and that each addresses a clear task/issue.
          Providing thats the case if you'd like to squash it thats great and if you'd like to preserve history thats just as great.
          If there is an issue either we'll squash it during integration or we'll ask you to squash it once the integration review is under way.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Matteo, Sorry to hear about the flu! The changes look spot on thanks. Hehe there is one thing that has been raised, coding style guidelines again. If we use a TODO in code we are required to create an issue in tracker and refer to it within the todo. This way we can be sure that things aren't forgotten (and that we don't need to fix it now). // TODO MDL-XXXXX: Change the signature: $aggregate must be added. I really like the abstract of the code calculating the definitions using a store to a method of the config object, thats a good abstraction! In regards to the commits that is up to you. Generally we let the developer decide as to whether they want to squash it or not providing there are a reasonable number of commits and that each addresses a clear task/issue. Providing thats the case if you'd like to squash it thats great and if you'd like to preserve history thats just as great. If there is an issue either we'll squash it during integration or we'll ask you to squash it once the integration review is under way. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          BTW once you've created an issue for the aggregation thing, and have added the MDL's to the last commit feel free to push this up for integration

          I've just checked with Michael in regards to that. As a member of the developers group you are able to push things up for integration review. We ask that you always get a peer-review from the component maintainer if there is one, or from someone at HQ if there isn't (you've done that already for this issue which is great, just noting for future reference)

          Show
          Sam Hemelryk added a comment - BTW once you've created an issue for the aggregation thing, and have added the MDL's to the last commit feel free to push this up for integration I've just checked with Michael in regards to that. As a member of the developers group you are able to push things up for integration review. We ask that you always get a peer-review from the component maintainer if there is one, or from someone at HQ if there isn't (you've done that already for this issue which is great, just noting for future reference)
          Hide
          Matteo Scaramuccia added a comment -

          Hi Sam,
          I've added a new issue (MDL-36660) as well as squashed the 5 commits into 2: with regards to your peer review, the first commit covers previous commits 1-3 while the second commits 4-5.

          TNX for your pointers about Moodle coding style and workflow, they will be always appreciated!

          Show
          Matteo Scaramuccia added a comment - Hi Sam, I've added a new issue ( MDL-36660 ) as well as squashed the 5 commits into 2: with regards to your peer review, the first commit covers previous commits 1-3 while the second commits 4-5. TNX for your pointers about Moodle coding style and workflow, they will be always appreciated!
          Hide
          Matteo Scaramuccia added a comment -

          Silly question from a newbie, please be patient : I've closed the development thinking that a button like Request integration review will appear next to Request peer review, then nothing. What am I supposed to do to perform that request? I've already looked at http://docs.moodle.org/dev/Tracker_guide.

          Show
          Matteo Scaramuccia added a comment - Silly question from a newbie, please be patient : I've closed the development thinking that a button like Request integration review will appear next to Request peer review , then nothing. What am I supposed to do to perform that request? I've already looked at http://docs.moodle.org/dev/Tracker_guide .
          Hide
          Sam Hemelryk added a comment -

          Hi Matteo,

          Tracker only shows two buttons there normally, and next to them a workflow drop down that will contain the other actions you can perform on the issue.
          The button/action you are looking for is "Submit for integration review", if its not one of the two buttons it should be under the workflow drop down.
          If its no where to be found I'll move this for you and then have a chat to Michael about it and find out whether that is expected or something we need to change.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Matteo, Tracker only shows two buttons there normally, and next to them a workflow drop down that will contain the other actions you can perform on the issue. The button/action you are looking for is "Submit for integration review", if its not one of the two buttons it should be under the workflow drop down. If its no where to be found I'll move this for you and then have a chat to Michael about it and find out whether that is expected or something we need to change. Many thanks Sam
          Hide
          Matteo Scaramuccia added a comment -

          Hi Sam,
          my actions, for the current status, are:

          1. Start development
          2. Close issue
          Show
          Matteo Scaramuccia added a comment - Hi Sam, my actions, for the current status, are: Start development Close issue
          Hide
          Sam Hemelryk added a comment -

          Aha no probs Matteo, I've pushed this up for integration review now.
          I'll chat to Michael when he comes online today and see what the deal is.

          Cheers

          Show
          Sam Hemelryk added a comment - Aha no probs Matteo, I've pushed this up for integration review now. I'll chat to Michael when he comes online today and see what the deal is. Cheers
          Hide
          Dan Poltawski added a comment -

          Thanks Matteo, i've integrated this now. (Not MDL-36660 as I accidentally closed and cause da load of noise!)

          Show
          Dan Poltawski added a comment - Thanks Matteo, i've integrated this now. (Not MDL-36660 as I accidentally closed and cause da load of noise!)
          Hide
          Jason Fowler added a comment -

          All good Matteo!

          Show
          Jason Fowler added a comment - All good Matteo!
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: