env variable for cache disable via WAVEFRONT_CACHE_ENABLED#245
env variable for cache disable via WAVEFRONT_CACHE_ENABLED#245rootflo-hardik wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a WAVEFRONT_CACHE_ENABLED flag to short-circuit cache methods and avoid Redis operations when disabled, and guards pubsub subscription in channels.py to return early if pubsub is unavailable. ChangesCache Manager Feature Flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py (1)
35-37: ValidateWAVEFRONT_CACHE_ENABLEDinstead of treating every unknown value asfalse.A typo like
tureor a whitespace-padded value will silently disable Redis and the listener path. It is safer to normalize and reject invalid inputs explicitly.Proposed change
- self.cache_enabled = ( - os.getenv('WAVEFRONT_CACHE_ENABLED', 'true').lower() == 'true' - ) + raw_cache_enabled = os.getenv( + 'WAVEFRONT_CACHE_ENABLED', 'true' + ).strip().lower() + if raw_cache_enabled not in {'true', 'false'}: + raise ValueError( + 'WAVEFRONT_CACHE_ENABLED must be either "true" or "false"' + ) + self.cache_enabled = raw_cache_enabled == 'true'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py` around lines 35 - 37, The current assignment to cache_enabled silently treats any non-exact 'true' as false; change it to normalize and validate the environment value by calling os.getenv('WAVEFRONT_CACHE_ENABLED', 'true').strip().lower(), then if the normalized value is 'true' or 'false' set self.cache_enabled = (value == 'true'), otherwise log an explicit warning or raise a ValueError indicating an invalid WAVEFRONT_CACHE_ENABLED value so typos like 'ture' or whitespace-padded strings are not silently accepted; update the code that sets cache_enabled to perform this validation and include the variable name cache_enabled in the log/error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`:
- Around line 200-202: The subscribe() and publish() docstrings currently omit
the behavior when cache_enabled is False (publish returning 0 and subscribe
returning None), so update both docstrings and accompanying type hints to
document and reflect this contract: in cache_manager.py, amend the publish()
docstring to state that when self.cache_enabled is False it is a no-op and
returns 0, and amend the subscribe() docstring and signature to indicate it may
return None (Optional[PubSub]) when cache is disabled; reference the
cache_enabled flag, REDIS_API_SERVICE_UPDATES_CHANNEL, and the
subscribe()/publish() methods so callers (e.g., floware/channels.py) know to
check for None or a 0 result.
---
Nitpick comments:
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`:
- Around line 35-37: The current assignment to cache_enabled silently treats any
non-exact 'true' as false; change it to normalize and validate the environment
value by calling os.getenv('WAVEFRONT_CACHE_ENABLED', 'true').strip().lower(),
then if the normalized value is 'true' or 'false' set self.cache_enabled =
(value == 'true'), otherwise log an explicit warning or raise a ValueError
indicating an invalid WAVEFRONT_CACHE_ENABLED value so typos like 'ture' or
whitespace-padded strings are not silently accepted; update the code that sets
cache_enabled to perform this validation and include the variable name
cache_enabled in the log/error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8f63864-056f-4466-a02e-11ee029d2080
📒 Files selected for processing (2)
wavefront/server/apps/floware/floware/channels.pywavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py
| if not self.cache_enabled: | ||
| return 0 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== CacheManager publish/subscribe call sites ==="
rg -n -C3 '\bcache_manager\.(publish|subscribe)\s*\(' wavefront
echo
echo "=== Redis API update channel usage ==="
rg -n -C3 'REDIS_API_SERVICE_UPDATES_CHANNEL|process_message\s*\(' wavefront
echo
echo "=== CommonCache / subscribe contract ==="
rg -n -C3 '\bclass CommonCache\b|\bdef subscribe\s*\(' wavefront
echo
echo "=== Publisher / processor sources ==="
fd 'api_change_publisher\.py$' wavefront -x sed -n '1,220p' {}
fd 'api_change_processor\.py$' wavefront -x sed -n '1,260p' {}Repository: rootflo/wavefront
Length of output: 11393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full publish and subscribe methods ==="
sed -n '195,250p' wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py
echo
echo "=== Check return type hints for subscribe method ==="
rg -n 'def subscribe' wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py -A 50 | head -60
echo
echo "=== Any other usages of cache_manager.publish/subscribe beyond API updates ==="
rg -n '\.publish\(|\.subscribe\(' wavefront --type py | grep -v 'test' | grep -v '.pyc'Repository: rootflo/wavefront
Length of output: 5441
Update subscribe() and publish() docstrings to document behavior when cache is disabled.
The cache_enabled flag at lines 200–202 and 211–237 disables more than key/value caching. The REDIS_API_SERVICE_UPDATES_CHANNEL carries API service updates (create/update/delete operations that reload/remove services in the proxy router), which is critical infrastructure—not cache invalidation.
publish()becomes a no-op (returns 0) when cache is off, silencing service updates across the system.subscribe()returnsNoneinstead of a PubSub object, but the docstring only documents the normal case and omits this behavior. Callers must check forNoneexplicitly (as floware/channels.py line 22 does).
Either document the optional None return in both methods' docstrings, or reconsider whether disabling pub/sub is the right behavior when cache is disabled. If the current design is intentional, make the contract explicit in the type hints and docstrings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@wavefront/server/modules/db_repo_module/db_repo_module/cache/cache_manager.py`
around lines 200 - 202, The subscribe() and publish() docstrings currently omit
the behavior when cache_enabled is False (publish returning 0 and subscribe
returning None), so update both docstrings and accompanying type hints to
document and reflect this contract: in cache_manager.py, amend the publish()
docstring to state that when self.cache_enabled is False it is a no-op and
returns 0, and amend the subscribe() docstring and signature to indicate it may
return None (Optional[PubSub]) when cache is disabled; reference the
cache_enabled flag, REDIS_API_SERVICE_UPDATES_CHANNEL, and the
subscribe()/publish() methods so callers (e.g., floware/channels.py) know to
check for None or a 0 result.
There was a problem hiding this comment.
@rootflo-hardik Lets not disable publish and subscribe
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if not self.cache_enabled: | ||
| return 0 | ||
|
|
There was a problem hiding this comment.
@rootflo-hardik Lets not disable publish and subscribe
| socket_timeout=socket_timeout, | ||
| socket_keepalive=socket_keepalive, | ||
| pool_size=pool_size, | ||
| self.cache_enabled = ( |
There was a problem hiding this comment.
DId you test this? Is it working fine with cache disabled ?
2 participants
Summary by CodeRabbit
New Features
Bug Fixes