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 polling to queries #46

Closed
wants to merge 1 commit into from
Closed

Add polling to queries #46

wants to merge 1 commit into from

Conversation

slifty
Copy link
Member

@slifty slifty commented Mar 14, 2019

Subscriptions are the goal, since they are likely much more efficient
and scale, but for the purposes of a demo we think that just using built
in polling functionality of ApolloClient will do the trick.

This is related to #43 and #45 in that it accomplishes the core need of simulating real time for demonstration purposes. Ultimately I'm concerned that polling won't scale well, and that we will need to improve the backend to support apollo subscriptions.

Subscriptions are the goal, since they are likely much more efficient
and scale, but for the purposes of a demo we think that just using built
in polling functionality of ApolloClient will do the trick.

Issue #45
Copy link
Contributor

@reefdog reefdog left a comment

Choose a reason for hiding this comment

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

Polling is theoretically a fine stopgap, but we will need to do a little work before this works as expected.

✅ The live transcript polls correctly as-is, because upon app init we generate an after timestamp parameter ("five minutes before now"), and then each query poll uses that same fixed timestamp and just gets everything newer.

❌ The entity list doesn't poll correctly as-is. It uses a before/after timestamp pair to grab a fixed range. (That's legacy from the first TPT pass that included more granular date span controls. Now we're just doing "past four hours", "past 24 hours", and "past week".) But what's happening now is that we calculate those during app init (and if the interval selector is changed), but then the same fixed timestamp window is passed to each query poll, so the data never changes. If we load the page at 5:00 PM (and keep the "past four hours" default), then we're always polling for 1:00-5:00 PM.

What we really need to do, then, is adjust the before/after timestamps before each query is run so that the entity list is a moving window of data. Hopefully we still get some of the efficiencies of polling, but we might be doing a complete re-render each time. That'd be unfortunate and would seem to indicate polling isn't a fit for the entity list. (And in the case of the expensive queries, like "past 24 hours", would currently cause queries to stack up.)

@slifty We should chat about how to resolve this.

@slifty
Copy link
Member Author

slifty commented Mar 20, 2019

We talked a bit about this -- a potential path: have the initial data loaded in bulk; stored in a way that makes it easy to "drop" data as it expires, and also loading incremental updates going forward.

That way we are avoiding running the expensive "last 24 hours" or "last 2 weeks" queries every few seconds, but we are still able to update numbers in a responsive way.

Justin I'm thinking we should probably close this PR and delete the branch, since this is a more significant refactor that will warrant its own issue / branch pairing.

@reefdog
Copy link
Contributor

reefdog commented Mar 22, 2019

Justin I'm thinking we should probably close this PR and delete the branch, since this is a more significant refactor that will warrant its own issue / branch pairing.

Agree. Polling is great for demo (and may stay great for the live transcript), but we'll need to refactor around Subs and performance at some point.

@reefdog reefdog closed this Mar 22, 2019
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