Moodle
  1. Moodle
  2. MDL-33416

Get rid of referencelastsync and referencelifetime from the {files} table

    Details

    • Testing Instructions:
      Hide

      You must have existing shortcuts to internal and external repositories before upgrade.

      Before test hack the code in repository/lib.php and change value returned by get_reference_file_lifetime() to something small (i.e. 30 seconds). Note that filesystem repo overwrites it.

      Test for regressions/errors in the following operations:

      1. If you had shortcuts before the upgrade, make sure they still work.
      2. Add a file from repository as a copy and as a shortcut (both image and non-image files, in both filemanager and text editor)
      3. Create a file resource with shortcut to some external repository, make sure to check the option "display size"
      4. Replace the files in the repositories
      5. Make sure the image thumbnail and file size for non-image files is updated for the shortcuts (you'll have to wait for 30s or whatever you have hacked to above)
      6. Create several shortcuts to the same file in internal repository (private files, server files)
      7. Overwrite the source file of those shortcuts, make sure the shortcuts are updated immediately
      8. Delete the source of those shortcuts, make sure they are converted to the normal files.

      Please don't report UI bugs already reported in MDL-41491 and MDL-41591

      Show
      You must have existing shortcuts to internal and external repositories before upgrade. Before test hack the code in repository/lib.php and change value returned by get_reference_file_lifetime() to something small (i.e. 30 seconds). Note that filesystem repo overwrites it. Test for regressions/errors in the following operations: If you had shortcuts before the upgrade, make sure they still work. Add a file from repository as a copy and as a shortcut (both image and non-image files, in both filemanager and text editor) Create a file resource with shortcut to some external repository, make sure to check the option "display size" Replace the files in the repositories Make sure the image thumbnail and file size for non-image files is updated for the shortcuts (you'll have to wait for 30s or whatever you have hacked to above) Create several shortcuts to the same file in internal repository (private files, server files) Overwrite the source file of those shortcuts, make sure the shortcuts are updated immediately Delete the source of those shortcuts, make sure they are converted to the normal files. Please don't report UI bugs already reported in MDL-41491 and MDL-41591
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      wip-MDL-33416-master
    • Rank:
      41292

      Description

      In DB, we now have files::referencelastsync and files::referencelifetime as well as files_reference::lastsync and files_reference::lifetime. Surely they should be in one table only. I believe that those is files_reference are valid as the lifetime and lastsync is common for all aliases.

      Also add field files_reference.details and API for repositories to store custom information there (i.e. result of last sync)

        Issue Links

          Activity

          Hide
          David Mudrak added a comment -

          While working on another issue with file references, I spotted this. Can some of you Wise Men confirm this is just another regression of the DB changes related to file references?

          Show
          David Mudrak added a comment - While working on another issue with file references, I spotted this. Can some of you Wise Men confirm this is just another regression of the DB changes related to file references?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi David, 100% taking from memory here...

          if sounds to me that, in the original proposal from Petr, the update/sync was sort of 2-steps process, so first each reference was synced from source and later the files using that reference (on demand). So, to know if one file is updated with the reference, some comparison of lastsync was done.

          I've tried to look to original code from Petr but have been not able to find it anywhere... so take that comment with caution because its 100% in the "hypothetical" area. Just sounds in my head.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi David, 100% taking from memory here... if sounds to me that, in the original proposal from Petr, the update/sync was sort of 2-steps process, so first each reference was synced from source and later the files using that reference (on demand). So, to know if one file is updated with the reference, some comparison of lastsync was done. I've tried to look to original code from Petr but have been not able to find it anywhere... so take that comment with caution because its 100% in the "hypothetical" area. Just sounds in my head. Ciao
          Hide
          Michael de Raadt added a comment -

          So, is this an issue or not?

          Show
          Michael de Raadt added a comment - So, is this an issue or not?
          Hide
          David Mudrak added a comment -

          Michael, I believe that all suspicious DB structures must be clarified before the release. Awaiting for confirmation from Petr.

          Show
          David Mudrak added a comment - Michael, I believe that all suspicious DB structures must be clarified before the release. Awaiting for confirmation from Petr.
          Hide
          Petr Škoda added a comment -

          My original proposal used these duplicates for performance and backwards compatibility reasons, but I do not know what is in master now, sorry.

          Show
          Petr Škoda added a comment - My original proposal used these duplicates for performance and backwards compatibility reasons, but I do not know what is in master now, sorry.
          Hide
          David Mudrak added a comment -

          We had an intensive talk about this with Petr today. At the end we agreed that the best (though not optimal) approach for now would be to drop the fields from

          {files}

          and keeps then in

          {files_reference}

          only. This implies that we need to make sure that all file API operations have data from both tables available whenever they accept $file_record (this is what original Petr's proposal did not require).

          Show
          David Mudrak added a comment - We had an intensive talk about this with Petr today. At the end we agreed that the best (though not optimal) approach for now would be to drop the fields from {files} and keeps then in {files_reference} only. This implies that we need to make sure that all file API operations have data from both tables available whenever they accept $file_record (this is what original Petr's proposal did not require).
          Hide
          Dan Poltawski added a comment -

          From what I saw, what is there mostly matches that expectation at the moment.

          Show
          Dan Poltawski added a comment - From what I saw, what is there mostly matches that expectation at the moment.
          Hide
          David Mudrak added a comment -

          Yes, that was my reasoning, too.

          Show
          David Mudrak added a comment - Yes, that was my reasoning, too.
          Hide
          Marina Glancy added a comment -

          David, please also look in repository::sync_external_file(), maybe we avoid extra querying the table files_reference there. Note, I'm altering this function in MDL-33550

          Show
          Marina Glancy added a comment - David, please also look in repository::sync_external_file(), maybe we avoid extra querying the table files_reference there. Note, I'm altering this function in MDL-33550
          Hide
          Michael de Raadt added a comment -

          Bumping this as it's critical time-wise.

          Show
          Michael de Raadt added a comment - Bumping this as it's critical time-wise.
          Hide
          Martin Dougiamas added a comment -

          David's been on carer's leave recently ...

          Show
          Martin Dougiamas added a comment - David's been on carer's leave recently ...
          Hide
          Martin Dougiamas added a comment -

          it's too late to do anything about this for 2.3.x now, I think. Is that OK? Did we get some performance benefit out of it?

          Show
          Martin Dougiamas added a comment - it's too late to do anything about this for 2.3.x now, I think. Is that OK? Did we get some performance benefit out of it?
          Hide
          David Mudrak added a comment -

          Yeah sorry I did not manage to finish this in time. Let me suggest that we postpone this to 2.4 which will give us time to

          • realize if we need these two fields or not
          • get rid of using them in the code
          Show
          David Mudrak added a comment - Yeah sorry I did not manage to finish this in time. Let me suggest that we postpone this to 2.4 which will give us time to realize if we need these two fields or not get rid of using them in the code
          Hide
          David Mudrak added a comment -

          As agreed on Friday, retriaging to DEV backlog.

          Show
          David Mudrak added a comment - As agreed on Friday, retriaging to DEV backlog.
          Hide
          Marina Glancy added a comment -

          David, I linked to the issue MDL-34290
          I can't avoid using field files.referencelastsync now

          both lifetime fields still seem strange to me

          Show
          Marina Glancy added a comment - David, I linked to the issue MDL-34290 I can't avoid using field files.referencelastsync now both lifetime fields still seem strange to me
          Hide
          David Mudrak added a comment -

          Re-assigning to Marina as she knows the whole syncing machinery best now.

          Marina, if you realize this is not valid issue any more, feel free to close. We had a chat about it and you know what my original idea was. Cross fingers

          Show
          David Mudrak added a comment - Re-assigning to Marina as she knows the whole syncing machinery best now. Marina, if you realize this is not valid issue any more, feel free to close. We had a chat about it and you know what my original idea was. Cross fingers
          Hide
          Marina Glancy added a comment -

          David, I agree completely that this is unimportant information. And already in MDL-34290 I tried to stop using those fields in

          {files}

          table. Also I would like to add a 'details' field into

          {files_reference}

          table. The corresponding repository will decide how to use it.

          For example I would store there the result of the last synchronisation. Some APIs return something like revision number. So we can store last revision number (or time last modified on external server). If we request meta information about the file from external repo and we see that it did not change we just won't download the file again.

          Show
          Marina Glancy added a comment - David, I agree completely that this is unimportant information. And already in MDL-34290 I tried to stop using those fields in {files} table. Also I would like to add a 'details' field into {files_reference} table. The corresponding repository will decide how to use it. For example I would store there the result of the last synchronisation. Some APIs return something like revision number. So we can store last revision number (or time last modified on external server). If we request meta information about the file from external repo and we see that it did not change we just won't download the file again.
          Hide
          Marina Glancy added a comment -

          Requesting peer review

          Show
          Marina Glancy added a comment - Requesting peer review
          Hide
          David Mudrak added a comment -

          Marina Glancy, the gihub diff URL does not display any data.

          Show
          David Mudrak added a comment - Marina Glancy , the gihub diff URL does not display any data.
          Hide
          Marina Glancy added a comment -

          sorry David, just updated

          Show
          Marina Glancy added a comment - sorry David, just updated
          Hide
          Marina Glancy added a comment -

          reading the comments above I now remembered about the files_reference.details field that some repositories would want. Frédéric Massart, David Mudrak do you think we should add it now?

          Show
          Marina Glancy added a comment - reading the comments above I now remembered about the files_reference.details field that some repositories would want. Frédéric Massart , David Mudrak do you think we should add it now?
          Hide
          Marina Glancy added a comment -

          Fred do you mind reviewing it?

          Show
          Marina Glancy added a comment - Fred do you mind reviewing it?
          Hide
          Frédéric Massart added a comment -

          Hi Marina,

          I can't find anything wrong with your patch, the remaining usage of those 2 fields seem to always be taken from the reference table which is good. Unit Tests passed and code looks good. But just to comment on something, I'd say that your commit message could mention the component .

          Please feel free to push for integration.

          Thanks
          Fred

          PS: Maybe make pre-existence of references mandatory in the testing instructions?

          Show
          Frédéric Massart added a comment - Hi Marina, I can't find anything wrong with your patch, the remaining usage of those 2 fields seem to always be taken from the reference table which is good. Unit Tests passed and code looks good. But just to comment on something, I'd say that your commit message could mention the component . Please feel free to push for integration. Thanks Fred PS: Maybe make pre-existence of references mandatory in the testing instructions?
          Hide
          Marina Glancy added a comment -

          Thanks Fred, I will change the commit text when I rebase after release

          Show
          Marina Glancy added a comment - Thanks Fred, I will change the commit text when I rebase after release
          Hide
          Sam Hemelryk added a comment -

          Thanks Marina - this has been integrated now

          Just noting an additional commit was added to fix whitespace.

          Show
          Sam Hemelryk added a comment - Thanks Marina - this has been integrated now Just noting an additional commit was added to fix whitespace.
          Hide
          Dan Poltawski added a comment -

          Not sure if this is related to this week, but when looking at the file info of a referenced file from box.net i'm getting an error:

          Show
          Dan Poltawski added a comment - Not sure if this is related to this week, but when looking at the file info of a referenced file from box.net i'm getting an error:
          Hide
          Dan Poltawski added a comment -

          Same problem exists on master (and everything else is ok, so passing).

          Show
          Dan Poltawski added a comment - Same problem exists on master (and everything else is ok, so passing).
          Hide
          Dan Poltawski added a comment -

          Created MDL-42092 for the issue I experienced while testing.

          Show
          Dan Poltawski added a comment - Created MDL-42092 for the issue I experienced while testing.
          Hide
          Dan Poltawski added a comment -

          You did it!

          Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

          Show
          Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: