Skip to content

Add func tests run with s3 and fix several bugs#34215

Merged
alesapin merged 60 commits intomasterfrom
revert-34211-revert-34153-add_func_tests_over_s3
Feb 15, 2022
Merged

Add func tests run with s3 and fix several bugs#34215
alesapin merged 60 commits intomasterfrom
revert-34211-revert-34153-add_func_tests_over_s3

Conversation

@alesapin
Copy link
Copy Markdown
Member

@alesapin alesapin commented Feb 1, 2022

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Feb 1, 2022
@alesapin alesapin changed the title Add func tests run with s3 [WIP] Add func tests run with s3 Feb 1, 2022
@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 2, 2022

If it will work, need to add a lot of comments.

alesapin added 17 commits February 2, 2022 20:36
@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 4, 2022

Reverted #34219 in this PR.

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 10, 2022

Need to implement proper fix.

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 10, 2022

Strategy picker fundamentally bad idea. Better to implement exclusive locks for merges and mutations.

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 13, 2022

One known bug left: lock part during fetch with ephemeral lock.

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 14, 2022

Also need fix for sparse with wide parts -- separate PR.

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 14, 2022

I hope I've fixed everything. So one more run with all sanitizers and run with a single check.

@KochetovNicolai KochetovNicolai force-pushed the revert-34211-revert-34153-add_func_tests_over_s3 branch from ac778a6 to b75d551 Compare February 14, 2022 13:08
amosbird
amosbird reviewed Feb 14, 2022
View reviewed changes
src/Disks/IDiskRemote.cpp
}
}

/// Load metadata by path or create empty if `create` flag is set.
Copy link
Copy Markdown
Collaborator

@amosbird amosbird Feb 14, 2022

Choose a reason for hiding this comment

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

There is no create flag anymore

Copy link
Copy Markdown
Member Author

@alesapin alesapin Feb 14, 2022

Choose a reason for hiding this comment

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

Yes, what do you thing about using local fs hardlink count? Why do you think it is bad idea?

Copy link
Copy Markdown
Collaborator

@amosbird amosbird Feb 14, 2022

Choose a reason for hiding this comment

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

I don't fully understand the purpose of userland hardlink tracking. However, if we somehow need to do it properly, we will have to synchronize every file operation which can change the hardlink property. If the mutex is per-disk, it will be too heavy, since some common mutation like DROP COLUMN can generate tons of hardlinks. I don't know how good flock is, but it smells bad.

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 14, 2022

No, one more race during fetch...

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 15, 2022

00502_custom_partitioning_local -- flaky test, unrelated to S3.

I'll try to add comments.

@alesapin alesapin changed the title [WIP] Add func tests run with s3 and fix several bugs Add func tests run with s3 and fix several bugs Feb 15, 2022
@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Feb 15, 2022 •
edited
Loading

Tests should be Ok, but we have to fix non-user-visible in the next PRs:

  1. Get rid of userspace hardlink tracking. Currently they can lead to garbage in zookeeper.
  2. Better lock for merges and mutations.
  3. Stress tests with S3 🤞

@kssenii kssenii self-assigned this Feb 15, 2022
@alesapin alesapin merged commit bc2d0ee into master Feb 15, 2022
@alesapin alesapin deleted the revert-34211-revert-34153-add_func_tests_over_s3 branch February 15, 2022 16:07
@azat azat mentioned this pull request Feb 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kssenii kssenii kssenii approved these changes

+1 more reviewer

@amosbird amosbird amosbird left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kssenii kssenii

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alesapin @amosbird @kssenii @robot-clickhouse @KochetovNicolai