Adding secondary cache to settings #1156
Conversation
Caching get_setting_value independent from what backend is used.
WalkthroughIntroduces a secondary in-memory cache for settings and updates get_setting and get_setting_value to use a two-tier caching strategy. Enhances file-change invalidation to clear both caches. Refactors JSON loading with with-open and explicit error handling, updating the last cache date only after successful parsing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Helper as helper.get_setting_value
participant SecCache as Secondary Cache
participant Conf as conf.mySettings
participant Prim as helper.get_setting
participant FS as table_settings.json
Caller->>Helper: get_setting_value(key)
Helper->>SecCache: Lookup key
alt Hit in secondary cache
SecCache-->>Helper: value
Helper-->>Caller: value
else Miss in secondary cache
Helper->>Conf: Read mySettings[key]
alt Found in conf
Conf-->>Helper: value
Helper->>SecCache: Store key=value
Helper-->>Caller: value
else Not in conf
Helper->>Prim: get_setting(key)
Prim->>FS: Check mtime / read JSON
alt File changed
Prim->>SecCache: Clear all
Prim->>Prim: Clear primary cache
end
Prim->>FS: Parse JSON (with-open)
alt Parse success
Prim->>Prim: Update primary cache + last cache date
Prim-->>Helper: value or None
Helper->>SecCache: Store key=value
Helper-->>Caller: value
else Parse error
Prim-->>Helper: None
Helper-->>Caller: None
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/helper.py (1)
331-349: Do not preemptively wipe caches; use transactional reload + in-place update.Reassigning to {} drops existing objects (breaking external refs) and clearing before a successful parse causes avoidable outages on transient JSON errors. Load into a temporary map, then replace contents atomically; fall back to old cache on errors.
Apply:
- # invalidate CACHE - SETTINGS_CACHE = {} - SETTINGS_SECONDARYCACHE={} - - # Load JSON and populate cache - try: - with open(settingsFile, 'r') as json_file: - data = json.load(json_file) - SETTINGS_CACHE = {item["setKey"]: item for item in data.get("data", [])} - except json.JSONDecodeError: - mylog('none', [f'[Settings] ⚠ JSON decode error in file {settingsFile}']) - return None - except ValueError as e: - mylog('none', [f'[Settings] ⚠ Value error: {e} in file {settingsFile}']) - return None - - # Only update file date when we successfully parsed the file - SETTINGS_LASTCACHEDATE = fileModifiedTime + # Load JSON and populate cache (transactional) + try: + with open(settingsFile, 'r') as json_file: + data = json.load(json_file) + new_cache = {item["setKey"]: item for item in data.get("data", [])} + except json.JSONDecodeError: + mylog('none', [f'[Settings] ⚠ JSON decode error in file {settingsFile}']) + # Keep serving from the previous cache instead of dropping values + return SETTINGS_CACHE.get(key) + except ValueError as e: + mylog('none', [f'[Settings] ⚠ Value error: {e} in file {settingsFile}']) + return SETTINGS_CACHE.get(key) + + # Only update global caches after successful parse (in-place to preserve references) + SETTINGS_CACHE.clear() + SETTINGS_CACHE.update(new_cache) + SETTINGS_SECONDARYCACHE.clear() + SETTINGS_LASTCACHEDATE = fileModifiedTimeOptional concurrency hardening (outside this hunk): protect this critical section with a module-level RLock if the app runs multi-threaded.
🧹 Nitpick comments (2)
server/helper.py (2)
293-294: Good addition: secondary in-memory cache introduced.Adds a fast path for get_setting_value; sensible for hot settings. Consider documenting expected key cardinality and lifetime to make it clear this is bounded by number of settings.
397-398: Typo in comment.“retrive” → “retrieve”.
- # Otherwise fall back to retrive from json + # Otherwise fall back to retrieve from json
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
server/helper.py(7 hunks)
⏰ 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). (1)
- GitHub Check: docker_dev
🔇 Additional comments (4)
server/helper.py (4)
347-349: Nice: timestamp updated only after successful parse.This fixes an edge case where a bad read could pin an invalid cache state.
394-395: Good: populate secondary cache from conf.mySettings path.Ensures hot lookups bypass tuple scanning on subsequent calls.
409-412: Good: also cache JSON-derived values.Keeps secondary cache coherent across both sources.
311-313: Cache stability concern appears unfounded: no external references to SETTINGS_CACHE/SETTINGS_SECONDARYCACHE found
Ranrg -nP -C2 '\bSETTINGS_(SECONDARY)?CACHE\b' -g '!server/helper.py'— no matches. These globals aren’t imported elsewhere, so reassigning them in server/helper.py won’t break external code. Please manually confirm there are no dynamic imports before proceeding.
|
@ingoratsdorf amazing! Seems like we've managed to lower the CPU usage by 40-50% (14% -> 7%) which is amazing. Let me know if this is ready to be merged |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Merged suggestion by CodeRabbit. |
2 participants
Caching get_setting_value independent of what backend is used.
Summary by CodeRabbit
Performance
Bug Fixes