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.

Sign up for GitHub

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: add support for coordinator schemas #28031

Merged
merged 23 commits into from
Jan 31, 2025
+292 −73
Merged

feat: add support for coordinator schemas #28031

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
158c430
chore: update migrations to use ClickhouseCluster class instead
Daesgar Jan 28, 2025
22110bd
chore: refactor compose and migrations to use system.clusters info
Daesgar Jan 29, 2025
b2e050f
Update query snapshots
github-actions[bot] Jan 29, 2025
a3a126d
chore: allow running migrations on worker or coordinator nodes
Daesgar Jan 29, 2025
7d63488
Merge branch 'coordinator-migrations' of github.com:PostHog/posthog i…
Daesgar Jan 29, 2025
265bae4
fix: pass port when building tuple
Daesgar Jan 29, 2025
ac7bf42
Merge remote-tracking branch 'origin/master' into coordinator-migrations
Daesgar Jan 29, 2025
a0c7feb
fix: linting
Daesgar Jan 29, 2025
5263982
Merge branch 'master' into coordinator-migrations
Daesgar Jan 29, 2025
294f3b1
Merge remote-tracking branch 'origin/master' into coordinator-migrations
Daesgar Jan 30, 2025
6595959
chore: add test for node role filtering
Daesgar Jan 30, 2025
37267ed
fix: mock bootstrap_client instead of private attribute
Daesgar Jan 30, 2025
32673da
chore: update hobby deployment
Daesgar Jan 30, 2025
8246bce
ref: update configs to leave hobby deployment with its previous config
Daesgar Jan 30, 2025
36f3789
Merge remote-tracking branch 'origin/master' into coordinator-migrations
Daesgar Jan 30, 2025
18f64e8
chore: create map_hosts_by_role
Daesgar Jan 30, 2025
e7867dd
fix: update comment
Daesgar Jan 30, 2025
71439c5
fix: replace paremters properly
Daesgar Jan 30, 2025
bd654ba
fix: revert parameters replace
Daesgar Jan 30, 2025
21d1fde
Merge branch 'master' into coordinator-migrations
Daesgar Jan 30, 2025
8584672
Merge remote-tracking branch 'origin/master' into coordinator-migrations
Daesgar Jan 31, 2025
6ba1e5f
chore: add posthog_migrations to hobby deployment
Daesgar Jan 31, 2025
ed24ba3
fix: add macros for hobby deployment
Daesgar Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .flox/env/manifest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,10 @@ fi

if [[ -t 0 ]]; then # The block below only runs when in an interactive shell
# Add required entries to /etc/hosts if not present
if ! grep -q "127.0.0.1 kafka clickhouse" /etc/hosts; then
if ! grep -q "127.0.0.1 kafka clickhouse clickhouse-coordinator" /etc/hosts; then
echo
echo "🚨 Amending /etc/hosts to map hostnames 'kafka' and 'clickhouse' to 127.0.0.1..."
echo "127.0.0.1 kafka clickhouse" | sudo tee -a /etc/hosts 1> /dev/null
echo "127.0.0.1 kafka clickhouse clickhouse-coordinator" | sudo tee -a /etc/hosts 1> /dev/null
echo "✅ /etc/hosts amended"
Daesgar marked this conversation as resolved.
Show resolved Hide resolved
fi

Expand Down
1 change: 1 addition & 0 deletions docker-compose.dev-full.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ services:
- ./posthog/idl:/idl
- ./docker/clickhouse/docker-entrypoint-initdb.d:/docker-entrypoint-initdb.d
- ./docker/clickhouse/config.xml:/etc/clickhouse-server/config.xml
- ./docker/clickhouse/config.d/default.xml:/etc/clickhouse-server/config.d/default.xml
- ./docker/clickhouse/users-dev.xml:/etc/clickhouse-server/users.xml
- ./docker/clickhouse/user_defined_function.xml:/etc/clickhouse-server/user_defined_function.xml
- ./posthog/user_scripts:/var/lib/clickhouse/user_scripts
Expand Down
20 changes: 19 additions & 1 deletion docker-compose.dev.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ services:
ports:
- '5555:5555'

clickhouse:
clickhouse: &clickhouse
extends:
file: docker-compose.base.yml
service: clickhouse
Expand All @@ -69,6 +69,7 @@ services:
- ./posthog/idl:/idl
- ./docker/clickhouse/docker-entrypoint-initdb.d:/docker-entrypoint-initdb.d
- ./docker/clickhouse/config.xml:/etc/clickhouse-server/config.xml
- ./docker/clickhouse/config.d/worker.xml:/etc/clickhouse-server/config.d/worker.xml
- ./docker/clickhouse/users-dev.xml:/etc/clickhouse-server/users.xml
- ./docker/clickhouse/user_defined_function.xml:/etc/clickhouse-server/user_defined_function.xml
- ./posthog/user_scripts:/var/lib/clickhouse/user_scripts
Expand All @@ -78,6 +79,23 @@ services:
- kafka
- zookeeper

clickhouse-coordinator:
hostname: clickhouse-coordinator
<<: *clickhouse
volumes:
# this new entrypoint file is to fix a bug detailed here https://github.com/ClickHouse/ClickHouse/pull/59991
# revert this when we upgrade clickhouse
- ./docker/clickhouse/entrypoint.sh:/entrypoint.sh
- ./posthog/idl:/idl
- ./docker/clickhouse/docker-entrypoint-initdb.d:/docker-entrypoint-initdb.d
- ./docker/clickhouse/config.xml:/etc/clickhouse-server/config.xml
- ./docker/clickhouse/config.d/coordinator.xml:/etc/clickhouse-server/config.d/coordinator.xml
- ./docker/clickhouse/users-dev.xml:/etc/clickhouse-server/users.xml
- ./docker/clickhouse/user_defined_function.xml:/etc/clickhouse-server/user_defined_function.xml
- ./posthog/user_scripts:/var/lib/clickhouse/user_scripts
Comment on lines +85 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider deduplicating volume mounts between clickhouse and clickhouse-coordinator services using YAML anchors

ports:
- '9001:9001'

zookeeper:
extends:
file: docker-compose.base.yml
Expand Down
1 change: 1 addition & 0 deletions docker-compose.hobby.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ services:
- ./posthog/posthog/idl:/idl
- ./posthog/docker/clickhouse/docker-entrypoint-initdb.d:/docker-entrypoint-initdb.d
- ./posthog/docker/clickhouse/config.xml:/etc/clickhouse-server/config.xml
- ./posthog/docker/clickhouse/config.d/default.xml:/etc/clickhouse-server/config.d/default.xml
- ./posthog/docker/clickhouse/users.xml:/etc/clickhouse-server/users.xml
- clickhouse-data:/var/lib/clickhouse
zookeeper:
Expand Down
42 changes: 42 additions & 0 deletions docker/clickhouse/config.d/coordinator.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<clickhouse>
<tcp_port>9001</tcp_port>
<remote_servers>
<posthog>
<shard>
<replica>
<host>clickhouse</host>
<port>9000</port>
</replica>
</shard>
</posthog>
<posthog_single_shard>
<shard>
<replica>
<host>clickhouse</host>
<port>9000</port>
</replica>
</shard>
</posthog_single_shard>
<posthog_migrations>
<shard>
<replica>
<host>clickhouse</host>
<port>9000</port>
</replica>
</shard>
<shard>
<replica>
<host>clickhouse-coordinator</host>
<port>9001</port>
</replica>
</shard>
</posthog_migrations>
</remote_servers>

<macros>
<shard>02</shard>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: shard number '02' may conflict with existing shards if not carefully coordinated across the cluster

<replica>coord</replica>
<hostClusterType>online</hostClusterType>
<hostClusterRole>coordinator</hostClusterRole>
</macros>
</clickhouse>
27 changes: 27 additions & 0 deletions docker/clickhouse/config.d/default.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<clickhouse>
<tcp_port>9000</tcp_port>

<remote_servers>
<posthog>
<shard>
<replica>
<host>localhost</host>
<port>9000</port>
</replica>
</shard>
</posthog>
<posthog_single_shard>
<shard>
<replica>
<host>localhost</host>
<port>9000</port>
</replica>
</shard>
</posthog_single_shard>
</remote_servers>

<macros>
<shard>01</shard>
<replica>ch1</replica>
</macros>
</clickhouse>
42 changes: 42 additions & 0 deletions docker/clickhouse/config.d/worker.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<clickhouse>
<tcp_port>9000</tcp_port>
<remote_servers>
<posthog>
<shard>
<replica>
<host>clickhouse</host>
<port>9000</port>
</replica>
</shard>
</posthog>
<posthog_single_shard>
<shard>
<replica>
<host>clickhouse</host>
<port>9000</port>
</replica>
</shard>
</posthog_single_shard>
<posthog_migrations>
<shard>
<replica>
<host>clickhouse</host>
<port>9000</port>
</replica>
</shard>
<shard>
<replica>
<host>clickhouse-coordinator</host>
<port>9001</port>
</replica>
</shard>
</posthog_migrations>
</remote_servers>

<macros>
<shard>01</shard>
<replica>ch1</replica>
<hostClusterType>online</hostClusterType>
<hostClusterRole>worker</hostClusterRole>
</macros>
</clickhouse>
27 changes: 1 addition & 26 deletions docker/clickhouse/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
<errorlog>/var/log/clickhouse-server/clickhouse-server.err.log</errorlog>
<size>1000M</size>
<count>10</count>
<console>1</console>
</logger>

<http_port>8123</http_port>
<tcp_port>9000</tcp_port>
<mysql_port>9004</mysql_port>
<postgresql_port>9005</postgresql_port>
<https_port>8443</https_port>
Expand Down Expand Up @@ -162,25 +162,6 @@
<!-- Reallocate memory for machine code ("text") using huge pages. Highly experimental. -->
<remap_executable>false</remap_executable>

<remote_servers>
<posthog>
<shard>
<replica>
<host>localhost</host>
<port>9000</port>
</replica>
</shard>
</posthog>
<posthog_single_shard>
<shard>
<replica>
<host>localhost</host>
<port>9000</port>
</replica>
</shard>
</posthog_single_shard>
</remote_servers>

<remote_url_allow_hosts>
<host_regexp>.*</host_regexp>
</remote_url_allow_hosts>
Expand All @@ -192,12 +173,6 @@
</node>
</zookeeper>

<macros>
<shard>01</shard>
<replica>ch1</replica>
</macros>


<!-- Reloading interval for embedded dictionaries, in seconds. Default: 3600. -->
<builtin_dictionaries_reload_interval>3600</builtin_dictionaries_reload_interval>

Expand Down
6 changes: 6 additions & 0 deletions posthog/clickhouse/client/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ class Workload(Enum):
OFFLINE = "OFFLINE"


class NodeRole(Enum):
ALL = "ALL"
COORDINATOR = "COORDINATOR"
WORKER = "WORKER"


_default_workload = Workload.ONLINE


Expand Down
29 changes: 17 additions & 12 deletions posthog/clickhouse/client/migration_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,28 @@
from typing import Union
from collections.abc import Callable
import logging

from infi.clickhouse_orm import migrations

from posthog.clickhouse.client.execute import sync_execute
from posthog.clickhouse.client.connection import NodeRole
from posthog.clickhouse.cluster import get_cluster
from posthog.settings.data_stores import CLICKHOUSE_MIGRATIONS_CLUSTER

logger = logging.getLogger("migrations")

def run_sql_with_exceptions(sql: Union[str, Callable[[], str]], settings=None):

def run_sql_with_exceptions(sql: str, settings=None, node_role: NodeRole = NodeRole.WORKER):
"""
migrations.RunSQL does not raise exceptions, so we need to wrap it in a function that does.
node_role is set to WORKER by default to keep compatibility with the old migrations.
"""

if settings is None:
settings = {}
cluster = get_cluster(client_settings=settings, cluster=CLICKHOUSE_MIGRATIONS_CLUSTER)

def run_sql(database):
nonlocal sql
if callable(sql):
sql = sql()
sync_execute(sql, settings=settings)
def run_migration():
if node_role == NodeRole.ALL:
logger.info(" Running migration on coordinators and workers")
return cluster.map_all_hosts(lambda client: client.execute(sql)).result()
else:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: No error handling for failed migrations across nodes - could leave cluster in inconsistent state

logger.info(f" Running migration on {node_role.value.lower()}s")
return cluster.map_all_hosts(lambda client: client.execute(sql), node_role=node_role).result()

return migrations.RunPython(run_sql)
return migrations.RunPython(lambda _: run_migration())
Loading
Loading