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

Bug: TTL not updated on settings change #103

Closed
bmtcril opened this issue Jun 3, 2024 · 1 comment
Closed

Bug: TTL not updated on settings change #103

bmtcril opened this issue Jun 3, 2024 · 1 comment
Assignees
Labels
backport PR backports a change from main to a named release. bug Report of or fix for something that isn't working as intended

Comments

@bmtcril
Copy link
Contributor

bmtcril commented Jun 3, 2024

It seems like there is a bug where changes to the TTL string do not cause the table to be altered in ClickHouse.

Repro:

  • Init ClickHouse with the default tutor-contrib-aspects plugin settings.
  • Run show create table xapi.xapi_events_all_parsed;
  • You should see a line like TTL toDateTime(emission_time) + INTERVAL 1 YEAR
  • Change the setting ASPECTS_DATA_TTL_EXPRESSION to toDateTime(emission_time) + INTERVAL 5 YEAR in your env/config.yml or local Tutor plugin
  • tutor config save
  • tutor local do dbt
  • Wait a minute or two, depending on whether you're running a local ClickHouse or Cloud (Cloud takes a minute to run the DDL on all nodes)
  • Run show create table xapi.xapi_events_all_parsed;
  • You should see no changes- it will always be 1 year no matter how long you wait

The real fix is probably somewhere in dbt-clickhouse, but we should first pursue a short-term fix in this repo.

Proposed fix:

  • Write a macro that will execute an ALTER TABLE ... MODIFY TTL if the setting is not empty or ALTER TABLE ... REMOVE TTL` if the setting is an empty string
  • Add the macro as a post_hook on all models that have the TTL setting in their config block, remove the existing TTL setting reference

Related docs:
https://clickhouse.com/docs/en/sql-reference/statements/alter/ttl#modify-ttl
https://docs.getdbt.com/reference/resource-configs/pre-hook-post-hook

@bmtcril bmtcril added bug Report of or fix for something that isn't working as intended aspects v1 backport PR backports a change from main to a named release. labels Jun 3, 2024
@saraburns1
Copy link
Contributor

This actually is working as expected - no changes needed.

  1. Table definition shows TTL 1 year
SHOW CREATE TABLE xapi.completion_events
Query id: 4f50efc4-47f1-40e5-ab52-c94b4a7318dc
1. │ CREATE TABLE xapi.completion_events
(
    `event_id` UUID,
    `emission_time` DateTime,
    `actor_id` String,
    `object_id` String,
    `course_key` LowCardinality(String),
    `org` LowCardinality(String),
    `verb_id` LowCardinality(String),
    `progress_percent` String
)
ENGINE = ReplacingMergeTree
PARTITION BY toYYYYMM(emission_time)
PRIMARY KEY (org, course_key, verb_id)
ORDER BY (org, course_key, verb_id, emission_time, actor_id, object_id, event_id)
TTL toDateTime(emission_time) + toIntervalYear(1)
SETTINGS replicated_deduplication_window = 0, index_granularity = 8192
  1. Changed TTL to 10 years in plugin.py
    image

  2. tutor config save

  3. tutor local do dbt -- only_changed defaults to True

19:02:36  Up to date!
Running dbt run
Requested to only run modified state, checking for /app/aspects/dbt_state//manifest.json
Found /app/aspects/dbt_state//manifest.json so only running modified items and their downstreams
19:02:37  Running with dbt=1.7.13
  1. Table definition shows 10 years
8b119c0e08b2 :) show create table xapi.completion_events
SHOW CREATE TABLE xapi.completion_events
Query id: 71022dcb-5160-4b03-b8de-d723be3d5309
1. │ CREATE TABLE xapi.completion_events
(
    `event_id` UUID,
    `emission_time` DateTime,
    `actor_id` String,
    `object_id` String,
    `course_key` LowCardinality(String),
    `org` LowCardinality(String),
    `verb_id` LowCardinality(String),
    `progress_percent` String
)
ENGINE = ReplacingMergeTree
PARTITION BY toYYYYMM(emission_time)
PRIMARY KEY (org, course_key, verb_id)
ORDER BY (org, course_key, verb_id, emission_time, actor_id, object_id, event_id)
TTL toDateTime(emission_time) + toIntervalYear(10)
SETTINGS replicated_deduplication_window = 0, index_granularity = 8192
1 row in set. Elapsed: 0.005 sec.
  1. Set TTL to empty string & do steps 3&4 again

  2. Table definition removes TTL setting

1. │ CREATE TABLE xapi.completion_events
(
    `event_id` UUID,
    `emission_time` DateTime,
    `actor_id` String,
    `object_id` String,
    `course_key` LowCardinality(String),
    `org` LowCardinality(String),
    `verb_id` LowCardinality(String),
    `progress_percent` String
)
ENGINE = ReplacingMergeTree
PARTITION BY toYYYYMM(emission_time)
PRIMARY KEY (org, course_key, verb_id)
ORDER BY (org, course_key, verb_id, emission_time, actor_id, object_id, event_id)
SETTINGS replicated_deduplication_window = 0, index_granularity = 8192 │

@saraburns1 saraburns1 self-assigned this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport PR backports a change from main to a named release. bug Report of or fix for something that isn't working as intended
Projects
None yet
Development

No branches or pull requests

2 participants