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

Fetch live resolver urls in background #529

Merged
merged 1 commit into from
Dec 16, 2018

Conversation

stefanbuck
Copy link
Member

@stefanbuck stefanbuck commented Dec 16, 2018

This is one (of a few) changes which are needed to implement #277.

On app startup it prefetch all live resolver urls at once in the background by calling a bulk endpoint. For details check out OctoLinker/api#59. If a user clicks on a link, the click handler will check an internal cache first. If the requested resource isn't available it will fallback to the old behaviour.

With this Pull Request I want to verify that our live resolver server is able to handle such traffic increase. The timing for this is perfect. It's one week before Christmas which I assume is already a less busy week than any other week. Also in case something goes really bad, it won't affect that many people.

@@ -11,11 +11,18 @@ const LINK_SELECTOR = '.octolinker-link';
const $body = $('body');
let matches;

function openUrl(url, newWindow = false, newWindowActive = true) {
function openUrl(event, url) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Disclaimer: We will get rid of this file soon anyway, so need to aim for a perfect solution 😉

@stefanbuck stefanbuck merged commit a0555f7 into OctoLinker:master Dec 16, 2018
@stefanbuck stefanbuck deleted the background-fetch branch December 16, 2018 22:15
@stefanbuck
Copy link
Member Author

Gangsta merge 😉

@stefanbuck
Copy link
Member Author

So, as expected, the server quickly reached a critical memory usage. This is caused by our current caching strategy. We use mem which worked well in the past, but maybe it isn't suitable with our new requirement. I'll investigate.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant