Uploaded image for project: '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

      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)

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              mudrd8mz David Mudrák 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
              mudrd8mz David Mudrák 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
              stronk7 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
              stronk7 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
              salvetore Michael de Raadt added a comment -

              So, is this an issue or not?

              Show
              salvetore Michael de Raadt added a comment - So, is this an issue or not?
              Hide
              mudrd8mz David Mudrák added a comment -

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

              Show
              mudrd8mz David Mudrák added a comment - Michael, I believe that all suspicious DB structures must be clarified before the release. Awaiting for confirmation from Petr.
              Hide
              skodak Petr Skoda 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
              skodak Petr Skoda 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
              mudrd8mz David Mudrák 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
              mudrd8mz David Mudrák 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
              poltawski Dan Poltawski added a comment -

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

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

              Yes, that was my reasoning, too.

              Show
              mudrd8mz David Mudrák added a comment - Yes, that was my reasoning, too.
              Hide
              marina 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 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
              salvetore Michael de Raadt added a comment -

              Bumping this as it's critical time-wise.

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

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

              Show
              dougiamas Martin Dougiamas added a comment - David's been on carer's leave recently ...
              Hide
              dougiamas 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
              dougiamas 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
              mudrd8mz David Mudrák 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
              mudrd8mz David Mudrák 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
              mudrd8mz David Mudrák added a comment -

              As agreed on Friday, retriaging to DEV backlog.

              Show
              mudrd8mz David Mudrák added a comment - As agreed on Friday, retriaging to DEV backlog.
              Hide
              marina 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 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
              mudrd8mz David Mudrák 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
              mudrd8mz David Mudrák 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 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 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 Marina Glancy added a comment -

              Requesting peer review

              Show
              marina Marina Glancy added a comment - Requesting peer review
              Hide
              mudrd8mz David Mudrák added a comment -

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

              Show
              mudrd8mz David Mudrák added a comment - Marina Glancy , the gihub diff URL does not display any data.
              Hide
              marina Marina Glancy added a comment -

              sorry David, just updated

              Show
              marina Marina Glancy added a comment - sorry David, just updated
              Hide
              marina 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 Mudrák do you think we should add it now?

              Show
              marina 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 Mudrák do you think we should add it now?
              Hide
              marina Marina Glancy added a comment -

              Fred do you mind reviewing it?

              Show
              marina Marina Glancy added a comment - Fred do you mind reviewing it?
              Hide
              fred 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
              fred 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 Marina Glancy added a comment -

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

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

              Thanks Marina - this has been integrated now

              Just noting an additional commit was added to fix whitespace.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Marina - this has been integrated now Just noting an additional commit was added to fix whitespace.
              Hide
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

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

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

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

              Show
              poltawski Dan Poltawski added a comment - Created MDL-42092 for the issue I experienced while testing.
              Hide
              poltawski 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
              poltawski 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:
                    Fix Release Date:
                    18/Nov/13