search

cellule-tice / moodle-format_collapsibletopics

Clone of core topics format that adds collapsing behaviour to sections.
7 stars 6 forks source link

Reduce database load #15

Closed yorickreum closed 5 years ago

yorickreum commented 5 years ago

Hi there,

first of all: Thanks for this amazing plugin, this is going to make our Moodle-website better!

As our installation suffers peaks with up to 10.000 online users, we're a little bit concerned about the database reads/writes triggered by the plugin (since it's writing all the "section is open"-states – for all users, courses, sections – to the database at the moment).

The setting _formatcollapsibletopics | keepstateoversession still keeps all the data in the database, but deletes it when the user logs out.

Is there already any effort to reduce database writes?

In our opinion, it would be preferable to store this information not at the database but at the client-site (for example JavaScript local storage or session storage), at least if the "keep state over sessions"-setting is disabled. We don't see a use case where it's really necessary to share this information with different devices.

Kind regards from Germany + Let us know if we can help somehow! Yorick

cellule-tice commented 5 years ago

Hi Yorick, We've been using this course format for 2 years as the only course format (except for collapsibleweeks) and haven't noticed performance issues. However, we are running moodle with only 10k users and 1.5 k courses. Maybe problems arise with bigger instances, but none I am aware of. However after your post I've started investigating the possibility to use browser localStorage and sessionStorage to keep "keepstateoversession" feature, but that would no longer be "moodle session"-based but browser-session based, which I think is no big deal. I'm going to adapt the code in that way and push it on a new branch soon. Will you test it? Regards Jean-Roch from Belgium

cellule-tice commented 5 years ago

Hi, I've pushed a new branch "browserpersistance" that uses javascript storage (session or local according to "keepstateoversession" setting. Could you test it and give me feedback? The work is not finalized, I still have to clean some code related to db implementation, adapt privacy handling and upgrade process to remove db entries. Regards

yorickreum commented 5 years ago

That was fast. Thanks a lot, we'll test it tomorrow!

yorickreum commented 5 years ago

Sorry, we couldn't test it as fast as wrote you.

We've installed it in our testing environment now. It works like a charm! The code also already looks really good.

Two things we recognised:

  1. The setting now works vice versa than expected: If it's checked, meaning "state will be kept when user logs out", it's using session storage – if not checked, local storage. Shouldn't it be the other way round?

  2. By saving the JSON-stringified data, we (in our opinion) get some overhead in the storage. It wouldn't be exactly necessary to store "true" for all of the expanded sections, storing the number should be enough? But since the code looks quite clean this way, it's maybe better to leave it like this. (I can't imagine it will cause any real harm.)

Thanks a lot! Yorick

cellule-tice commented 5 years ago

Hi,

  1. I cannot reproduce... (tested on Chrome and Firefox) Here when checked, it's using local storage, and session storage when unchecked. kAnd someone else (Chris Kenniburg - Fordson theme) confirmed. I checked the code and everything seems ok. Please check again ;-)
  2. As for the overhead in browser storage, it could be optimized, but it's no big deal (IMO) and better than moodle database overload. Maybe I'll do that in a future release, but for the moment I have other things on the line.

Keep me posted on your tests please Regards Jean-Roch

cellule-tice commented 5 years ago

Integrated in last release, closing issue !