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

Initial backends dead #253

Closed
wants to merge 8 commits into from
Closed

Conversation

KnicKnic
Copy link
Contributor

@KnicKnic KnicKnic commented Jan 14, 2020

Idea was that when backends are initially added they should be dead if Healthchecking is occurs.

This PR builds upon #252 and completes #251

@yyyar
Copy link
Owner

yyyar commented Jan 23, 2020

Some questions:

  • What are the benefits of introducing new type CheckResultLiveness. Why not use existing bool value? The new type also adds a possibility to panic if it is not a valid string.

@KnicKnic
Copy link
Contributor Author

KnicKnic commented Jan 23, 2020

Some questions:

  • What are the benefits of introducing new type CheckResultLiveness. Why not use existing bool value? The new type also adds a possibility to panic if it is not a valid string.

These are two separate things.

  1. CheckResultLiveness

    1. Originally this was just live. Live denoted if the backend was live or not. If we transition from live to failed, or failed to live; we would signal that the backend changed state. However I needed a new state that was neither live or failed. A state that could transition to either live or failed, and guarantees to signal regardless of what state it started in. This is why an enum with three states was introduced. Initial, live or failed.
  2. InitialBackendStatus

    1. Now that backends can go to fail or live, the question is what state to start them off in. I wanted to always start them off as failed so they did not get traffic until they passed health check (if they were configured with health check[start them off as live if they were not configured with health check])
      1. However after talking on the issue page discovered backends are added live before healthcheck runs #251 (comment) @nickdoikov suggested that I default to the old behavior and make it configurable, and default to the old behavior and add backends as live.

Onto the panic. After I submitted this PR I saw that you had a contributors guidlines and that panic is frowned up. I was going to state that hey I was following pattern at location X, when I read the message it stated that manager first checked the result. As such I have added another commit which first validates the change in manager. see commit KnicKnic@470b0cf

@illarion illarion added this to the Selected for development milestone Jul 7, 2020
Copy link
Collaborator

@illarion illarion left a comment

Choose a reason for hiding this comment

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

  1. If there are only 2 options: live or dead, why use additional type for that, instead of simple bool?
  2. Please make a separate PR with typos fixed in comments of gobetween.toml, if you want. These changes are not related to the original PR

@illarion illarion marked this pull request as draft July 8, 2020 09:49
@illarion illarion self-assigned this Jul 18, 2020
@KnicKnic
Copy link
Contributor Author

KnicKnic commented Jul 22, 2020

  1. If there are only 2 options: live or dead, why use additional type for that, instead of simple bool?
    I tried to answer this question in my previous response.

"CheckResultLiveness

Originally this was just live. Live denoted if the backend was live or not. If we transition from live to failed, or failed to live; we would signal that the backend changed state. However I needed a new state that was neither live or failed. A state that could transition to either live or failed, and guarantees to signal regardless of what state it started in. This is why an enum with three states was introduced. Initial, live or failed."

  1. Please make a separate PR with typos fixed in comments of gobetween.toml, if you want. These changes are not related to the original PR

Done!

@KnicKnic KnicKnic marked this pull request as ready for review July 24, 2020 04:59
@KnicKnic KnicKnic requested a review from illarion July 24, 2020 05:14
illarion added a commit that referenced this pull request Aug 3, 2020
@illarion illarion closed this Aug 3, 2020
@illarion
Copy link
Collaborator

illarion commented Aug 3, 2020

Merged

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.

None yet

3 participants