-
-
Notifications
You must be signed in to change notification settings - Fork 61
build: Upgrade ClickHouse version bounds and fix migrations for CH 25.8 compatibility #7800
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
base: master
Are you sure you want to change the base?
Changes from 11 commits
48c3979
3c4d465
aa91c19
53ecf11
98cd815
0710b04
72932a2
86d3898
b6250af
b65eb41
df4484a
6cc7b73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,2 @@ | ||
| CLICKHOUSE_SERVER_MIN_VERSION = "23.8.11.29" | ||
| # Note: SaaS, self-hosted, and sentry dev | ||
| # environements should all be on 23.8.11.29 | ||
| CLICKHOUSE_SERVER_MAX_VERSION = "25.3.6.10034" | ||
| CLICKHOUSE_SERVER_MIN_VERSION = "25.3.6.10034" | ||
| CLICKHOUSE_SERVER_MAX_VERSION = "25.8.16.10001" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,33 +2,57 @@ | |
|
|
||
| from snuba.clusters.storage_sets import StorageSetKey | ||
| from snuba.migrations import migration, operations | ||
| from snuba.migrations.migration_utilities import get_clickhouse_version_for_storage_set | ||
| from snuba.migrations.operations import OperationTarget | ||
|
|
||
|
|
||
| class Migration(migration.ClickhouseNodeMigration): | ||
| blocking = False | ||
| storage_set_key = StorageSetKey.EVENTS_ANALYTICS_PLATFORM | ||
| class _AlterWorkloadOp(operations.RunSql): | ||
| """Version-aware RunSql that picks the correct thread setting at execution time.""" | ||
|
|
||
| def forwards_ops(self) -> Sequence[operations.SqlOperation]: | ||
| alter_workload = """ | ||
| def __init__(self) -> None: | ||
| # Placeholder statement for validation; actual SQL is generated in execute() | ||
| super().__init__( | ||
| storage_set=StorageSetKey.EVENTS_ANALYTICS_PLATFORM, | ||
| statement="SELECT 1", | ||
| target=OperationTarget.LOCAL, | ||
| ) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dry-run outputs placeholder "SELECT 1" instead of actual SQLMedium Severity
|
||
|
|
||
| def _build_statement(self, thread_setting: str) -> str: | ||
| return f""" | ||
| CREATE OR REPLACE WORKLOAD low_priority_deletes | ||
| IN all | ||
| SETTINGS | ||
| priority = 100, | ||
| max_requests = 2, | ||
| max_threads = 4; | ||
| {thread_setting} = 4; | ||
| """ | ||
|
|
||
| return [ | ||
| operations.RunSql( | ||
| storage_set=self.storage_set_key, | ||
| statement=alter_workload, | ||
| target=OperationTarget.LOCAL, | ||
| ), | ||
| ] | ||
| def execute(self) -> None: | ||
| ch_version = get_clickhouse_version_for_storage_set(self._storage_set, None) | ||
|
|
||
| # max_threads was renamed to max_concurrent_threads in ClickHouse 25.8+ | ||
| if ch_version >= (25, 8): | ||
| thread_setting = "max_concurrent_threads" | ||
| else: | ||
| thread_setting = "max_threads" | ||
|
|
||
| resolved = operations.RunSql( | ||
| storage_set=self._storage_set, | ||
| statement=self._build_statement(thread_setting), | ||
| target=OperationTarget.LOCAL, | ||
| ) | ||
| resolved.execute() | ||
|
|
||
|
|
||
| class Migration(migration.ClickhouseNodeMigration): | ||
| blocking = False | ||
| storage_set_key = StorageSetKey.EVENTS_ANALYTICS_PLATFORM | ||
|
|
||
| def forwards_ops(self) -> Sequence[operations.SqlOperation]: | ||
| return [_AlterWorkloadOp()] | ||
|
|
||
| def backwards_ops(self) -> Sequence[operations.SqlOperation]: | ||
| # Restore original settings without max_threads | ||
| # Restore original settings without thread limit | ||
| alter_workload = """ | ||
| CREATE OR REPLACE WORKLOAD low_priority_deletes | ||
| IN all | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,9 +52,9 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]: | |
| tags.key, | ||
| tags.indexed_value, | ||
| tags.raw_value, | ||
| maxState(timestamp as raw_timestamp) as last_timestamp, | ||
| maxState(timestamp) as last_timestamp, | ||
| toDateTime(multiIf(granularity=0,10,granularity=1,60,granularity=2,3600,granularity=3,86400,-1) * | ||
| intDiv(toUnixTimestamp(raw_timestamp), | ||
| intDiv(toUnixTimestamp(timestamp), | ||
| multiIf(granularity=0,10,granularity=1,60,granularity=2,3600,granularity=3,86400,-1))) as rounded_timestamp, | ||
| least(retention_days, | ||
| multiIf(granularity=0,decasecond_retention_days, | ||
|
Comment on lines
52
to
60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: In migration Suggested FixUpdate the Prompt for AI Agent |
||
|
|
@@ -68,7 +68,7 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]: | |
| sumState(sampling_weight * arrayJoin(gauges_values.sum)) AS sum_weighted, | ||
| sumState(arrayJoin(gauges_values.count)) as count, | ||
| sumState(sampling_weight * arrayJoin(gauges_values.count)) AS count_weighted, | ||
| argMaxState(arrayJoin(gauges_values.last), raw_timestamp) as last | ||
| argMaxState(arrayJoin(gauges_values.last), timestamp) as last | ||
| FROM generic_metric_gauges_raw_local | ||
| WHERE materialization_version = 3 | ||
| AND metric_type = 'gauge' | ||
|
|
@@ -80,7 +80,7 @@ def forwards_ops(self) -> Sequence[operations.SqlOperation]: | |
| tags.key, | ||
| tags.indexed_value, | ||
| tags.raw_value, | ||
| raw_timestamp, | ||
| timestamp, | ||
| granularity, | ||
| retention_days | ||
| """, | ||
|
|
||


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.
Min version set to old value instead of new default
Medium Severity
CLICKHOUSE_SERVER_MIN_VERSIONis set to"25.3.6.10034"(the old default image version) instead of"25.3.8.10041"(the new default). The PR description explicitly states the min bound is25.3.8.10041, and the supported versions doc lists25.3.8.10041as the lowest supported version—yet the version check incheck_clickhousewill accept25.3.6.10034, which is no longer listed as a supported version.