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

          Issue Links

            Activity

            Hide
            mudrd8mz 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
            mudrd8mz 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
            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 Mudrak 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 Mudrak 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 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
            mudrd8mz 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
            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 Mudrak added a comment -

            Yes, that was my reasoning, too.

            Show
            mudrd8mz David Mudrak 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 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
            mudrd8mz 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
            mudrd8mz David Mudrak added a comment -

            As agreed on Friday, retriaging to DEV backlog.

            Show
            mudrd8mz David Mudrak 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 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
            mudrd8mz 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 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 Mudrak added a comment -

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

            Show
            mudrd8mz David Mudrak 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 Mudrak 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 Mudrak 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