-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: Bump dev clickhouse version to 23.11 #19434
Conversation
@@ -25,7 +25,7 @@ services: | |||
# Note: please keep the default version in sync across | |||
# `posthog` and the `charts-clickhouse` repos | |||
# | |||
image: ${CLICKHOUSE_SERVER_IMAGE:-clickhouse/clickhouse-server:23.6.1.1524} | |||
image: ${CLICKHOUSE_SERVER_IMAGE:-clickhouse/clickhouse-server:23.11.2.11-alpine} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why alpine
? it's about 15% smaller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that since ClickHouse uses ICU for collation that this means the merge tree ordering should be stable if a volume is reused across these images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clickhouse is really great about forwards compatibility of merge tree files on disk. You have to worry more about backwards compatibility, but even then it's really great and you can suppress things that will make it backwards incompatible (we've done that before) like with compress_primary_key
. This is actually something I'll be looking to enable since we'd fail back to a version that supports it.
https://clickhouse.com/codebrowser/ClickHouse/src/Storages/MergeTree/MergeTreeIOSettings.h.html#54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually interesting - it won't rework the parts and so for some time (done via merge) so you'll have a mix of parts written by different version of CH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
b0d4816
to
531efcc
Compare
* master: (94 commits) feat: Add cta on billing for >20k annual spend customers (#19508) refactor(temporal/squash): Support flat person override table in squash workflow (#19347) fix(surveys): remove link in user interview template (#19584) feat: populate plugin capabilities on install and edit (#19188) chore: Make plugin-server ignore deleted plugin configs (#18444) chore: Add Flutter feature flags snippets (#19563) fix(bi): fixed some of the query duplications (#19573) fix: assume typeless series nodes are of type events node (#19550) chore(deps): bump chromaui/action from 1 to 10 (#19560) fix(trends): fix breakdowns persons label (#19534) fix(trends): fix breakdown legend (#19533) feat: incremental updates for mobile transformer (#19567) chore(deps): bump peter-evans/find-comment from 1 to 2 (#19559) chore(data-warehouse): cleanup unused celery code and extend time (#19568) Revert "feat(data-warehouse): hubspot integration" (#19569) feat(data-warehouse): hubspot integration (#19529) feat: Feature gate session replay controls using available_product_features (#19401) fix: Padding on bullet lists (#19565) chore: Post 3000 LemonButtton cleanup (#19540) chore(deps): bump aws-actions/amazon-ecr-login from 1 to 2 (#19558) ...
# create table + 3 move operations | ||
self.assertEqual(len(MIGRATION_DEFINITION.operations), 4) | ||
self.assertEqual(len(MIGRATION_DEFINITION.operations), 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't make a lot of sense with the subsequent line now since it's off by one. I'm curious (but don't know much about these migrations) -- what happened here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea and it applies to both versions so I'm assuming this has been broken for some time now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at this for some time, but decided...since we'll probably nuke async migrations as they exist right now (please be temporal based) we can safely ignore this. It definitely won't impact production unless we decide to reuse async migration functionality.
@@ -25,7 +25,7 @@ services: | |||
# Note: please keep the default version in sync across | |||
# `posthog` and the `charts-clickhouse` repos | |||
# | |||
image: ${CLICKHOUSE_SERVER_IMAGE:-clickhouse/clickhouse-server:23.6.1.1524} | |||
image: ${CLICKHOUSE_SERVER_IMAGE:-clickhouse/clickhouse-server:23.11.2.11-alpine} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming that since ClickHouse uses ICU for collation that this means the merge tree ordering should be stable if a volume is reused across these images?
Problem
We will be moving to 23.11 on US and EU in prep for upgrade of EU cluster. This is the first step in that process
Changes
local dev and test will now be on 23.11. Next will be CI matrix testing.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?