Skip to content

Conversation

@jgraff2
Copy link
Contributor

@jgraff2 jgraff2 commented Sep 25, 2019

This PR includes some refactoring so it may not be immediately clear what changed - these are the functional changes:

  1. set lastUpdateTime at the beginning of handleEvent, for all event types
  2. setup timeouts based on aspects in hierarchy
  3. load published aspects only
  4. don't update timeouts on events anymore

1 and 2 are the bugs that caused issues in prod when it was released last week.
The lastUpdateTime was only being updated on aspect events, which caused the auto-reload to always trigger after the timeout period.
The timeout was being set based on all aspects that matched the perspective filters, which caused problems for perspectives with no filters - it was set to the minimum of all aspects in prod (6s), causing those perspectives to reload every 12s.

3 is a related bug I noticed while working on this. It didn't have an impact in prod last week as far as I know, but it does have the potential to cause issues and it's better not to track extra aspects unnecessarilly.

4 is a decision I made to simplify the code and wrap this up more quickly. I was running into lots of problems getting the updates working with the hierarchy-based approach, and I realized it really isn't worth the extra complexity. Instead, the timeouts will be set solely based on the data that was present on page load. Most of the time this should be exactly the same, and in the rare case that an update happens that would have changed the timeout, worst case is either the page reloads unnecessarily once, or if events stop coming in it doesn't reload as quickly as it possibly could. Not a big deal, and not worth the overly complicated tracking logic required to handle those cases.

@coveralls
Copy link

coveralls commented Sep 25, 2019

Coverage Status

Coverage decreased (-0.01%) to 89.15% when pulling 8460dfa on interception-fix into 431a303 on master.

Copy link
Contributor

@iamigo iamigo left a comment

Choose a reason for hiding this comment

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

Please let me know when this is available in sandbox ;)

@jgraff2 jgraff2 merged commit b83877b into master Sep 26, 2019
@jgraff2 jgraff2 temporarily deployed to refocus-staging October 22, 2019 23:33 Inactive
@cassiodias cassiodias deleted the interception-fix branch March 14, 2020 12:30
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.

6 participants