Skip to content

test: Add test that simulates multiple concurrent syncs #20907

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

erezrokah
Copy link
Member

Summary

This PR adds a test for an issue that's more evident on older ClickHouse versions (e.g. 22 that we don't support anymore) where max_concurrent_queries defaulted to 100.

The result of hitting the limit is that some tables can be dropped unexpectedly, since checkPartitionOrOrderByChanged called from checkForced here

if err := c.checkForced(ctx, have, want, messages); err != nil {
will not return an error while checkPartitionOrOrderByChanged can return an error when called further down the line here
if unsafe := unsafeChanges(changes); len(unsafe) > 0 || c.checkPartitionOrOrderByChanged(ctx, want, c.spec.Partition, c.spec.OrderBy) != nil {

Opening for reference. Will add a fix for the issue in this PR as well.
Had to override the max_concurrent_queries configuration so using the docker compose file for that

@cq-bot
Copy link
Contributor

cq-bot commented Jun 24, 2025

@erezrokah erezrokah force-pushed the fix/handle_force_migration_false_positives branch 2 times, most recently from 4f17884 to 6778728 Compare June 26, 2025 17:33
@erezrokah erezrokah force-pushed the fix/handle_force_migration_false_positives branch from 6778728 to 56298c6 Compare June 30, 2025 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants