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.

Sign up for GitHub

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

Jump to bottom

feat(#169): cache expiry for remote places #183

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat(#169): cache expiry for remote places #183

wants to merge 3 commits into from

Conversation

inromualdo
Copy link
Contributor

@inromualdo inromualdo commented Jun 20, 2024 •
edited
Loading

closes #169

@kennsippell
Copy link
Member

I'm so pleased you're working on this. There is quite a bit of confusion from people in Kenya when the cache is stale. Much appreciated @inromualdo

@inromualdo
Copy link
Contributor Author

inromualdo commented Jul 15, 2024 •
edited
Loading

Hi @kennsippell

Here’s the approach I’m taking to resolve the issue:

  • I’ve integrated the node-cache package to handle caching.

  • The primary change is in the cht-api file, where I initialize the cache with a one-hour expiration time.

  • When retrieving data for a place by type, if the data is not in the cache, an Axios request is made; otherwise, the cached content is returned.

  • Whenever a place of a given type is updated or created, the cache for that type is cleared to ensure that the next read request fetches fresh data from the API.

  • Consequently, the remote-place-cache file is no longer necessary, which has resulted in changes across various files and unit tests.

Currently, some tests are failing. I am working on resolving these issues and performing a manual test. In the meantime, please review my progress and provide any feedback or suggestions you may have on the process.

@inromualdo inromualdo requested a review from kennsippell July 15, 2024 13:36
kennsippell
kennsippell requested changes Oct 10, 2024
View reviewed changes
Copy link
Member

@kennsippell kennsippell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice ideas here. some feedback

src/liquid/app/nav.html
@@ -47,9 +47,6 @@

<div class="navbar-dropdown">
{% if op == 'table' %}
<a class="navbar-item" hx-post="/app/refresh-all" hx-swap="none">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not support manual refresh? Do you need to wait an hour?

src/lib/cht-api.ts
@@ -182,20 +189,28 @@ export class ChtApi {
include_docs: true,
};
console.log('axios.get', url, params);
const resp = await this.axiosInstance.get(url, { params });
let places: any[] | undefined = ChtApi.cache.get(placeType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously cache was keyed by [domain][contact-type] but now only by [contact-type]. Isn't the domain dimension required? This allowed independant caches and timers for each county eCHIS instance... just because the cache has expired for Nairobi county instance, should it also expire for Migori county instance?

src/lib/cht-api.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennsippell kennsippell kennsippell requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cache expiry for remote places
2 participants
@inromualdo @kennsippell