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

Migration to remove scan field values triggers for DBs that turn it off #41348

Merged
merged 25 commits into from
Apr 16, 2024

Conversation

qnkhuat
Copy link
Contributor

@qnkhuat qnkhuat commented Apr 12, 2024

Second part to fix #40715

This adds a migration that will scan field values trigger for existing DBs that have advanced scan options are either "Only when adding a new filter" or "Never, I'll do this manually if I need to".

@qnkhuat qnkhuat requested a review from camsaul as a code owner April 12, 2024 09:56
@metabase-bot metabase-bot bot added the .Team/BackendComponents also known as BEC label Apr 12, 2024
Copy link

replay-io bot commented Apr 12, 2024

Status Complete ↗︎
Commit eb7a21e
Results
⚠️ 9 Flaky
2400 Passed

Copy link
Contributor

@crisptrutski crisptrutski left a comment

Choose a reason for hiding this comment

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

Just a bunch of non-functional fixes and suggestions, looks good 😎

resources/migrations/001_update_migrations.yaml Outdated Show resolved Hide resolved
src/metabase/db/custom_migrations.clj Show resolved Hide resolved
Comment on lines 1083 to 1084
(qs/delete-trigger scheduler (triggers/key (format "metabase.task.update-field-values.trigger.%d" (:id db)))))
(t2/update! :model/Database :id [:in (map :id dbs-without-scan-field-values)] {:cache_field_values_schedule nil})
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we only ever use the ids from the database, so we might as well not select any other fields, and immediately map to just these values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we also need details, is_full_sync, is_on_demand for the filter.

src/metabase/db/custom_migrations.clj Outdated Show resolved Hide resolved
test/metabase/db/custom_migrations_test.clj Outdated Show resolved Hide resolved
@crisptrutski
Copy link
Contributor

crisptrutski commented Apr 12, 2024

This seems to have broken metabase.api.field-test/get-field-test, but only for a few drivers 🤷

@qnkhuat
Copy link
Contributor Author

qnkhuat commented Apr 15, 2024

This seems to have broken metabase.api.field-test/get-field-test, but only for a few drivers 🤷

yea it was a flake, which I fixed in an upstream commit
92b7e36

Base automatically changed from make-scan-fv-optional to master April 15, 2024 14:20
@qnkhuat qnkhuat enabled auto-merge (squash) April 16, 2024 04:11
@qnkhuat qnkhuat added the no-backport Do not backport this PR to any branch label Apr 16, 2024
@qnkhuat
Copy link
Contributor Author

qnkhuat commented Apr 16, 2024

No backport because I'll squash backporting this with #41170 in #41415

@qnkhuat qnkhuat merged commit 9b8df36 into master Apr 16, 2024
127 of 129 checks passed
@qnkhuat qnkhuat deleted the migration-to-remove-scan-field-values-triggers branch April 16, 2024 04:53
Copy link

@qnkhuat Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

qnkhuat added a commit that referenced this pull request Apr 16, 2024
* Fix unable to disable scan field values (#41170)

* Migration to remove scan field values triggers for DBs that turn it off (#41348)

---------

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/BackendComponents also known as BEC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Advanced option to turn off syncing field values doesn't work
2 participants