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 AsyncLoader with dependency tracking and runtime prioritization #48923

Merged
merged 69 commits into from May 14, 2023

Conversation

serxa
Copy link
Member

@serxa serxa commented Apr 19, 2023

This tool is needed to make async table loading possible in #43424.
Please, read a comment about AsyncLoader class in the source code for implementation details.

Here is how it is going to be used in the follow-up PRs. But I want to merge AsyncLoader first. The idea is to load the dependency graph created by TablesLoader into AsyncLoader as a graph of LoadJobs. There will be at least 4 types of LoadJobs:

  1. Load table (including not outdated parts).
  2. Startup table (depend on 1, has initial low priority to allow loading of all table before startups to delay the start of merges/mutations).
  3. Startup DDLWorker for ReplicatedDatabase (depends on 2 for all tables, also has low priority because depends on low-priority jobs).
  4. Startup Server job (no-op job) clickhouse-server will wait on this job before starting servers (depends on 2 for all tables that should be loaded before server start.

On incoming queries, we will wait for every involved table to be started (job type 2). Important: to speed up the loading of these tables, we will prioritize all corresponding jobs (with their dependencies) using AsyncLoader::prioritize(job, new_priority).

cc @alesapin @tavplubix

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@serxa
Copy link
Member Author

serxa commented May 2, 2023

Stateless tests (asan) [2/4] — fail: 1
00814_replicated_minimalistic_part_header_zookeeper - #43983

@serxa
Copy link
Member Author

serxa commented May 2, 2023

Unit tests (msan) — Exit on signal. PASSED record not found.

msan found a problem with JobFailure unittest: https://pastila.nl/?0060f2ab/915507a84a7d00c58c8f4f12bf83efc6

Reproduced. Again, I'm not sure what is the reason. But now we can see there is access to uninitialized variable

UPD. should be fixed by #49316

src/Common/AsyncLoader.cpp Outdated Show resolved Hide resolved
src/Common/AsyncLoader.h Outdated Show resolved Hide resolved
src/Common/AsyncLoader.h Outdated Show resolved Hide resolved
src/Common/AsyncLoader.h Outdated Show resolved Hide resolved
src/Common/AsyncLoader.cpp Show resolved Hide resolved
serxa and others added 7 commits May 3, 2023 15:41
Co-authored-by: Antonio Andelic <antonio2368@users.noreply.github.com>
Co-authored-by: Antonio Andelic <antonio2368@users.noreply.github.com>
Co-authored-by: Antonio Andelic <antonio2368@users.noreply.github.com>
Co-authored-by: Antonio Andelic <antonio2368@users.noreply.github.com>
@serxa
Copy link
Member Author

serxa commented May 11, 2023

@rschu1ze Could you please take a look at this https://s3.amazonaws.com/clickhouse-test-reports/48923/1263c58f648dd8c3b111f82b2e50123c40789719/unit_tests__msan_/run.log

I thought it should be fixed by #49316. But it is not, for some reason. Or the "Update branch" button is broken.

@rschu1ze
Copy link
Member

@serxa let me try to reproduce locally ...

@rschu1ze
Copy link
Member

rschu1ze commented May 11, 2023

Reproduces easily, checking ...

@rschu1ze
Copy link
Member

@serxa Unlike first suspected, the failure is definitely unrelated to #49316. And I am pretty sure it is a false alarm. I tried to suppress it yesterday with __msan_unpoison (in demangle.cpp) and -fsanitize-blacklist but without success yet. I will need to check phdr_cache.cpp closer.

To un-block this PR, feel free to disable the failing test for now(TEST(AsyncLoader, DISABLEDJobFailure)), I'll follow up in a separate PR.

@serxa serxa merged commit 8f20085 into master May 14, 2023
257 of 258 checks passed
@serxa serxa deleted the async-loader branch May 14, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-not-for-changelog This PR should not be mentioned in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants