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 live-list poller #2790

Merged
merged 1 commit into from
Apr 13, 2016
Merged

add live-list poller #2790

merged 1 commit into from
Apr 13, 2016

Conversation

erwinmombay
Copy link
Member

@dvoytenko poller review here

@erwinmombay
Copy link
Member Author

@dvoytenko PTAL

return;
}
if (this.lastBackoffTimeoutId_) {
this.win.clearTimeout(this.lastBackoffTimeoutId_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this.lastBackoffTimeoutId_ = null; after clearing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

done. also added tests

@erwinmombay erwinmombay force-pushed the poller branch 6 times, most recently from e260328 to ced61e0 Compare April 5, 2016 22:15
@erwinmombay
Copy link
Member Author

@dvoytenko PTAL, ready for review again.

* The poller is running but is doing exponential backoff (using
* the original interval value as base) every tick.
*/
BACKEDOFF: 'backedoff',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how useful this state is. From a point of view of the poller itself: it can be either running or not running, right? And it can be either in normal mode or recovery mode. But it depends on how you intend the PollerState enum to be used - if it's mostly for internal/testing purposes, it's certainly fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed.

@erwinmombay erwinmombay force-pushed the poller branch 3 times, most recently from 953a250 to 3c4c2ea Compare April 11, 2016 17:19
@erwinmombay erwinmombay force-pushed the poller branch 2 times, most recently from 5b75245 to 9dee506 Compare April 11, 2016 17:45
@erwinmombay
Copy link
Member Author

@dvoytenko PTAL. ready for review again

* Polling state of a Poller instance.
* @enum {string}
*/
export const PollerState = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a binary state now - so a simple boolean would do :)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done.

@dvoytenko
Copy link
Contributor

@erwinmombay Looks great. Just a few requests, but pretty close to LGTM.

@erwinmombay
Copy link
Member Author

@dvoytenko addressed last rounds of comments. PTAL

@dvoytenko
Copy link
Contributor

LGTM

@erwinmombay erwinmombay merged commit 28ee52e into ampproject:master Apr 13, 2016
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.

4 participants