Remove count restriction in leader topology #299
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
krnowak
suggested changes
Jun 21, 2018
examples/leader/README.md
Outdated
|
|
||
| Note: Whenever creating a `leader` topology specify instance `count` of 3 or more and would be best if the number is odd, this is so the election can take place. | ||
| Note: To have a functioning cluster, you must set the `count` to atleast 3, when creating a `leader` topology. It would be best to if the number is odd, as this prevents a split quorum during the leader election. |
There was a problem hiding this comment.
How about:
Note: To have functioning services in the `leader` topology, you must set the `count` field to at least 3. It is recommended that the number is odd as this prevents a split quorum during the leader election.
examples/leader/habitat.yml
Outdated
| @@ -10,8 +10,10 @@ spec: | |||
| v1beta2: | |||
| # the core/redis habitat service packaged as a Docker image | |||
| image: habitat/redis-hab | |||
| # count must be at least 3 for a leader-follower topology | |||
| count: 3 | |||
| # While it is possible to deploy only one pod in the | |||
There was a problem hiding this comment.
While it is possible to deploy only one pod with a service instance in the
examples/leader/habitat.yml
Outdated
| # count must be at least 3 for a leader-follower topology | ||
| count: 3 | ||
| # While it is possible to deploy only one pod in the | ||
| # leader-follower topology, at least at least 3 instances are |
There was a problem hiding this comment.
At least one at least too many. ;)
examples/leader/habitat.yml
Outdated
| count: 3 | ||
| # While it is possible to deploy only one pod in the | ||
| # leader-follower topology, at least at least 3 instances are | ||
| # required for a functioning cluster |
There was a problem hiding this comment.
required for services to be started by the supervisor.
pkg/controller/v1beta2/utils.go
Outdated
| switch spec.Service.Topology { | ||
| case habv1beta1.TopologyStandalone: | ||
| case habv1beta1.TopologyLeader: | ||
| if spec.Count < leaderFollowerTopologyMinCount { |
There was a problem hiding this comment.
I would just drop this if instead of rewriting the switch to if. What do you think?
indradhanush
force-pushed
the
indradhanush/remove-count-restriction-in-leader-topology
branch
2 times, most recently
from
4667f91
to
aead737
Compare
krnowak
suggested changes
Jul 12, 2018
| @@ -54,9 +54,6 @@ func validateCustomObject(h habv1beta1.Habitat) error { | |||
| switch spec.Service.Topology { | |||
| case habv1beta1.TopologyStandalone: | |||
| case habv1beta1.TopologyLeader: | |||
| if spec.Count < leaderFollowerTopologyMinCount { | |||
There was a problem hiding this comment.
Is the leaderFollowerTopologyMinCount used anywhere after dropping this line? If no, then maybe drop the constant too.
| @@ -54,9 +54,6 @@ func validateCustomObject(h habv1beta1.Habitat) error { | |||
| switch spec.Service.Topology { | |||
| case habv1beta1.TopologyStandalone: | |||
| case habv1beta1.TopologyLeader: | |||
| if spec.Count < leaderFollowerTopologyMinCount { | |||
There was a problem hiding this comment.
Same here about leaderFollowerTopologyMinCount.
While atleast three instances are required for a functioning cluster in the leader topology, we should not prevent users from making deployments with initial count set to one. This change allows users to incrementally add instances if that fits their requirements better instead of forcing them to add atleast three right from the onset. Signed-off-by: Indradhanush Gupta <indra@kinvolk.io>
indradhanush
force-pushed
the
indradhanush/remove-count-restriction-in-leader-topology
branch
from
aead737
to
1640cbd
Compare
krnowak
approved these changes
Jul 12, 2018
|
LFAD. |
|
Merged on the command line. |
indradhanush
deleted the
indradhanush/remove-count-restriction-in-leader-topology
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While atleast three instances are required for a functioning cluster
in the leader topology, we should not prevent users from making
deployments with initial count set to one. This change allows users to
incrementally add instances if that fits their requirements better
instead of forcing them to add atleast three right from the onset.
Signed-off-by: Indradhanush Gupta indra@kinvolk.io