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

Add rate limiter to imports #1101

Merged
merged 2 commits into from
Dec 18, 2018
Merged

Add rate limiter to imports #1101

merged 2 commits into from
Dec 18, 2018

Conversation

mirka
Copy link
Member

@mirka mirka commented Dec 17, 2018

This adds rate limiting to the CoreImporter so that we don't have to rely on the hacky workaround in nudgeUnsynced to get large imports unstuck:

With my new familiarity with the LocalQueue 😌, I was able to realize that the stuck import notes were not stuck locally, nor were they being "ignored" by the server. They were sent out correctly, but then each of them were being returned 503 errors from the Simperium server.

@roundhill:

  • Should node-simperium be patched to handle these errors?
  • Should there be an official rate limit for the Simperium API? (In this app, I set the limit to 250 requests/sec. It is a little conservative, but I think it's fine because according to Tracks, imports larger than 100 notes are very rare.)

To test

  1. Set localStorage.debug="sync:*,analytics" and reload.
  2. Import a file with >500 notes.

The syncing should complete, and nudgeUnsynced shouldn't log anything other than 0 unsynced notes. (Or, you can try commenting out the client.client.connect() line quoted above.)

Import file (1000 notes) for your convenience: few-tags-1000-800kb.json.zip

Next steps

Now that this is out of the way, I will modify the nudgeUnsynced function so that it specifically addresses the v === 0 notes that resulted from unpersisted offline changes (#1098). We'll get rid of the client.connect() call there, thus fixing the init flood once and for all 🔨

@mirka mirka added this to the 1.3.4 milestone Dec 17, 2018
@mirka mirka requested a review from roundhill December 17, 2018 16:33
@roundhill
Copy link
Contributor

As discussed in Slack, let's set MAX_REQUESTS_PER_SEC to 50.

@roundhill
Copy link
Contributor

Filed Simperium/node-simperium#70 for our friend node-simperium.

@mirka mirka merged commit 9c08758 into master Dec 18, 2018
@mirka mirka deleted the fix/simperium-rate-limiting branch December 18, 2018 10:06
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

Successfully merging this pull request may close these issues.

2 participants