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

Fixes #68 - Handle Backoff header. #82

Merged
merged 11 commits into from
Jul 21, 2015
Merged

Fixes #68 - Handle Backoff header. #82

merged 11 commits into from
Jul 21, 2015

Conversation

n1k0
Copy link
Contributor

@n1k0 n1k0 commented Jul 17, 2015

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 18, 2015

I just realized we need a request queue in order to avoid getting a disordered requests chain when the backoff release timestamp is reset. That's slightly more complicated than initially expected; I wish we'll actually use that feature :)

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 19, 2015

This is really a bunch of work; especially, we'd need to think what are the consequences of not having the response immediately while requests are queued, and what happens precisely when a bunch of them are performed when the backoff release timestamp is reached.

A very simple solution would be to simply trigger and error when a Backoff header is received, and that would be developers' responsibility to hold on performing more requests and/or handling the queuing by themselves.

I'm voting for the latter, though would love getting some feedback :)

@Natim
Copy link
Member

Natim commented Jul 20, 2015

Actually the Backoff is not an error. IMHO it is better not to do anything (like queuing or stopping requests) rather than raising error.

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 20, 2015

After discussion, here's the new plan:

  • On Backoff release timestamp being set, reject any call to Collection#sync();
  • Provide an option to Collection#sync() to force the synchronization even if a backoff is ongoing.

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 20, 2015

@Natim r=?

@@ -589,12 +589,18 @@ export default class Collection {
* Conflicting server records will be overriden with local changes.
* * `Collection.strategy.MANUAL`:
* Conflicts will be reported in a dedicated array.
* - {Boolean} forceBackoff: Force synchronization even if server is currently
Copy link
Member

Choose a reason for hiding this comment

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

Maybe ignoreBackoff is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's better indeed, will update.

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 20, 2015

@Natim r=? regarding your comments.

I'll update this patch to allow running integration tests against a kinto server instance configured to send Backoff headers, for safety. Hold on merging in the meanwhile.

@n1k0
Copy link
Contributor Author

n1k0 commented Jul 20, 2015

Note that this patch embeds a fix for Kinto/kinto#148; We could land this as is and remove the fix when the related issue is closed, or wait for a new kinto version to be released (I really want to rely on stable releases for tests).

@Natim
Copy link
Member

Natim commented Jul 21, 2015

r+ I agree we should not wait on Kinto/kinto#148 to land this PR

n1k0 added a commit that referenced this pull request Jul 21, 2015
@n1k0 n1k0 merged commit b8139a7 into master Jul 21, 2015
@n1k0 n1k0 deleted the backoff-headers branch July 21, 2015 08:09
@n1k0 n1k0 added this to the 1.0.0 milestone Jul 21, 2015
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.

None yet

2 participants