Make CLICKHOUSE_REPLICATION work correctly #8730

macobo · 2022-02-23T09:47:13Z

Changes

This PR finally makes CLICKHOUSE_REPLICATION set up the right sharded and replicated schema on fresh installs. It also tries to replicate posting cloud as closely as possible.

See #8652 for the full ticket and details of next steps.

If you don't know what writable_events and Distributed table engines do - read up on https://posthog.com/handbook/engineering/event-ingestion

Note that there are cases where the schema here is correct but might be incorrect on cloud: see #7571

How did you test this code?

See unit tests.

Also set up a local instance with CLICKHOUSE_REPLICATION and tested it working.

yakkomajuri

one comment so far as we're pretty busy at the offsite - also this is not a small pr nor a light one so will take more time

yakkomajuri · 2022-02-24T09:35:19Z

ee/clickhouse/migrations/0016_collapsing_person_distinct_id.py

+        migrations.RunSQL(f"DROP TABLE kafka_person_distinct_id ON CLUSTER {CLICKHOUSE_CLUSTER}"),
+        migrations.RunSQL(
+            f"""
+            INSERT INTO {TEMPORARY_TABLE_NAME} (distinct_id, person_id, team_id, _sign, _timestamp, _offset)


given we're re-writing this migration, could we use ATTACH PARTITION instead?

I didn't rewrite this - just skipping it in some fresh installs where it's not needed :)

hazzadous

I'm just about to head out but wanted to get give you something at least.

Is the intention to converge on a single schema eventually? I haven't thought through what the migration path would need to look like here, but having the bifurcation in there I guess has ramifications on e.g. tests. I didn't actually look at that, do we enable CLICKHOUSE_REPLICATION in tests?

conftest.py

ee/clickhouse/migrations/0004_kafka.py

ee/clickhouse/sql/session_recording_events.py

ee/clickhouse/sql/table_engines.py

ee/clickhouse/sql/session_recording_events.py

macobo · 2022-02-24T11:37:04Z

Is the intention to converge on a single schema eventually? I haven't thought through what the migration path would need to look like here, but having the bifurcation in there I guess has ramifications on e.g. tests. I didn't actually look at that, do we enable CLICKHOUSE_REPLICATION in tests?

Yes we want to converge - though this will be handled separately from this ticket. See the master issue for full details: #8652

We can't really get rid of the "old" path until other steps in there are solved.

This brings us closer to having production and local schemas in sync

…uted

yakkomajuri

haven't tested but looks good to me overall.

a piece of feedback (particularly following the eng offsite) is that this PR could have been broken up to make it easier to review. The classes for engines and the lambdas for sql could have come before the logic changes

yakkomajuri · 2022-03-01T09:47:08Z

ee/clickhouse/sql/events.py

-    engine=table_engine(EVENTS_TABLE, "_timestamp", REPLACING_MERGE_TREE),
+    table_name=EVENTS_DATA_TABLE(),
+    cluster=settings.CLICKHOUSE_CLUSTER,
+    engine=ReplacingMergeTree("events", ver="_timestamp", replication_scheme=ReplicationScheme.SHARDED),


nit, non blocker: this was confusing to me at first - I suspected that the class would account for ClickHouse replication - and it does - but it is a bit misleading that I pass in ReplicationScheme.SHARDED but it might actually be ReplicationScheme.NOT_SHARDED.

Suggestions on how to clear this up?

Do the env check here instead of down in the class? Does move a bit of clutter elsewhere though at the benefit of being more explicit.

I wonder if there's also a more explicit name for the argument that would help clarify this.

Going to do so in a follow-up as my subsequent commits would otherwise conflict. Thanks for the ideas! :)

macobo · 2022-03-01T09:56:44Z

a piece of feedback (particularly following the eng offsite) is that this PR could have been broken up to make it easier to review. The classes for engines and the lambdas for sql could have come before the logic changes

Given I'm alone on this project, this PR has been waiting for a review for a while (due to the offsite) and this is the first (and simple) part of the to-sharded migration I don't think that would have been feasible here without sacrificing being able to get things done in the first place.

hazzadous

Looks good although there's lots going on

Some non-blocking points:

this is changing things across 27 files, which seems like a lot. I'm not sure if that's because there is a lot changing in the PR or if we should re factor the code somehow. Unsure
It's really difficult to visualise what's happening with all the tables/shards/replication. I don't know what to do there, but I guess all the docs you're adding will help 🤞
It makes me scared that we don't have the tests run in CI with CLICKHOUSE_REPLICATION set. I guess that won't be an issue once we're unified the code paths

macobo marked this pull request as ready for review February 23, 2022 12:16

macobo requested review from yakkomajuri, guidoiaquinti and hazzadous February 23, 2022 12:18

macobo changed the title ~~WIP: Make CLICKHOUSE_REPLICATION work correctly~~ Make CLICKHOUSE_REPLICATION work correctly Feb 23, 2022

macobo mentioned this pull request Feb 23, 2022

ClickHouse sharding plan #8652

Closed

yakkomajuri reviewed Feb 24, 2022

View reviewed changes

hazzadous reviewed Feb 24, 2022

View reviewed changes

macobo mentioned this pull request Feb 24, 2022

Set up sharding for clickhouse PostHog/charts-clickhouse#310

Merged

6 tasks

macobo added 18 commits February 25, 2022 08:32

Refactor table engine logic

9705cdf

This brings us closer to having production and local schemas in sync

Raise an error if running async migration but events table is distrib…

45b7e1f

…uted

Use django.conf settings

08685c5

Remove unneeded debug code

d302241

Make EVENTS_TABLE_MV_SQL lazy

76c8e6c

Make events data table name dynamic

3a49366

Make event inserts, mv table lazy

dd110ea

Refactor table engines code (again)

9182bc9

Schema for writable_events and events distributed tables

9f89a85

Session recordings sharding support in schema

983cffe

test_schema cleanup

c29d371

fixup session recordings schema

c1e97f2

Fix CLICKHOUSE_REPLICATED migrations

233b9f8

Skip ch migration 0016 on CLICKHOUSE_REPLICATED

8ba3a44

Search the right db

9ec533a

Do operations in right order for session recording events

d05a221

CREATE DATABASE before the first migration

cdec8ef

Add parens

24e8342

macobo force-pushed the refactor-schema-replicated branch from db04ceb to 24e8342 Compare February 25, 2022 06:33

macobo removed the request for review from guidoiaquinti March 1, 2022 08:04

macobo requested review from hazzadous and yakkomajuri March 1, 2022 08:04

yakkomajuri approved these changes Mar 1, 2022

View reviewed changes

hazzadous approved these changes Mar 1, 2022

View reviewed changes

macobo merged commit 401e6f5 into master Mar 1, 2022

macobo deleted the refactor-schema-replicated branch March 1, 2022 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CLICKHOUSE_REPLICATION work correctly #8730

Make CLICKHOUSE_REPLICATION work correctly #8730

macobo commented Feb 23, 2022 •

edited

Loading

yakkomajuri left a comment

yakkomajuri Feb 24, 2022

macobo Feb 24, 2022

hazzadous left a comment

macobo commented Feb 24, 2022 •

edited

Loading

yakkomajuri left a comment

yakkomajuri Mar 1, 2022

macobo Mar 1, 2022

yakkomajuri Mar 1, 2022

macobo Mar 1, 2022

macobo commented Mar 1, 2022 •

edited

Loading

hazzadous left a comment

Make CLICKHOUSE_REPLICATION work correctly #8730

Make CLICKHOUSE_REPLICATION work correctly #8730

Conversation

macobo commented Feb 23, 2022 • edited Loading

Changes

How did you test this code?

yakkomajuri left a comment

Choose a reason for hiding this comment

yakkomajuri Feb 24, 2022

Choose a reason for hiding this comment

macobo Feb 24, 2022

Choose a reason for hiding this comment

hazzadous left a comment

Choose a reason for hiding this comment

macobo commented Feb 24, 2022 • edited Loading

yakkomajuri left a comment

Choose a reason for hiding this comment

yakkomajuri Mar 1, 2022

Choose a reason for hiding this comment

macobo Mar 1, 2022

Choose a reason for hiding this comment

yakkomajuri Mar 1, 2022

Choose a reason for hiding this comment

macobo Mar 1, 2022

Choose a reason for hiding this comment

macobo commented Mar 1, 2022 • edited Loading

hazzadous left a comment

Choose a reason for hiding this comment

macobo commented Feb 23, 2022 •

edited

Loading

macobo commented Feb 24, 2022 •

edited

Loading

macobo commented Mar 1, 2022 •

edited

Loading