Closed timotheeg closed 5 months ago
This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers
Note: Typically another way to deal with pages and fetching data is by using the github sdk octokit.js, which has pagination handling built in. I did not introduce it in this PR because the goal was just to remove the env var ISOMERPAGES_REPO_PAGE_COUNT
, and I kept the parallel fetch for pages 2-N, which octokit wouldn't do.
That should probably be revisited at some point.
All comments addressed @seaerchin , Let me know if good to go. Thanks! 🙏
Result in prod:
Problem
The way isomercms retrieves and cache the list of repos is with the help of an environment variable
ISOMERPAGES_REPO_PAGE_COUNT
. That variable contains a number, which is used to create parallel requests to github.In practice that works, but operationally this is not great:
Github APIs have a very robust pagination system, so we can derive the number of pages from that to make the system dynamic, and remove the (small) operational risk of forgetting to update the env var.
Additionally, the cache only has one exposed API, which is to retrieve the lastUpdated time by repoName. The list of repo is not stored in cache by name, but it is a list and so the lookup does a list search every time.
Solution
getAllRepoData()
calls into 2 distinct use cases: a. exported API optimized for time: we fetch the first page, assess how many pages there in total, and fetch all remaining pages in parallel b. optimized for load distribution: we fetch the pages sequentially by following github pagination system's next linkO(1)
lookupNotes:
getAllRepoData()
does all its calls in parallel again.Breaking Changes
Reference:
Results: Sample trace for the new refresh process that shows fetches the pages sequentially works:
Post Deploy Work (after stability is observed)
STAGING_ISOMERPAGES_REPO_PAGE_COUNT
. This should ONLY be done when we are sure we will not need to roll back possibly wait one full release.