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: revert ClickHouse replicas number #1392

Merged

Conversation

zacharyarnaise
Copy link
Contributor

@zacharyarnaise zacharyarnaise commented Aug 22, 2024

Revert the ClickHouse replicas value modified by this previous PR:

AFAIK you can't simply change the number of ClickHouse replicas on an existing installation and except it to work
As of today, in this chart setting x ClickHouse replicas will result in x shards created so the data is split across multiple servers.

I suggest we simply revert this for now.
I understand it would be great to provide better defaults for anyone doing a fresh install of this chart, so I think the best course of action would be to integrate the change I reverted in a major version release instead and inform on the consequences of this change.

@ivoistaken
Copy link

Absolutely.
You can't change replicas, this breaks sharding. Clickhouse does not automatically recalculate/redistribute shards.
Here is more info: https://clickhouse.com/docs/en/guides/sre/scaling-clusters

@Mokto Mokto merged commit ad6fc29 into sentry-kubernetes:develop Aug 22, 2024
1 of 2 checks passed
@Mokto
Copy link
Contributor

Mokto commented Aug 22, 2024

Thanks.

@Mokto Mokto mentioned this pull request Aug 22, 2024
@zacharyarnaise zacharyarnaise deleted the fix_clickhouse_replicas branch August 22, 2024 09:46
@zacharyarnaise
Copy link
Contributor Author

@Mokto Sorry but I think we might need to do the same thing with the replicas of zookeeper-clickhouse as well. I'm investigating the failed check on my PR to be sure.
I have to admit you merged this faster than I anticipated 😅

@zacharyarnaise
Copy link
Contributor Author

@Mokto Looks like the failure was unrelated, the checks passed just fine in #1393 so I think we're good to go!

@Mokto
Copy link
Contributor

Mokto commented Aug 22, 2024

Thanks for the heads up

@mosesdd
Copy link

mosesdd commented Aug 22, 2024

I think its the same for Kafka Controllers isnt it?

@deadlybore
Copy link

deadlybore commented Aug 22, 2024

Well it was a major version bump from 23.x.x to 24.x.x and it was described as a breaking change in the changelog

@Mokto
Copy link
Contributor

Mokto commented Aug 22, 2024

That's true. Maybe I should kept the values from 24.0.0.

It's up to everyone to update the replica values

@mosesdd
Copy link

mosesdd commented Aug 22, 2024

Yeah major version with breaking changes and WITHOUT upgrade instructions. I'm using this chart for years now and thinks like that don't happen the first time.
We need to make sure which replicas are safe to scale down and which have to be updated manually before upgrading to 24.0.0 and put this in the README.

@deadlybore
Copy link

Seems pretty clear to me that if you upgrade without reviewing the change it will break https://github.com/sentry-kubernetes/charts/blob/74a9838390d9320cfab57f7588f48586f8597737/charts/sentry/CHANGELOG.md#-breaking-changes

@mosesdd
Copy link

mosesdd commented Aug 22, 2024

But its missing here https://github.com/sentry-kubernetes/charts/blob/74a9838390d9320cfab57f7588f48586f8597737/README.md

Also its not clear what exactly to change. Which replicas are safe to scale down and which are not. As we see, Clickhouse is not save to scale down and Kafka is not even working scaled down after clean install: #1394

Mokto added a commit that referenced this pull request Aug 23, 2024
@Mokto
Copy link
Contributor

Mokto commented Aug 23, 2024

I reverted the change again in 25.0.0.

And added it to the changelog, just in case. even though it was stated in the release notes

@mosesdd
Copy link

mosesdd commented Aug 23, 2024

@Mokto well, now clickhouse replica is in the wrong section again so it uses default value of clickhouse chart with 3 replicas...

@Mokto
Copy link
Contributor

Mokto commented Aug 23, 2024

What a mess. it's fixed, sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants