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.

Sign up for GitHub

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

Jump to bottom

Do not surface left servers #6420

Merged
merged 3 commits into from
Oct 9, 2019
Merged

Do not surface left servers #6420

merged 3 commits into from
Oct 9, 2019

Conversation

hanshasselberg
Copy link
Member

Left servers will never come back so there is no reason to surface them in GetDatacentersByDistance. One of the results is that dcs with only left servers will no longer show up in v1/catalog/datacenters, which is the right thing to do since this dc is not reachable.

@hanshasselberg hanshasselberg force-pushed the no_left_servers_from_catalog branch from 706e7ba to fcf8421 Compare August 29, 2019 08:26
@hanshasselberg hanshasselberg force-pushed the no_left_servers_from_catalog branch from fcf8421 to f0cb17f Compare August 29, 2019 08:27
freddygv
freddygv approved these changes Aug 29, 2019
View reviewed changes
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

Overall I think this looks good, made one minor comment inline but it's not a blocker.

agent/router/router.go Outdated Show resolved Hide resolved
@hanshasselberg hanshasselberg force-pushed the no_left_servers_from_catalog branch from d38f2ee to b3eb511 Compare August 29, 2019 15:39
@freddygv
Copy link
Contributor

This feels like a small breaking change since the output of an endpoint is changing. Maybe we should get it into 1.7.0 but not 1.6.x?

@pierresouchay
Copy link
Contributor

@freddygv Well, this change the output in very rare occasions (when DC is dead for more than 72 hours or operator did force-leave the servers), so it looks to me like a very reasonable choice that would unlikely break workflows (while helping a lot during decommission).
This looks like a great change to me.

@hanshasselberg
Copy link
Member Author

I agree with @pierresouchay on this one. I think this a bugfix, what would anyone want to do with decommissioned datacenters... And thus I think it would be fine to include it into 1.6.x series.

@hanshasselberg
Copy link
Member Author

I noticed the other functions that are returning datacenters and I want to make sure to make them all behave similarly.

@schristoff schristoff merged commit b6499fe into master Oct 9, 2019
@hanshasselberg hanshasselberg mentioned this pull request Nov 18, 2019
Closed
@schristoff schristoff deleted the no_left_servers_from_catalog branch November 19, 2019 22:23
@ghost
Copy link

ghost commented Jan 25, 2020

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.

@ghost ghost locked and limited conversation to collaborators Jan 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@freddygv freddygv freddygv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hanshasselberg @freddygv @pierresouchay @schristoff