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

fix: Specify the number of read replicas as one less that total (#190) #193

Closed

Conversation

emillbrandt
Copy link

@emillbrandt emillbrandt commented Feb 17, 2021

Description

replica_scale_min and replica_count are used incorrectly as the total cluster node count.

Motivation and Context

#190
Currently setting read replicas to 0 deletes the writer instance.
Once extra auto scaling read replicas gets created with the auto-scaling policy, wasting money.

Breaking Changes

None.

How Has This Been Tested?

Created a cluster with zero read replicas, one writer and zero read replicas were created
Created a cluster with one read replicas, one writer and one read replicas were created
Created a cluster with two read replicas, one writer and two read replicas were created

@svenlito
Copy link

svenlito commented Apr 5, 2021

@emillbrandt Hey, thank you for the PR! Would you mind resolving the conflict here?

@antonbabenko antonbabenko requested a review from bryantbiggs April 5, 2021 17:20
@antonbabenko
Copy link
Member

This is a breaking change to my mind.

I wonder how bad is this breaking change for users who are using this module already. If this PR is merged they will suddenly have 1 more instance, right? This is not what people expect.

@bryantbiggs What do you think?

@emillbrandt
Copy link
Author

@svenlito Done

@bryantbiggs
Copy link
Member

yes I agree @antonbabenko - this would fall under breaking change I think since it will create 1 additional instance in everyones cluster. its not as severe, but still a potentially disruptive change. maybe we hold off for a bit, there are a few things that we know of that we would like to fix at the next breaking change (i.e. -

# TODO - remove coalesce() at next breaking change - adding existing name as fallback to maintain backwards compatibility
, maybe set min TF to 0.13.x, change some variable wording, etc.)

@antonbabenko
Copy link
Member

Agree, let's put this on hold and make a bigger release with the changes you just described.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants