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

Add tests for ClickHouse schema upgrades #5932

Merged
merged 2 commits into from
Jun 22, 2024

Conversation

bnaecker
Copy link
Collaborator

  • Adds a check that verifies the SQL contained in each upgrade. This does the basic sanity checks like: one statement per file, no data modifications, but doesn't actually apply the upgrades.
  • Adds another test that ensures that the set of tables we arrive at either by upgrading or directly initializing the latest database version are the same.

- Adds a check that verifies the SQL contained in each upgrade. This
  does the basic sanity checks like: one statement per file, no data
  modifications, but doesn't actually apply the upgrades.
- Adds another test that ensures that the set of tables we arrive at
  either by upgrading or directly initializing the latest database
  version are the same.
@bnaecker bnaecker requested a review from zeeshanlakhani June 22, 2024 00:19
oximeter/db/src/client/mod.rs Show resolved Hide resolved
oximeter/db/src/client/mod.rs Show resolved Hide resolved
@bnaecker bnaecker enabled auto-merge (squash) June 22, 2024 00:48
@bnaecker bnaecker merged commit 32a37f5 into main Jun 22, 2024
19 checks passed
@bnaecker bnaecker deleted the add-clickhouse-schema-upgrade-tests branch June 22, 2024 01:51
const FIRST_VERSION: u64 = 3;
for version in FIRST_VERSION..=OXIMETER_VERSION {
let upgrade_file_contents = Client::read_schema_upgrade_sql_files(
log, false, version, SCHEMA_DIR,
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears we're only testing single node and not replicated here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Derp (on me). Good find. I set false to replicated in my branch: #5897, which runs a new series of migrations (and why @bnaecker added this), and things are passing correctly. That PR is almost ready for merge, so, I'll ensure this test fn is updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants