Details

    • Testing Instructions:
      Hide

      Test 1:

      1. Backup a course (Go to course and click backup), no error should be produced and course should be backed up.
      2. Restore same course and make sure course is restored properly.

      Test2:

      1. Backup multiple course. Open multiple/different browsers and backup different courses at the same time
      2. restore these courses at the same time in different browsers and make sure they get restored properly.

      Test 3:

      1. Set automated backup (Settings -> Site administrator -> Courses -> Backups -> Automated backup setup)
      2. Run cron (/admin/cron.php), no error should be produced.

      Test 4:
      Run phpunit and make sure you don't get any error (especially backup/converter/moodle1/tests/lib_test.php)

      Show
      Test 1: Backup a course (Go to course and click backup), no error should be produced and course should be backed up. Restore same course and make sure course is restored properly. Test2: Backup multiple course. Open multiple/different browsers and backup different courses at the same time restore these courses at the same time in different browsers and make sure they get restored properly. Test 3: Set automated backup (Settings -> Site administrator -> Courses -> Backups -> Automated backup setup) Run cron (/admin/cron.php), no error should be produced. Test 4: Run phpunit and make sure you don't get any error (especially backup/converter/moodle1/tests/lib_test.php)
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-27120
    • Rank:
      6469

      Description

      Backup / restore is performing slowly which is causing many imports to fail for large courses.

      In addition to this, an interactive backup/restore/import does not increase max_execution_time, so max_execution_time will remain at PHP's default (which could be something like 60 secs). In Moodle 1.9, both backup and restore would set max execution time to 3000 (50 minutes).

      I have also identified 3 major bottlenecks so far in the backup code (see attached profiles for further evidence):
      1) base_plan::get_setting() function does a sequential scan lookup to find settings, instead of hash-based, which was very slow and called $setting->get_name() over 1 million times and taking ~30s (see get_setting.png)
      2) The various backup_ui_stage classes in backup/util/ui/backup_ui_stage.class.php each contain an initialise_stage_form() method which is using QuickForms in an inefficient way. It calls $form->add_setting() which calls to moodle_form->setDefault which calls QuickForm->setDefaults which fires an "updateValue" even to all quickform elements. So it has n*n complexity (see quickforms1.png and quickforms2.png)
      3) The backup_xml_transformer's constructor calls register_links_encoders() which causes an expensive call to get_plugins_list(). The problem is, the class can be constructed 1500+ times during a backup, so it needs a cache! (see getplugins1.png and getplugins2.png)

      I have patches for the above 3 issues which I will put on GitHub and link here.

      1. get_setting.png
        36 kB
      2. getplugins1.png
        24 kB
      3. getplugins2.png
        24 kB
      4. quickforms1.png
        64 kB
      5. quickforms2.png
        59 kB

        Issue Links

          Activity

          Hide
          Ashley Holman added a comment -

          Patches are now up at https://github.com/ashleyholman/moodle/commits/mdl-27120

          Any opinion on upping the max execution time? Perhaps set to 50 minutes like in 1.9?

          Show
          Ashley Holman added a comment - Patches are now up at https://github.com/ashleyholman/moodle/commits/mdl-27120 Any opinion on upping the max execution time? Perhaps set to 50 minutes like in 1.9?
          Hide
          Tony Levi added a comment -

          Some restore optimisations going on
          https://github.com/tlevi/moodle/tree/mdl27120

          (some more to come...)

          Show
          Tony Levi added a comment - Some restore optimisations going on https://github.com/tlevi/moodle/tree/mdl27120 (some more to come...)
          Hide
          Tony Levi added a comment -

          Execution time and patches here so far should be applied sooner rather than never.

          Further work can be done later if it is still needed.

          Show
          Tony Levi added a comment - Execution time and patches here so far should be applied sooner rather than never. Further work can be done later if it is still needed.
          Hide
          Michael Blake added a comment -

          This issue has been reported by a MP, please give it priority.

          Show
          Michael Blake added a comment - This issue has been reported by a MP, please give it priority.
          Hide
          Tim Lock added a comment -

          Recent testing has shown increasing timeout from 30 seconds to 60 seconds in XML Parser, Zip Class & Quick Form has helped avoid the timeouts.

          Show
          Tim Lock added a comment - Recent testing has shown increasing timeout from 30 seconds to 60 seconds in XML Parser, Zip Class & Quick Form has helped avoid the timeouts.
          Hide
          Jason Ilicic added a comment -

          Found an issue: When running through the backup wizard and unticking "Include enrolled users", the form does not automatically disable the dependencies except for the last one. This is due to a problem where only the last item is being added as a dependency.

          Fix is on its way

          Show
          Jason Ilicic added a comment - Found an issue: When running through the backup wizard and unticking "Include enrolled users", the form does not automatically disable the dependencies except for the last one. This is due to a problem where only the last item is being added as a dependency. Fix is on its way
          Hide
          Ashley Holman added a comment -

          the above fix by Jason has been included in the mdl-27120 github branch

          Show
          Ashley Holman added a comment - the above fix by Jason has been included in the mdl-27120 github branch
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, guys.

          Chris Follin pointed out that this had not been placed on any backlog, and as such it's likely that it has been overlooked.

          I've put this in line for some attention in the next sprint.

          Show
          Michael de Raadt added a comment - Thanks for working on this, guys. Chris Follin pointed out that this had not been placed on any backlog, and as such it's likely that it has been overlooked. I've put this in line for some attention in the next sprint.
          Hide
          moodle.com added a comment -

          Assigning to Raj for integration

          Show
          moodle.com added a comment - Assigning to Raj for integration
          Hide
          Rajesh Taneja added a comment -

          Thanks Everyone,

          I have created branch and will review it, before pushing it up through process.

          Show
          Rajesh Taneja added a comment - Thanks Everyone, I have created branch and will review it, before pushing it up through process.
          Hide
          Rajesh Taneja added a comment -

          Thanks for providing spot-on patch
          Adding Eloy as peer-reviewer to make sure I didn't miss anything.

          Show
          Rajesh Taneja added a comment - Thanks for providing spot-on patch Adding Eloy as peer-reviewer to make sure I didn't miss anything.
          Hide
          Jonathan Champ added a comment -

          To de-duplicate some code, may want to make add_setting() a wrapper function for the new add_settings() function:

          function add_setting(backup_setting $setting, base_task $task=null)

          { return $this->add_settings(array(array($setting, $task))); }
          Show
          Jonathan Champ added a comment - To de-duplicate some code, may want to make add_setting() a wrapper function for the new add_settings() function: function add_setting(backup_setting $setting, base_task $task=null) { return $this->add_settings(array(array($setting, $task))); }
          Hide
          Rajesh Taneja added a comment -

          Thanks Jonathan,

          Probably this can be put up as new bug for code optimization and then backup can be looked throughly to avoid code duplication.
          This issue is more related to performance, so probably I will avoid this, for now.

          Show
          Rajesh Taneja added a comment - Thanks Jonathan, Probably this can be put up as new bug for code optimization and then backup can be looked throughly to avoid code duplication. This issue is more related to performance, so probably I will avoid this, for now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi guys, I like what I've seen there... also Tony's one about backup_ids... but there is one problem and it's that we are rejecting any new use of static caches until it's decided if finally the centralised MUC is going to be implemented (MDL-25290).

          I've been talking with SamH a bit about this and we are going to push a bit about the MUC @ HQ, to know if/when it's going to be created and, depending of that, for sure we'll get a better idea about how to implement those static caches.

          So really, to allow this to land... I'd propose to:

          1) take out all the static-caches related parts from this issue/patch. Integrate it normally.
          2) create as many new issues as necessary to put on them each one of the static-cache solutions provided here.
          3) Make those issues created in 2) to be "blocked by" (link) the MUC issue. So we'll start accumulating good places where the MUC can be useful.

          Many thanks all, I hope we get some directions about the MUC soon, because it's really a shame to stop accepting solutions like the provided in this patch. But, at the same time, is good to have them "pressing" towards the MUC implementation.

          Let's see...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi guys, I like what I've seen there... also Tony's one about backup_ids... but there is one problem and it's that we are rejecting any new use of static caches until it's decided if finally the centralised MUC is going to be implemented ( MDL-25290 ). I've been talking with SamH a bit about this and we are going to push a bit about the MUC @ HQ, to know if/when it's going to be created and, depending of that, for sure we'll get a better idea about how to implement those static caches. So really, to allow this to land... I'd propose to: 1) take out all the static-caches related parts from this issue/patch. Integrate it normally. 2) create as many new issues as necessary to put on them each one of the static-cache solutions provided here. 3) Make those issues created in 2) to be "blocked by" (link) the MUC issue. So we'll start accumulating good places where the MUC can be useful. Many thanks all, I hope we get some directions about the MUC soon, because it's really a shame to stop accepting solutions like the provided in this patch. But, at the same time, is good to have them "pressing" towards the MUC implementation. Let's see...ciao
          Hide
          Tony Levi added a comment -

          The static caches are very significant improvements, this situation can't keep up as it is holding up performance all over the place.

          Re MDL-25290 - I've proposed some things about that to provoke progress but there is very little discussion, I'm not sure if something different is needed...

          Show
          Tony Levi added a comment - The static caches are very significant improvements, this situation can't keep up as it is holding up performance all over the place. Re MDL-25290 - I've proposed some things about that to provoke progress but there is very little discussion, I'm not sure if something different is needed...
          Hide
          Adam Olley added a comment -

          It should be noted that these are some horribly inefficient things that are fixed by a simple static cache. Is it really wise to just leave the performance of backup/restore as horrible while we wait for something that sounds like its ages off? It seems like you're going to have to do an audit of all uses of the static keyword throughout Moodle anyway when MUC is finally implemented...

          Show
          Adam Olley added a comment - It should be noted that these are some horribly inefficient things that are fixed by a simple static cache. Is it really wise to just leave the performance of backup/restore as horrible while we wait for something that sounds like its ages off? It seems like you're going to have to do an audit of all uses of the static keyword throughout Moodle anyway when MUC is finally implemented...
          Hide
          Rajesh Taneja added a comment -

          Without Static cache, there is not much improvement in the patch. I agree with Tony and Adam here. Probably, we can have a mdl to remove static and link it to MDL-25290.

          Show
          Rajesh Taneja added a comment - Without Static cache, there is not much improvement in the patch. I agree with Tony and Adam here. Probably, we can have a mdl to remove static and link it to MDL-25290 .
          Hide
          Rajesh Taneja added a comment -

          Moving it to next sprint to keep this up, and if required remove static usage.

          Show
          Rajesh Taneja added a comment - Moving it to next sprint to keep this up, and if required remove static usage.
          Hide
          Rajesh Taneja added a comment -

          Chat log with Eloy.
          (09:19:56) Rajesh Taneja: Can you please give your final say on MDL-27120, before I do some changes. I don't see any point in going with this if we plan to remove static
          (09:22:55) Eloy: well, last week I was thinking that... perhaps we could accept them if:
          1) all them are limited
          2) all them are reset. Imagine running multiple backups in a loop.
          3) we add to all them one big TODO about to move them to the MUC once it's available (2.3 only surely).
          (09:24:19) Rajesh Taneja: Sure, I will try to test multiple backups and see if it all pass...
          (09:24:40) Rajesh Taneja: And subtask to MCU
          (09:24:44) Rajesh Taneja: Thanks Eloy.

          Show
          Rajesh Taneja added a comment - Chat log with Eloy. (09:19:56) Rajesh Taneja: Can you please give your final say on MDL-27120 , before I do some changes. I don't see any point in going with this if we plan to remove static (09:22:55) Eloy: well, last week I was thinking that... perhaps we could accept them if: 1) all them are limited 2) all them are reset. Imagine running multiple backups in a loop. 3) we add to all them one big TODO about to move them to the MUC once it's available (2.3 only surely). (09:24:19) Rajesh Taneja: Sure, I will try to test multiple backups and see if it all pass... (09:24:40) Rajesh Taneja: And subtask to MCU (09:24:44) Rajesh Taneja: Thanks Eloy.
          Hide
          Rajesh Taneja added a comment - - edited

          Couldn't found any problem running simultanious backup and restore, hence pushing it for integration review.

          Will add task to MUC, after this gets integrated.

          Show
          Rajesh Taneja added a comment - - edited Couldn't found any problem running simultanious backup and restore, hence pushing it for integration review. Will add task to MUC, after this gets integrated.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Rajesh Taneja added a comment -

          Branch re-based.

          Show
          Rajesh Taneja added a comment - Branch re-based.
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,
          I've had a look and a few notes :

          • backup/util/ui/base_plan.class.php : get_setting_name() doc blocks describes "Throws exception if multiple are found". This comment needs to be updated here and in any docs that are out there. Also this change will have to be verified in all usages depending on this function for checking of multiple names. Very quickly i see settings_exists() below depending on base_plan_exception from get_setting() - that will now return true even with duplicates instead of returning false.
          • backup/moodle2/backup_xml_transformer.class.php - class backup_xml_transformer : the $LINK_ENCODERS_CACHE global cache in __contruct() might be better off being updated in register_link_encoders() , thereby ensuring any other possible calls to the function to actually use the cache rather than only checking cache within __construct(). (
          • backup/util/ui/base_moodleform.class.php : can we modify add_setting() here to do the duplicated code bit (add_settings()) but return the $defaults if passed an optional argument? this could help reduce the duplication of code here. so heres a thought:
            function add_setting(backup_setting $setting, base_task $task=null, $returndefault=false) {
                    ...
                    if($returndefault) {
                            //feel free to optimize more alls here even
                            return array($setting->get_ui_name(), $setting->get_value());
                    } else {
                            //calls that can be optimized are here.
                            //perhaps we need to implement more 'function set.*s(' plural type functions for moodleforms?
                            $this->_form->setDefault($setting->get_ui_name(), $setting->get_value());
                    }
                    ...
            }
            
            function add_settings(array $settingstasks) {
             global $OUTPUT;
             
             $defaults = array();
             foreach ($settingstasks as $task) {
                    $defaults[] = $this->add_setting(backup_setting $setting, base_task $task=null, true) 
             }
             $this->_form->setDefaults($defaults);
             return true;
            }
            

          Lets see where we go with possible improvements from this issue

          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - Hi Raj, I've had a look and a few notes : backup/util/ui/base_plan.class.php : get_setting_name() doc blocks describes "Throws exception if multiple are found". This comment needs to be updated here and in any docs that are out there. Also this change will have to be verified in all usages depending on this function for checking of multiple names. Very quickly i see settings_exists() below depending on base_plan_exception from get_setting() - that will now return true even with duplicates instead of returning false. backup/moodle2/backup_xml_transformer.class.php - class backup_xml_transformer : the $LINK_ENCODERS_CACHE global cache in __contruct() might be better off being updated in register_link_encoders() , thereby ensuring any other possible calls to the function to actually use the cache rather than only checking cache within __construct(). ( backup/util/ui/base_moodleform.class.php : can we modify add_setting() here to do the duplicated code bit (add_settings()) but return the $defaults if passed an optional argument? this could help reduce the duplication of code here. so heres a thought: function add_setting(backup_setting $setting, base_task $task= null , $returndefault= false ) { ... if ($returndefault) { //feel free to optimize more alls here even return array($setting->get_ui_name(), $setting->get_value()); } else { //calls that can be optimized are here. //perhaps we need to implement more 'function set.*s(' plural type functions for moodleforms? $ this ->_form->setDefault($setting->get_ui_name(), $setting->get_value()); } ... } function add_settings(array $settingstasks) { global $OUTPUT; $defaults = array(); foreach ($settingstasks as $task) { $defaults[] = $ this ->add_setting(backup_setting $setting, base_task $task= null , true ) } $ this ->_form->setDefaults($defaults); return true ; } Lets see where we go with possible improvements from this issue cheers, Aparup
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'd also point about these two (commented above):

          https://github.com/tlevi/moodle/compare/6b14adf210acf895e2e9d4309b4c104abd3535fa...mdl27120

          Both need to be amended with a big // TODO pointing to the MDL-25290 one.

          And the ids one needs to be reset on restoreid change (to allow multiple restore operations to work in the same request). But I think both are really good enough.

          In fact they sound "more important" (in my mind) than the settings and encoders ones present in current patch.

          All them are welcome, of course, once they are amended / corrected.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'd also point about these two (commented above): https://github.com/tlevi/moodle/compare/6b14adf210acf895e2e9d4309b4c104abd3535fa...mdl27120 Both need to be amended with a big // TODO pointing to the MDL-25290 one. And the ids one needs to be reset on restoreid change (to allow multiple restore operations to work in the same request). But I think both are really good enough. In fact they sound "more important" (in my mind) than the settings and encoders ones present in current patch. All them are welcome, of course, once they are amended / corrected. Ciao
          Hide
          Jonathan Champ added a comment -

          Aparup: Regarding your third point about duplicated code, it would be better to have the multiple item function be the base case (because that's the one that can be optimized more effectively). See my comment from March 1st, 2012 on this issue (comment-146766).

          Show
          Jonathan Champ added a comment - Aparup: Regarding your third point about duplicated code, it would be better to have the multiple item function be the base case (because that's the one that can be optimized more effectively). See my comment from March 1st, 2012 on this issue ( comment-146766 ).
          Hide
          Rajesh Taneja added a comment -

          Thanks Jonathan, Eloy and Apu,

          I have included Tony's changes and updated docblock linking to MDL-25290.
          Also, reverted dirname($path) (backup/util/xml/parser/progressive_parser.class.php) change as it will be a problem if a specific course backup is restored simultaneously (even with different names).

          Eloy, can you please review this again for me

          Show
          Rajesh Taneja added a comment - Thanks Jonathan, Eloy and Apu, I have included Tony's changes and updated docblock linking to MDL-25290 . Also, reverted dirname($path) (backup/util/xml/parser/progressive_parser.class.php) change as it will be a problem if a specific course backup is restored simultaneously (even with different names). Eloy, can you please review this again for me
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          It's looking great, just some comments to get this ready for integration:

          A) The encoders cache:

          1. The "return $LINKS_ENCODERS_CACHE = $encoders;", better split, first assignment, later return. Not critical but...

          B) The backup-ids cache:

          1. Surely those static members ($BACKUP_IDS_CACHE and $BACKUP_IDS_EXIST) need both to be lowercase (we only allow uppers in globals) and also need to specify its scope (public/private).
          2. count() is super-slow, surely I would keep the count on a separate $BACKUP_IDS_SIZE (lowercase, of course).
          3. IMPORTANT: Unless I'm missing something... the $BACKUP_IDS_EXIST cache grows and grows without limit. While it's working because the key includes $restoreid... IMO it should be invalidated on each change of $restoreid. Imagine one automatic restore of 1000 courses, all the used backup_ids along the 1000 courses will be there => Memory boom! So perhaps you also need to store $BACKUP_IDS_LAST_RESTOREID (again, lowercased).
          4. There is also one return + assignment in the same line. Better split them.

          C) The associative settings in plan: Looks perfect. Hope it works.

          D) The bulk adding settings: Looks perfect too. Hope it works too.

          Finally, take a look to this smurf.xml file:

          http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/211/artifact/work/smurf.xml

          It contains various codechecker and phpdocs points that should be fixed too. Feel free to ignore some of the issues reported on it, basically:

          1. All the ones being warnings (weight = 1). The start with uppercase and end with full-stops ones. They are simply recommendations.
          2. The "LINKS_ENCODERS_CACHE" ones. It is a global and it's ok for it to be uppercase. I need to tidy that check.

          The rest of issues should be amended.

          And that's all. Hope it helps...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited It's looking great, just some comments to get this ready for integration: A) The encoders cache: The "return $LINKS_ENCODERS_CACHE = $encoders;", better split, first assignment, later return. Not critical but... B) The backup-ids cache: Surely those static members ($BACKUP_IDS_CACHE and $BACKUP_IDS_EXIST) need both to be lowercase (we only allow uppers in globals) and also need to specify its scope (public/private). count() is super-slow, surely I would keep the count on a separate $BACKUP_IDS_SIZE (lowercase, of course). IMPORTANT: Unless I'm missing something... the $BACKUP_IDS_EXIST cache grows and grows without limit. While it's working because the key includes $restoreid... IMO it should be invalidated on each change of $restoreid. Imagine one automatic restore of 1000 courses, all the used backup_ids along the 1000 courses will be there => Memory boom! So perhaps you also need to store $BACKUP_IDS_LAST_RESTOREID (again, lowercased). There is also one return + assignment in the same line. Better split them. C) The associative settings in plan: Looks perfect. Hope it works. D) The bulk adding settings: Looks perfect too. Hope it works too. Finally, take a look to this smurf.xml file: http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/211/artifact/work/smurf.xml It contains various codechecker and phpdocs points that should be fixed too. Feel free to ignore some of the issues reported on it, basically: All the ones being warnings (weight = 1). The start with uppercase and end with full-stops ones. They are simply recommendations. The "LINKS_ENCODERS_CACHE" ones. It is a global and it's ok for it to be uppercase. I need to tidy that check. The rest of issues should be amended. And that's all. Hope it helps...ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy,

          I will update the branch with your recommendations and push it for integration

          Show
          Rajesh Taneja added a comment - Thanks Eloy, I will update the branch with your recommendations and push it for integration
          Hide
          Rajesh Taneja added a comment -

          Hello Eloy,
          I have done all the recommended changes, except B-3
          (IMPORTANT: Unless I'm missing something... the $BACKUP_IDS_EXIST cache grows and grows without limit.)

          I am not sure how this will react if multiple backups are running simultaneously. My concern is at https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-27120#L1R187

          Show
          Rajesh Taneja added a comment - Hello Eloy, I have done all the recommended changes, except B-3 (IMPORTANT: Unless I'm missing something... the $BACKUP_IDS_EXIST cache grows and grows without limit.) I am not sure how this will react if multiple backups are running simultaneously. My concern is at https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-27120#L1R187
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hiho, not sure if I get your comment about multiple backups... do you mean executed in parallel (different requests at the same time) or serial (one request performing one operation - backup or restore - for multiple courses)?

          The point B3 above is about $BACKUP_IDS_EXIST accumulating and accumulating entries along the life of the request, not being reset ever. And, as far as it's possible since 2.0 to programmatically perform the execution of multiple backup&restore operations in the same request, it can become really HUGE (out of memory). In fact, it can be become that huge with simply one course. Just imagine the millions of ids entries to handle one backup of the "Using Moodle" course (every post in forums will be using at least one => millions).

          So I just proposed to be able to reset that array each time one change of $backupid is detected (it means that a new operation has started).

          Although, really... perhaps that's not enough either, because 1-operation request can be really big ("Using Moodle" example above). Perhaps, after all, we should ban that cache completely, and only keep the $BACKUP_IDS_INFO one. Or alternatively, also limit the size of $BACKUP_IDS_EXIST (say $BACKUP_IDS_INFO max * 10 entries, for example). And fallback to slower DB methods when something is not found (record_exists)

          That will be slower than current "unlimited" $BACKUP_IDS_EXIST, but better than current DB-only implementation. And for sure will avoid the memory-bound problem.

          LOL, not sure if that is what you were asking, but we need to fix that one, for sure.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hiho, not sure if I get your comment about multiple backups... do you mean executed in parallel (different requests at the same time) or serial (one request performing one operation - backup or restore - for multiple courses)? The point B3 above is about $BACKUP_IDS_EXIST accumulating and accumulating entries along the life of the request, not being reset ever. And, as far as it's possible since 2.0 to programmatically perform the execution of multiple backup&restore operations in the same request, it can become really HUGE (out of memory). In fact, it can be become that huge with simply one course. Just imagine the millions of ids entries to handle one backup of the "Using Moodle" course (every post in forums will be using at least one => millions). So I just proposed to be able to reset that array each time one change of $backupid is detected (it means that a new operation has started). Although, really... perhaps that's not enough either, because 1-operation request can be really big ("Using Moodle" example above). Perhaps, after all, we should ban that cache completely, and only keep the $BACKUP_IDS_INFO one. Or alternatively, also limit the size of $BACKUP_IDS_EXIST (say $BACKUP_IDS_INFO max * 10 entries, for example). And fallback to slower DB methods when something is not found (record_exists) That will be slower than current "unlimited" $BACKUP_IDS_EXIST, but better than current DB-only implementation. And for sure will avoid the memory-bound problem. LOL, not sure if that is what you were asking, but we need to fix that one, for sure. Ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy,

          I have update the branch, cleaning backupidsexist. It will fall back on old way of record retrieval, in case record is not cached.

          Hope this is what you were expecting.

          Show
          Rajesh Taneja added a comment - Thanks Eloy, I have update the branch, cleaning backupidsexist. It will fall back on old way of record retrieval, in case record is not cached. Hope this is what you were expecting.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rajesh,

          I think it's near, near being perfect... I just have found some minor problems that need clarification/work, all them in the set_backup_ids_cached() implementation:

          1) $backupidscachesize is the size of $backupidsexist or $backupidscache ? I bet it's the later, but in line #247 you're incrementing independently of $backupidscache being created or no. So, at the end... the counter won't be showing the number of stored elements at any moment. Move them to be together and in sync.

          2) In line #241, you perform the insert_record_raw() in a "blind" way. You just know that $backupidsexist does not exist for that key, but now we are deleting the cache partially and the record may exist in the DB, aka BOOM!! So, for sure, that insert operation... needs to be "protected" by one "record_exists" or similar. Also, you keep the size of both $backupidsexist and $backupidscache, and I commented that it could be interesting to have the former bigger (say x10). That would lead to more isset() matching, saving us a good number of those, now needed, record_exists().

          3) Not 100% sure, but perhaps for the "if" sentence in line #252 it would be interesting to add one "else", so if the $backupidscache[$key] is not set, we re-introduce it to the cache too (in a similar way we do at line #243 - #247. Don't forget point 1) above is needed here too.

          And that's all. I think that all the rest look perfect. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rajesh, I think it's near, near being perfect... I just have found some minor problems that need clarification/work, all them in the set_backup_ids_cached() implementation: 1) $backupidscachesize is the size of $backupidsexist or $backupidscache ? I bet it's the later, but in line #247 you're incrementing independently of $backupidscache being created or no. So, at the end... the counter won't be showing the number of stored elements at any moment. Move them to be together and in sync. 2) In line #241, you perform the insert_record_raw() in a "blind" way. You just know that $backupidsexist does not exist for that key, but now we are deleting the cache partially and the record may exist in the DB, aka BOOM!! So, for sure, that insert operation... needs to be "protected" by one "record_exists" or similar. Also, you keep the size of both $backupidsexist and $backupidscache, and I commented that it could be interesting to have the former bigger (say x10). That would lead to more isset() matching, saving us a good number of those, now needed, record_exists(). 3) Not 100% sure, but perhaps for the "if" sentence in line #252 it would be interesting to add one "else", so if the $backupidscache [$key] is not set, we re-introduce it to the cache too (in a similar way we do at line #243 - #247. Don't forget point 1) above is needed here too. And that's all. I think that all the rest look perfect. Ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks for all the inputs Eloy

          Not sure about 2). If we get isset then while updating extrarecord, we have to fetch record from db. Also, do you want me to reduce slice for $backupidsexist or introduce a different counter for $backupidsexist ?

          Show
          Rajesh Taneja added a comment - Thanks for all the inputs Eloy Not sure about 2). If we get isset then while updating extrarecord, we have to fetch record from db. Also, do you want me to reduce slice for $backupidsexist or introduce a different counter for $backupidsexist ?
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy.
          I have updated the branch with your suggestions.
          Can you please review this again.

          Show
          Rajesh Taneja added a comment - Thanks Eloy. I have updated the branch with your suggestions. Can you please review this again.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, you "cracked" it (in a positive way).

          I think everything is ok from a logical POV but a little detail, you are using insert_record_raw() and it's nor recommended for operations involving LOBs (text, binary). As far as the table has one text column where we store serialized information... I'd suggest to change it to insert_record().

          Send it straight to integration once done, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, you "cracked" it (in a positive way). I think everything is ok from a logical POV but a little detail, you are using insert_record_raw() and it's nor recommended for operations involving LOBs (text, binary). As far as the table has one text column where we store serialized information... I'd suggest to change it to insert_record(). Send it straight to integration once done, thanks!
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy.

          Show
          Rajesh Taneja added a comment - Thanks Eloy.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Rajesh Taneja added a comment -

          Branch re-based.

          Show
          Rajesh Taneja added a comment - Branch re-based.
          Hide
          Aparup Banerjee added a comment - - edited

          just resolving a problem here atm:

          Stacktrace
          moodle1_converter_testcase::test_stash_storage unserialize(): Error at offset 0 of 1 bytes
          /Users/Shared/Jenkins/Home/git_repositories/master/backup/util/dbops/restore_dbops.class.php:1402
          /Users/Shared/Jenkins/Home/git_repositories/master/backup/converter/moodle1/lib.php:464
          /Users/Shared/Jenkins/Home/git_repositories/master/backup/converter/moodle1/tests/lib_test.php:137 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76

          Show
          Aparup Banerjee added a comment - - edited just resolving a problem here atm: Stacktrace moodle1_converter_testcase::test_stash_storage unserialize(): Error at offset 0 of 1 bytes /Users/Shared/Jenkins/Home/git_repositories/master/backup/util/dbops/restore_dbops.class.php:1402 /Users/Shared/Jenkins/Home/git_repositories/master/backup/converter/moodle1/lib.php:464 /Users/Shared/Jenkins/Home/git_repositories/master/backup/converter/moodle1/tests/lib_test.php:137 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,
          have a look at backup/converter/moodle1/tests/fixtures/lib_test.php test.

          test_stash_storage() is failing around after ;
          $this->assertEquals(12, $converter->get_stash('tempinfo1'));

          a few suspects :

          • php5 reference handling loves you.
          • array_slices are nasty (things looked fine here though)
          • serializing/unserializing and codings are playing havoc with the caching..

          This patch was reverted as this test needs to be working fine. we can't risk moodle1 converters breaking at all.

          Show
          Aparup Banerjee added a comment - Hi Raj, have a look at backup/converter/moodle1/tests/fixtures/lib_test.php test. test_stash_storage() is failing around after ; $this->assertEquals(12, $converter->get_stash('tempinfo1')); a few suspects : php5 reference handling loves you. array_slices are nasty (things looked fine here though) serializing/unserializing and codings are playing havoc with the caching.. This patch was reverted as this test needs to be working fine. we can't risk moodle1 converters breaking at all.
          Hide
          Rajesh Taneja added a comment -

          grrr... Should have run unit test.
          Yes, you are right Apu, php reference loves me

          Code is modified, back for your review.

          Show
          Rajesh Taneja added a comment - grrr... Should have run unit test. Yes, you are right Apu, php reference loves me Code is modified, back for your review.
          Hide
          Aparup Banerjee added a comment -

          yay that works! integrated into master now for testing.

          ps: Was SamH's instinct on the references btw.

          Show
          Aparup Banerjee added a comment - yay that works! integrated into master now for testing. ps: Was SamH's instinct on the references btw.
          Hide
          Aparup Banerjee added a comment -

          testing this (upcoming roll soon)

          Show
          Aparup Banerjee added a comment - testing this (upcoming roll soon)
          Hide
          Aparup Banerjee added a comment -

          tested and backup processes is fine.

          ps: at one point i did encounter this - 2 parallel backup to same filename (ie same minute and default filename) seemed to produce only one backup file in the list shown under file to download or restore. i'll report another bug for that.

          Show
          Aparup Banerjee added a comment - tested and backup processes is fine. ps: at one point i did encounter this - 2 parallel backup to same filename (ie same minute and default filename) seemed to produce only one backup file in the list shown under file to download or restore. i'll report another bug for that.
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          Your work has made into the latest Moodle release!

          You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

          Show
          Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: