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
Cleanup moving parts #50489
Cleanup moving parts #50489
Conversation
This is an automated comment for commit ac63861 with description of existing statuses. It's updated for the latest CI running
|
String data_part_directory = cloned_part_storage->getFullPath(); | ||
|
||
TemporaryClonedPart cloned_part; | ||
cloned_part.temporary_directory_lock = data->getTemporaryPartDirectoryHolder(data_part_directory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does/should data_part_directory
include moving/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's set temporary_directories_lifetime
to 1 in tests for background moves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does/should data_part_directory include moving/?
Yes, since we lock part in moving
directory before it's renamed.
cloned_part_storage = part->makeCloneOnDisk(disk, MergeTreeData::MOVING_DIR_NAME);
Let's set temporary_directories_lifetime to 1 in tests for background moves
Added to some integration tests, for stateless already set
<temporary_directories_lifetime>1</temporary_directories_lifetime> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holder should accept only base name of the part
if (temporary_parts.contains(basename)) |
Issue found by https://s3.amazonaws.com/clickhouse-test-reports/50489/f28c4dd58f600624a07e2c395b4114a8eb8a4272/integration_tests_flaky_check__asan_.html, fixed
String data_part_directory = cloned_part_storage->getFullPath(); | ||
|
||
TemporaryClonedPart cloned_part; | ||
cloned_part.temporary_directory_lock = data->getTemporaryPartDirectoryHolder(data_part_directory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should hold the temp dir lock until the part is renamed to persistent name (renameTo
in swapClonedPart
)
Fixed couple of things:
ClickHouse/src/Storages/MergeTree/IMergeTreeDataPart.cpp Lines 501 to 507 in 5d0522c
Adjusted check to include parts from
|
except QueryRuntimeException as e: | ||
# PART_IS_TEMPORARILY_LOCKED | ||
assert 384 == e.returncode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it fine to ignore/retry such an error?
Does this solve it? #49197 |
@CheSema yes, your description is exactly the same |
67f4357
to
bc4e95c
Compare
I added to test case with replicated merge tree and check for a content of table. Locally it passes in release/debug both with minio and aws s3. Waiting for CI results... |
bc4e95c
to
989540e
Compare
505100e
to
ac63861
Compare
I added a config parameter ( |
Cleanup moving parts (cherry picked from commit df1ea0b)
Backport #50489 to 23.3: Cleanup moving parts
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
TODO:
system.remote_data_paths
)