Feature/product datastore - #643
Open
F1r3w477 wants to merge 6 commits into
Open
Conversation
Move record maps, cache/LRU, populate-times, and lookup into ProductDatastore. Harden null-provider guards, second-aligned cache lookup, atomic cache limit, and L2 chunks time-populated check. Refs dpaulat#506
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Const-correct locks, deleted ProviderManager moves, explicit lambda captures, named cache limit constant, and const ref Store params.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Extracts product record storage/cache responsibilities from RadarProductManager into a dedicated ProductDatastore as part of the multi-provider refactor (Slice B), and moves the internal ProviderManager into its own compilation unit.
Changes:
- Added
ProductDatastorefor record storage, cache/LRU behavior, and product-time population/lookup. - Extracted
ProviderManagerfromradar_product_manager.cppinto standaloneprovider_manager.{hpp,cpp}. - Updated
RadarProductManagerto delegate storage and cache lookups toproductDatastore_and added/updated unit tests and build wiring.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test.cmake | Registers new ProductDatastore unit tests in the test build. |
| test/source/scwx/qt/manager/radar_product_manager.test.cpp | Adds ProviderManager::name() formatting test and adjusts includes. |
| test/source/scwx/qt/manager/product_datastore.test.cpp | New unit tests for datastore dedup, lookup, cache key behavior, and cache limit floor. |
| scwx-qt/source/scwx/qt/manager/radar_product_manager.cpp | Delegates storage/cache/time-population to ProductDatastore and uses extracted ProviderManager. |
| scwx-qt/source/scwx/qt/manager/provider_manager.hpp | New standalone ProviderManager header. |
| scwx-qt/source/scwx/qt/manager/provider_manager.cpp | New standalone ProviderManager implementation (refresh scheduling, shutdown hardening). |
| scwx-qt/source/scwx/qt/manager/product_datastore.hpp | New datastore public API for caching/record maps/LRU and time population. |
| scwx-qt/source/scwx/qt/manager/product_datastore.cpp | Implements record storage/dedup, cache lookups, time population, and iteration helpers. |
| scwx-qt/scwx-qt.cmake | Adds new manager source/header files to the Qt build. |
Avoid relying on transitive includes for std::max/std::find.
Align dpaulat#643 with review standards from dpaulat#642/dpaulat#644: hide implementation details behind Impl, use ProviderManager accessors, fix RPM include order, and add QT_NO_EMIT for wxtest on Linux.
Keep ProductDatastore delegation in RPM; integrate RadarCoordinateTable and merged ProviderManager from develop. Drop duplicate cache/populate logic left in develop's RPM.
F1r3w477
marked this pull request as ready for review
June 24, 2026 20:38
Sync multi-provider / radar-site transition work from develop into ProductDatastore (AreProductTimesPopulated, PopulateProductTimes) while keeping RPM cache/load delegation through ProductDatastore.
Owner
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a thread-safe ChangesProduct datastore integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RadarProductManager
participant ProductDatastore
participant ProviderManager
RadarProductManager->>ProductDatastore: Populate product times
ProductDatastore->>ProviderManager: Query provider volume times
ProviderManager-->>ProductDatastore: Return volume time points
RadarProductManager->>ProductDatastore: Find cached record
ProductDatastore-->>RadarProductManager: Return record entry or NexradFile
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scwx-qt/source/scwx/qt/manager/radar_product_manager.cpp (1)
730-742: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCopy the provider manager before releasing the map lock.
level3ProviderManageris an iterator used after unlocking. A concurrent insertion can rehash the map, invalidating it and causing undefined behavior.Proposed fix
- std::shared_lock providerManagerLock(p->level3ProviderManagerMutex_); - auto level3ProviderManager = p->level3ProviderManagerMap_.find(product); - if (level3ProviderManager == p->level3ProviderManagerMap_.cend()) { - logger_->debug("No level 3 provider manager for product: {}", product); - return; + std::shared_lock lock {p->level3ProviderManagerMutex_}; + const auto it = p->level3ProviderManagerMap_.find(product); + if (it == p->level3ProviderManagerMap_.cend()) + { + logger_->debug("No level 3 provider manager for product: {}", product); + return; + } + level3ProviderManager = it->second; } - providerManagerLock.unlock(); p->LoadProviderData(time, - level3ProviderManager->second, + level3ProviderManager, product,Declare
std::shared_ptr<ProviderManager> level3ProviderManager;before the locked scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scwx-qt/source/scwx/qt/manager/radar_product_manager.cpp` around lines 730 - 742, Copy the provider manager shared pointer from the map while level3ProviderManagerMutex_ is held, then release the lock before calling LoadProviderData. Replace the post-unlock iterator use with the copied shared_ptr, preserving the existing missing-product return path.
🧹 Nitpick comments (1)
scwx-qt/source/scwx/qt/manager/product_datastore.cpp (1)
302-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the recent-records update logic.
The current logic using the
iteratorErasedflag and multiple conditions is functionally correct but convoluted. It can be simplified by directly checking if the record needs to be moved to the front.💡 Proposed refactor
void UpdateRecentRecords(RadarProductRecordList& recentList, const std::shared_ptr<types::RadarProductRecord>& record) { const std::size_t recentListMaxSize {cacheLimit_.load()}; - bool iteratorErased = false; auto it = std::find(recentList.cbegin(), recentList.cend(), record); - if (it != recentList.cbegin() && it != recentList.cend()) - { - recentList.erase(it); - iteratorErased = true; - } - - if (iteratorErased || recentList.size() == 0 || it != recentList.cbegin()) + if (recentList.empty() || it != recentList.cbegin()) { + if (it != recentList.cend()) + { + recentList.erase(it); + } recentList.push_front(record); } while (recentList.size() > recentListMaxSize)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scwx-qt/source/scwx/qt/manager/product_datastore.cpp` around lines 302 - 325, Refactor UpdateRecentRecords to remove the iteratorErased flag and consolidate the conditions controlling whether the record is moved to the front. Directly determine from the find result whether an existing non-front record should be erased and reinserted, while preserving the behavior for a front record, an absent record, and the cache-size trimming loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scwx-qt/source/scwx/qt/manager/product_datastore.cpp`:
- Around line 444-449: Update the volume-time population logic in Store() and
GetCachedNexradFile() to truncate each provider-supplied volumeTime to
std::chrono::seconds before using it as a cache key or inserting the
placeholder. Preserve the existing weak_ptr placeholder behavior while ensuring
these keys match the second-aligned keys produced by the existing cache paths.
In `@test/source/scwx/qt/manager/product_datastore.test.cpp`:
- Around line 9-11: In test/source/scwx/qt/manager/product_datastore.test.cpp at
lines 9-11, replace the C++17 nested namespace declaration with traditional
nested namespace blocks for scwx, qt, and manager; at lines 180-181, close them
with separate comments for manager, qt, and scwx.
---
Outside diff comments:
In `@scwx-qt/source/scwx/qt/manager/radar_product_manager.cpp`:
- Around line 730-742: Copy the provider manager shared pointer from the map
while level3ProviderManagerMutex_ is held, then release the lock before calling
LoadProviderData. Replace the post-unlock iterator use with the copied
shared_ptr, preserving the existing missing-product return path.
---
Nitpick comments:
In `@scwx-qt/source/scwx/qt/manager/product_datastore.cpp`:
- Around line 302-325: Refactor UpdateRecentRecords to remove the iteratorErased
flag and consolidate the conditions controlling whether the record is moved to
the front. Directly determine from the find result whether an existing non-front
record should be erased and reinserted, while preserving the behavior for a
front record, an absent record, and the cache-size trimming loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97e95821-c0ef-4c78-beb0-4a8e0fd9dfd4
📒 Files selected for processing (6)
scwx-qt/scwx-qt.cmakescwx-qt/source/scwx/qt/manager/product_datastore.cppscwx-qt/source/scwx/qt/manager/product_datastore.hppscwx-qt/source/scwx/qt/manager/radar_product_manager.cpptest/source/scwx/qt/manager/product_datastore.test.cpptest/test.cmake
Comment on lines
+444
to
+449
| [](const std::chrono::system_clock::time_point& volumeTime) | ||
| { | ||
| return std::pair<std::chrono::system_clock::time_point, | ||
| std::weak_ptr<types::RadarProductRecord>>( | ||
| volumeTime, std::weak_ptr<types::RadarProductRecord> {}); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Ensure populated volume times are aligned to seconds.
In Store() and GetCachedNexradFile(), time keys are explicitly truncated to std::chrono::seconds. However, the volumeTime populated here is inserted without truncation. If the provider returns a time_point with sub-second precision, it won't match the keys generated by Store(), defeating the placeholder mechanism and resulting in duplicate cache entries.
Align the volumeTime to seconds before insertion to guarantee consistent cache keys.
🛡️ Proposed fix
- [](const std::chrono::system_clock::time_point& volumeTime)
+ [](const std::chrono::system_clock::time_point& volumeTime)
{
+ const auto volumeTimeInSeconds =
+ std::chrono::time_point_cast<std::chrono::seconds,
+ std::chrono::system_clock>(
+ volumeTime);
+
return std::pair<std::chrono::system_clock::time_point,
std::weak_ptr<types::RadarProductRecord>>(
- volumeTime, std::weak_ptr<types::RadarProductRecord> {});
+ volumeTimeInSeconds, std::weak_ptr<types::RadarProductRecord> {});
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scwx-qt/source/scwx/qt/manager/product_datastore.cpp` around lines 444 - 449,
Update the volume-time population logic in Store() and GetCachedNexradFile() to
truncate each provider-supplied volumeTime to std::chrono::seconds before using
it as a cache key or inserting the placeholder. Preserve the existing weak_ptr
placeholder behavior while ensuring these keys match the second-aligned keys
produced by the existing cache paths.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Slice B of the multi-provider refactor (#506): extract
ProductDatastorefromRadarProductManager.Record storage, cache/LRU eviction, product-time population, and cache lookup now live in a dedicated class.
RadarProductManagerkeeps orchestration — providers, refresh, coordinate generation, and the L2/L3 load paths — and delegates storage toproductDatastore_.Stacked on #642 (
feature/extract-provider-manager/ Slice A).What moved
ProductDatastore— L2/L3 record maps, recent-record LRU,Store,GetCachedNexradFile,FindLevel2RecordEntries/FindLevel3RecordEntry,PopulateLevel2/3ProductTimes,AreProductTimesPopulatedStoreRadarProductRecord,UpdateRecentRecords, local typedefs)provider_before provider callsGetCachedNexradFilekeys aligned to seconds (matchesStore)cacheLimit_for concurrentSetCacheLimitAreLevel2ProductTimesPopulatedchecks archive and chunks providersTest plan
ProductDatastore.*unit tests (7) — dedup, find, cache lookup, sub-second cache key, cache limit floorRadarProductManager/ProviderManagertests passNotes