chore(core): fix metadata cache trying to load view metadata on startup#6613
chore(core): fix metadata cache trying to load view metadata on startup#6613bluestreak01 merged 7 commits intomasterfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce background view compilation during server startup via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/io/questdb/cairo/view/ViewStateStoreImpl.java (1)
44-47: Potential memory concern with unbounded queue.The unlimited queue prevents dropped notifications, but could theoretically grow unbounded if schema changes vastly outpace the view compiler's consumption rate. This is likely acceptable given the documented expectation of low notification volume, but consider adding queue size monitoring/metrics for observability in production.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/io/questdb/ServerMain.javacore/src/main/java/io/questdb/cairo/CairoEngine.javacore/src/main/java/io/questdb/cairo/MetadataCache.javacore/src/main/java/io/questdb/cairo/view/ViewCompilerJob.javacore/src/main/java/io/questdb/cairo/view/ViewStateStoreImpl.javacore/src/test/java/io/questdb/test/cairo/view/ViewBootstrapTest.java
💤 Files with no reviewable changes (1)
- core/src/main/java/io/questdb/cairo/CairoEngine.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (36)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (Hosted Running tests on windows-other-2)
- GitHub Check: New pull request (Hosted Running tests on windows-other-1)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
core/src/main/java/io/questdb/cairo/MetadataCache.java (1)
154-158: LGTM!The early return for view tokens correctly prevents attempting to read non-existent
_metafiles. The placement after theisTableDroppedcheck is appropriate, and the comment clearly explains the rationale.core/src/test/java/io/questdb/test/cairo/view/ViewBootstrapTest.java (1)
614-616: LGTM!Adding
awaitStartup()ensures the test waits for both metadata hydration and view compilation background threads to complete before proceeding with assertions, preventing potential race conditions.core/src/main/java/io/questdb/cairo/view/ViewCompilerJob.java (1)
87-111: LGTM!The
compileAllViews()method correctly follows the established pattern fromMetadataCache.onStartupAsyncHydrator(). The exception handling appropriately catchesCairoExceptionfrom token retrieval/iteration, whilecompileViewhandles its own exceptions internally. Thefinallyblock properly clears thread-local state.core/src/main/java/io/questdb/ServerMain.java (3)
468-480: Consider naming threads and verify startup ordering.Two observations:
Thread naming: Both background threads lack names, which makes debugging/profiling harder. Consider
new Thread(() -> {...}, "questdb-hydrate-metadata")pattern.Potential race condition:
hydrateMetadataThreadandcompileViewsThreadrun concurrently. IfcompileAllViews()attempts to compile a view before its underlying table's metadata is hydrated, could this cause issues? The view compilation may depend on table metadata fromMetadataCache.Suggested improvement for thread naming
// metadata and write tracker hydration - hydrateMetadataThread = new Thread(() -> { + hydrateMetadataThread = new Thread(() -> { engine.getMetadataCache().onStartupAsyncHydrator(); engine.hydrateRecentWriteTracker(); - }); + }, "questdb-hydrate-metadata"); hydrateMetadataThread.start(); // populate view state store and hydrate metadata cache with view metadata - compileViewsThread = new Thread(() -> { + compileViewsThread = new Thread(() -> { try (ViewCompilerJob viewCompiler = new ViewCompilerJob(-1, engine, 0)) { viewCompiler.compileAllViews(); } - }); + }, "questdb-compile-views"); compileViewsThread.start();
486-496: LGTM on the joinThread helper.Clean utility method with proper interrupt handling. The
ignoreInterruptparameter appropriately distinguishes between awaiting startup (where interrupts should be propagated) and shutdown (where they should be suppressed).
474-478:sharedQueryWorkerCount=0disables all view compilation parallelization.The
workerId=-1is a documented pattern used throughout the codebase to indicate an owner thread not assigned to the worker pool, making it valid for this context. However,sharedQueryWorkerCount=0has a concrete impact: it disables all query parallelization flags (parallelFilterEnabled,parallelGroupByEnabled,parallelTopKEnabled,parallelWindowJoinEnabled,parallelReadParquetEnabled) in the SQL execution context. This means views are compiled serially without worker-based parallelism, which may be intentional for startup but should be documented if this is a constraint.
[PR Coverage check]😍 pass : 111 / 126 (88.10%) file detail
|
2 participants
Summary
_metafile (views don't have one)Changes
hydrateTableStartup()since views don't have_metafilescompileViewsThreadto compile all views at startup, which initializes view state and hydrates metadata cachecompileAllViews()methodenqueueCompile()call frombuildViewGraphs()- compilation now happens in dedicated startup threadviewCompilerWorkerCountfrom 1 to 2Test plan
ViewBootstrapTestupdated to callawaitStartup()