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

Update app instance cell data when scaling up #3133

Merged
merged 4 commits into from
Dec 3, 2018

Conversation

richard-cox
Copy link
Contributor

@richard-cox richard-cox commented Oct 17, 2018

  • Every time we scale up app instances we now refresh the data we use to determine which cell the app instance is in
  • This helps avoid situations where user scales down and then scales up and old instance cell data is used
  • Unfortunately the firehose only emits this information every 60 seconds, so if the scale down + up happens within this time the old data is still used. There's not much we can do about this without lots of code
  • fixes Clear metrics used to show app instance cell when scaling #3126

- Every time we scale up app instances we now refresh the data we use to determine which cell the app instance is in
- This helps avoid situations where user scales down and then scales up and old instance cell data is used
- Unfortunately the firehose only emits this information every 60 seconds, so if the scale down + up happens within this time the old data is still used. There's not much we can do about this without lots of code
@cfdreddbot
Copy link

Hey richard-cox!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@codecov
Copy link

codecov bot commented Oct 17, 2018

Codecov Report

Merging #3133 into v2-master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@              Coverage Diff              @@
##           v2-master    #3133      +/-   ##
=============================================
- Coverage      71.09%   71.08%   -0.02%     
=============================================
  Files            632      632              
  Lines          27752    27771      +19     
  Branches        6314     6317       +3     
=============================================
+ Hits           19731    19741      +10     
- Misses          8021     8030       +9

Copy link
Contributor

@nwmac nwmac left a comment

Choose a reason for hiding this comment

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

This should check that metrics are available - if you have a system without metrics, we will make the API request and you'll see a 500 in the network tab - we should not make the request unless we have metrics for the App/CF.

@nwmac nwmac added the needs attention This PR needs attention label Oct 19, 2018
@richard-cox richard-cox added comments-addressed and removed needs attention This PR needs attention labels Oct 30, 2018
Copy link
Contributor

@nwmac nwmac left a comment

Choose a reason for hiding this comment

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

LGTM

@KlapTrap KlapTrap added the conflicts Merge conflicts on PR label Nov 20, 2018
@richard-cox richard-cox removed the conflicts Merge conflicts on PR label Nov 21, 2018
@nwmac nwmac merged commit b371dae into v2-master Dec 3, 2018
@nwmac nwmac deleted the fix-stale-cell-on-instance-scale branch December 3, 2018 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clear metrics used to show app instance cell when scaling
4 participants