Moodle
  1. Moodle
  2. MDL-27147

there is a potential preformance trouble in upgrade_set_timeout() in that its calling get_config() too many times.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Cannot Reproduce
    • Affects Version/s: 2.0.2, 2.1
    • Fix Version/s: None
    • Component/s: Administration
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Rank:
      16798

      Description

      in MDL-26580 Petr has described concern for performance problems with upgrade_set_timeout() as well as adding a no-timeout option for cli mode upgrade:

      (16:38:10) skodak@moodle.org: arrggh, I might have found a bug in my upgrade_set_timeout()
      (16:38:23) skodak@moodle.org: could you please review it for me?
      (16:38:27) apu: really?? sure
      (16:39:11) skodak@moodle.org: the question is - does it execute get_config() too often?
      (16:39:59) apu: !isset($CFG->upgraderunning) or $CFG->upgraderunning < time()
      (16:40:00) apu: hm
      (16:40:06) skodak@moodle.org: yes
      (16:40:53) skodak@moodle.org: if (!isset($CFG->upgraderunning) or $CFG->upgraderunning + XXX < time()) {
      (16:41:44) skodak@moodle.org: I suppose 10 could work there
      (16:42:11) apu: tricky, how much to use is how much u save
      (16:42:47) skodak@moodle.org: it would be critical if you use it in large loops like those you added
      (16:44:02) skodak@moodle.org: well, not really critical
      (16:44:22) apu: yes, it would help, what is the risk.. that between 0- 10 we skip get_config and assume running
      (16:44:44) skodak@moodle.org: no big deal, if anybody interrupts upgrade it may create problems anyway
      (16:45:16) apu: ahh.. yea, interrupt as in cli ctrl-c ?
      (16:45:37) skodak@moodle.org: hmmmmmmmmmmmmmm
      (16:45:53) skodak@moodle.org: I think can remove the timeouts completely in CLI mode
      (16:46:30) apu: yes, cli mode should be much more interactive (or it can be)
      (16:47:02) skodak@moodle.org: the problem is when you stop web loading the upgrade has to continue until the next step to get consistent data, on the other hand ppl using CLI do not ctrl-C when it runs
      (16:50:53) skodak@moodle.org: did you plan to fix some other upgrade issues next week?
      (16:51:35) apu: i don't have any, not even planned the next 'sprint' ... i can work on it still if there are any
      (16:52:33) skodak@moodle.org: the problem is that the MDL-26580 should be resolved by the PULL, but I think it might not be
      (16:53:11) apu: going with reports so far, it is, but there is no other trace given (from Luis)
      (16:53:26) skodak@moodle.org: did he test it?
      (16:53:32) apu: no
      (16:53:48) apu: no one did yet
      (16:53:59) skodak@moodle.org: that is not good
      (16:54:25) apu: yea.. i know.. i'll ask in tracker
      (16:54:57) apu: grr, MDL-17344 for MOODLE_20_STABLE keeps conflicting..
      (16:55:11) apu: espcially version.php
      (16:55:24) skodak@moodle.org: yeah, tricky
      (16:57:40) skodak@moodle.org: I am a bit scared to commit your patch without fixing the upgrade_set_timeout(), but at the same time I am not 100% sure the 10s is good there
      (16:58:15) skodak@moodle.org: also it would be great to get some testing from the reporter
      (16:58:43) apu: yea it would shall we get him to test the pull or test in tracker?
      (16:58:56) skodak@moodle.org: maybe we could add the 10s there and let him use your branch for testing

        Activity

        Aparup Banerjee created issue -
        Petr Škoda made changes -
        Field Original Value New Value
        Assignee moodle.com [ moodle.com ] Petr Škoda (skodak) [ skodak ]
        Petr Škoda made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Petr Škoda made changes -
        Status In Progress [ 3 ] Closed [ 6 ]
        Resolution Cannot Reproduce [ 5 ]
        Martin Dougiamas made changes -
        Workflow MDL Workflow [ 69227 ] MDL Full Workflow [ 96380 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved: