Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review last updated information that is cached #68

Closed
julen opened this issue Nov 1, 2016 · 7 comments
Closed

Review last updated information that is cached #68

julen opened this issue Nov 1, 2016 · 7 comments
Assignees

Comments

@julen
Copy link
Contributor

julen commented Nov 1, 2016

While checking #62, I realized that the cached information for the Last Updated column contains more information than we actually use.

With the new browsing table, we are only interested in the timestamp for the creation_time. The lastupdated key in the cache contains the following information though:

{
  "lastupdated": {
    "creation_time": <int:timestamp>,
    "display_datetime": <string:datetime>,
    "iso_datetime": <string:datetime>,
    "source": <string:unit-source>,
    "unit_url": <string:unit-permalink>,
  }
}

Unless there's a use-case I am missing, we should reduce this to a single value and hence make better use of the server memory. This would also simplify the initial queries for refreshing the data.

@julen
Copy link
Contributor Author

julen commented Nov 1, 2016

I now see this is used in the expanded stats view, but still the data is returned for all child items in the browsing table. We shouldn't do this, and maybe it's worth to even simplify the data we display in the expanded view.

@iafan
Copy link
Contributor

iafan commented Nov 1, 2016

When it comes to stats, I think I changed the code not to export unnecessary fields. Did you recalculate the stats?

@julen
Copy link
Contributor Author

julen commented Nov 1, 2016

Yes, definitely — please consider this issue in the context of #62 and the get_stats view, so definitely its exposure is way lower than regular browsing views.

Still, the main concerning point here is that there is too much information being cached. In browsing tables only the creation_time timestamp is used. The rest of the fields are pretty much unused until the detailed view is expanded, hence my doubt about the real usefulness of keeping that much data in the cache.
If we still want to keep that information visible, we can likewise make things more compact by keeping only the mtime, the trimmed source and the unit id.

Since with the recent changes we are already required to run a server-wide refresh_stats, I'm thinking that it makes sense to make any stats cache-related changes now and reduce to the minimum the amount of times we need to manually refresh stats.

@iafan
Copy link
Contributor

iafan commented Nov 1, 2016

In the detailed stats view, we can definitely use only timestamps as well. So feel free to remove all datetime fields. Where specifically are they currently used?

@julen
Copy link
Contributor Author

julen commented Nov 3, 2016

The rendering of last actions is already using timestamps in the detailed view, so the rest of datetimes are redundant.

Regarding the other fields, source (trimmed) and unit_url are used only, specifically in the expanded stats view. I was suggesting that in case we want to keep these, we could compact them even more to store source and uid only (then we can permalink to //server.tld/unit/<uid>/.

So the cached information would be reduced to:

{
  "lastupdated": {
    "mtime": <int:timestamp>,
    "source": <string:unit-source>,
    "uid": <int:unit-id>,
  }
}

or in case we decide not to display last updated information besides the date:

{
  "lastupdated": <int:timestamp>
}

@iafan
Copy link
Contributor

iafan commented Nov 3, 2016

As I understand, we're referring to the 'Last updated' column used in the browser table, and to the 'Updates' section in the expanded stats. The first one indeed requires only the timestamp and is heavily used, while the other seems to be less useful.

So if our only reason to cache extra data is to display this 'Updates' section, we can ditch it and store only the timestamp.

@julen
Copy link
Contributor Author

julen commented Nov 4, 2016

Your understanding is correct, and totally agree with what you say (it's in line with what I was proposing above). I'll update #75 so that we cache timestamps only.

@julen julen self-assigned this Nov 8, 2016
@julen julen closed this as completed in #75 Nov 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants