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

Drop support for DatabaseOrdinary in MaterializedMySQL #31292

Merged
merged 6 commits into from
Nov 22, 2021

Conversation

stigsb
Copy link
Contributor

@stigsb stigsb commented Nov 11, 2021

The default database engine has been "Atomic" since ClickHouse 20.10. To improve maintainability of the experimental MaterializedMySQL feature, this PR drops support for the "Ordinary" engine for backing MaterializedMySQL.

Also get rid of thread name logic for StorageMaterializeMySQL wrapping, use setInternalQuery instead (similar to MaterializedPostgreSQL).

Changelog category (leave one):

  • Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Drop support for using Ordinary databases with MaterializedMySQL

Detailed description / Documentation draft:

By adding documentation, you'll allow users to try your new feature immediately, not when someone else will have time to document it later. Documentation is necessary for all features that affect user experience in any way. You can add brief documentation draft above, or add documentation right into your patch as Markdown files in docs folder.

If you are doing this for the first time, it's recommended to read the lightweight Contributing to ClickHouse Documentation guide first.

Information about CI checks: https://clickhouse.tech/docs/en/development/continuous-integration/

@robot-clickhouse robot-clickhouse added the pr-build Pull request with build/testing/packaging improvement label Nov 11, 2021
@tavplubix tavplubix self-assigned this Nov 11, 2021
@stigsb stigsb force-pushed the materialized-mysql-atomic-only branch 2 times, most recently from 3068f01 to dcd444c Compare November 11, 2021 12:21
src/Databases/DatabaseFactory.cpp Show resolved Hide resolved
src/Databases/IDatabaseReplicating.h Outdated Show resolved Hide resolved
src/Databases/MySQL/MaterializedMySQLSyncThread.cpp Outdated Show resolved Hide resolved
@stigsb stigsb force-pushed the materialized-mysql-atomic-only branch 3 times, most recently from 8ffb46b to 11222fb Compare November 15, 2021 12:38
@tavplubix
Copy link
Member

Unrelated failures:
Stress test (thread) - #31423
Integration tests were broken in master

Related failures:
ClickHouse special build check
Stress tests (debug, actions) - can be reproduced with ClickHouse/tests$ ./clickhouse-test --db-engine="Replicated('/test/repl/db/123', 's1', 'r1')" 01185_create_or_replace_table

@stigsb stigsb force-pushed the materialized-mysql-atomic-only branch from 11222fb to 1011fd8 Compare November 16, 2021 14:00
@tavplubix
Copy link
Member

ClickHouse special build check and Stress test (debug, actions) failures are related to changes in the PR, @stigsb, could you please fix it?

@stigsb
Copy link
Contributor Author

stigsb commented Nov 17, 2021

Yes, I'll look at it today.

stigsb and others added 5 commits November 18, 2021 11:46
1. Dropped support for DatabaseOrdinary for MaterializeMySQL. It
   is marked as experimental, and dropping support makes the code
   more maintaible, and speeds up integration tests by 50%.

2. Get rid of thread name logic for StorageMaterializeMySQL wrapping,
   use setInternalQuery instead (similar to MaterializedPostgreSQL).
and print instructions on how to remove and recreate.
@stigsb stigsb force-pushed the materialized-mysql-atomic-only branch from 1011fd8 to b91f21d Compare November 18, 2021 10:47
@stigsb
Copy link
Contributor Author

stigsb commented Nov 22, 2021

@tavplubix looking at the remaining test failures:

"Fast test" seems to fail because the Raft port is already taken (could be a recent process exiting uncleanly?)

"Stress test (thread, actions)" has a failing test script, but I can't make out why from the logs. Any ideas @tavplubix ?

@tavplubix
Copy link
Member

Tests failures are unrelated to changes

@tavplubix tavplubix merged commit dc6d48f into ClickHouse:master Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-build Pull request with build/testing/packaging improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants