update_sliding_window#1307
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a per-entity Redis HASH combined-cache and refactors sliding-window reads to prefer HGETALL with lock-protected maintenance that backfills missing fields from per-period STRING keys; increments/decrements invalidate affected HASH fields; deletion removes only the combined HASH. ChangesSliding-Window Combined-Cache Read Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 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 `@lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs`:
- Around line 474-477: The code builds missing0/stale0 and triggers maintenance
when Redis.get oldKey returns Nothing but never memoizes absent buckets, causing
repeated fallback reads; modify the maintenance path (the logic around missing0,
stale0, needsMaintenance and the Redis.get oldKey handling) to write a
placeholder/empty field into the combined hash for bucket IDs whose Redis.get
returned Nothing so they are recorded as present (e.g., store a sentinel or zero
entry) and avoid re-entering maintenance repeatedly; ensure the write uses the
same combined hash key and the same bucket id keys used elsewhere so subsequent
reads see the memoized absence and skip repeated fallback work.
- Line 480: The call to Redis.withWaitAndLockRedis lockKey 10 50 uses a 50µs
retry delay which effectively busy-loops under contention; update the third
argument to a much larger microsecond value (e.g. 50_000 for 50ms) or replace
the magic literal with a named constant like retryDelayMicros and use a sensible
default, keeping the call site (withWaitAndLockRedis lockKey 10 <newDelay>) and
the function name as the reference to locate and change the value.
🪄 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
Run ID: a7be70e6-b419-49d4-9d85-36ed6d555979
📒 Files selected for processing (1)
lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs
Hari-Krishna-Dupaguntla
force-pushed
the
update_sliding_window
branch
2 times, most recently
from
95de565 to
fad1656
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs`:
- Around line 418-443: The new ToJSON a constraint was added to the exported
functions getCurrentWindowValuesUptoLast and getCurrentWindowValues, which
breaks callers that only require FromJSON; remove the ToJSON a constraint from
the type signatures of getCurrentWindowValuesUptoLast and getCurrentWindowValues
so they only require FromJSON a, and if serialization is needed for hash
maintenance keep ToJSON a on the internal helper (e.g.
readWindowFromCombinedCache or a new internal function) so the public API
remains source-compatible while internal code retains the serialization
requirement.
- Line 460: The bucket generation uses arithmetic subtraction via
convertPeriodTypeToSeconds which breaks for calendar periods; update the logic
that builds bucketIds (the map calling makeSWKeyForTime with swo, now,
periodBucketId) to step backwards using calendar-aware adjustments for Months
and Years (e.g., subtract one calendar month/year from the current time per
step) instead of subtracting a fixed number of seconds; apply the same change to
the other occurrences noted around the block that builds buckets (also
referenced in the region around lines 509-515) so monthly/yearly windows are
computed by calendar-aware date arithmetic rather than fixed-second offsets.
- Around line 445-501: The combined HASH (mkCombinedKey) can become stale
because writes to per-period STRING buckets (e.g., in
incrementByValueInTimeBucket and the decrement path) don't update or invalidate
the corresponding field in the combined hash; update those write paths so after
mutating a non-current per-period key you also remove or refresh its field in
the combined hash: compute the bucket id using
makeSWKeyForTime/makeSlidingWindowKey (same logic used in
readWindowFromCombinedCache), then call Redis.hDel combinedKey bucketId (or
Redis.hSetExp combinedKey bucketId newValue ttl if you have the new serialized
value) under the same locking/consistency semantics you already use so readers
of readWindowFromCombinedCache won't keep seeing stale hash entries.
🪄 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
Run ID: 044b9747-8ad2-4022-be04-55fb1b9000cc
📒 Files selected for processing (1)
lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs
| getCurrentWindowValuesUptoLast :: | ||
| ( L.MonadFlow m, | ||
| Redis.HedisFlow m r, | ||
| FromJSON a, | ||
| ToJSON a, | ||
| TryException m | ||
| ) => | ||
| Integer -> | ||
| Text -> | ||
| SlidingWindowOptions -> | ||
| m [Maybe a] | ||
| getCurrentWindowValuesUptoLast nPeriod key swo = do | ||
| utcTime <- L.runIO getCurrentTime | ||
| let keysToFetch = getkeysUptoThisPeriod swo utcTime (makeSlidingWindowKey (periodType swo) key) nPeriod | ||
| mapM Redis.get keysToFetch | ||
| getCurrentWindowValuesUptoLast nPeriod key swo = | ||
| take (fromInteger nPeriod) <$> readWindowFromCombinedCache key swo | ||
|
|
||
| -- | Read all `period` bucket values from the combined cache. | ||
| getCurrentWindowValues :: | ||
| ( L.MonadFlow m, | ||
| Redis.HedisFlow m r, | ||
| FromJSON a, | ||
| ToJSON a, | ||
| TryException m | ||
| ) => | ||
| Text -> | ||
| SlidingWindowOptions -> | ||
| m [Maybe a] | ||
| getCurrentWindowValues key swo = do | ||
| utcTime <- L.runIO getCurrentTime | ||
| let keysToFetch = getkeysForLastPeriods swo utcTime $ makeSlidingWindowKey (periodType swo) key | ||
| mapM Redis.get keysToFetch | ||
| getCurrentWindowValues = readWindowFromCombinedCache |
There was a problem hiding this comment.
Keep the exported read API source-compatible.
Adding ToJSON a here is a compile-time breaking change for callers that only needed FromJSON to read window values. If hash maintenance needs serialization, keep that requirement internal instead of widening the public surface.
🤖 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 `@lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs` around lines 418
- 443, The new ToJSON a constraint was added to the exported functions
getCurrentWindowValuesUptoLast and getCurrentWindowValues, which breaks callers
that only require FromJSON; remove the ToJSON a constraint from the type
signatures of getCurrentWindowValuesUptoLast and getCurrentWindowValues so they
only require FromJSON a, and if serialization is needed for hash maintenance
keep ToJSON a on the internal helper (e.g. readWindowFromCombinedCache or a new
internal function) so the public API remains source-compatible while internal
code retains the serialization requirement.
fad1656 to
1d0d5f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs`:
- Around line 240-242: The mutation of historical buckets (the Redis.hDel call
that removes field bid computed by periodBucketId using periodType/utcTime/now,
and similar mutations at the other locations) must be serialized with the
read-side repair by acquiring the same mkReadLockKey lock used in the repair
path before performing any modification; wrap the code paths that compute bid
and call Redis.hDel (and the analogous updates at the other two sites) in the
same lock acquisition/hold/release sequence used by the repair (use
mkReadLockKey and the existing lock helper used around the repair), ensuring the
lock is held while reading mbTimeStamp, computing bid and performing Redis.hDel
so a concurrent repair cannot read-stale and later re-write old values.
- Around line 490-492: The code currently writes a numeric 0 into the hash to
represent a cache miss (Redis.hSetExp combinedKey bid (0 :: Integer) ...), which
corrupts the polymorphic Maybe-a API; instead, persist absence by removing the
field from the hash so readers still see Nothing. Replace the hSetExp call with
a deletion call (use Redis.hDel combinedKey bid) and then ensure the combinedKey
retains the intended TTL by setting the key expiration separately (e.g. call
Redis.expire or the library equivalent with fromInteger ttl). This keeps the
wire format consistent with Maybe a and avoids encoding sentinel values into the
value hash.
🪄 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
Run ID: 94cbf8dc-96f3-482e-a5cf-968901e5a0ed
📒 Files selected for processing (1)
lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs
Hari-Krishna-Dupaguntla
force-pushed
the
update_sliding_window
branch
2 times, most recently
from
5e6ad46 to
9a3ebc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs`:
- Around line 526-527: The read-path repair call to Redis.withWaitAndLockRedis
is using the hardcoded retry delay of 20 (Redis.withWaitAndLockRedis
(mkReadLockKey key) 10 20 ...); change that literal to use the same shared
retry-delay constant used in the other repair fixes (e.g. waitRetryDelay /
acquireRetryDelay constant) so the call becomes Redis.withWaitAndLockRedis
(mkReadLockKey key) 10 <sharedRetryDelay> and then ensure the shared constant is
imported/visible where mkReadLockKey and repairUnderLock are used.
- Around line 250-252: The retry delay passed into withWaitAndLockRedis is
currently 20 (µs) causing busy-looping; change the delay argument to a much
larger backoff (e.g., 50000 for 50ms) or replace the fixed 20 with a
configurable/backoff value to reduce retry frequency: update the call site that
invokes Redis.withWaitAndLockRedis (the one using mkReadLockKey key and then
Redis.hDel (mkCombinedKey key) [bid]) to pass a sensible delay (or a backoff
function/config) instead of 20 so the lock retry loop sleeps long enough under
contention.
- Around line 300-302: The decrement path uses Redis.withWaitAndLockRedis
(mkReadLockKey key) 10 20 which suffers the same 20µs retry-delay bug; change
the retry/delay argument (the last numeric parameter passed to
Redis.withWaitAndLockRedis) from 20 to the corrected value used in the other
path (e.g. 20000) so the wait/retry delay is correctly scaled, leaving the
surrounding calls (mkReadLockKey, mkCombinedKey, Redis.hDel) unchanged.
🪄 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
Run ID: 8e32da73-3144-4844-a40d-2234fb33824a
📒 Files selected for processing (1)
lib/mobility-core/src/Kernel/Utils/SlidingWindowCounters.hs
Hari-Krishna-Dupaguntla
force-pushed
the
update_sliding_window
branch
3 times, most recently
from
95ee31d to
40dd915
Compare
| if null missing && null stale | ||
| then pure initial | ||
| else | ||
| Redis.withWaitAndLockRedis (mkReadLockKey key) 10 5000 $ |
There was a problem hiding this comment.
add a log here, and verify what is the input type for the dealy variable in withWaitAndLockRedis, is it millisec or microsec
| Just v -> do | ||
| Redis.hSetExp combinedKey bid v (fromInteger ttl) | ||
| pure $ Just (bid, v) | ||
| Nothing -> pure Nothing |
There was a problem hiding this comment.
nothing from here should update the count to 0 in hashmap so that we dont go and fetch it everytime in repair flow.
| repairUnderLock key swo@SlidingWindowOptions {..} historicalBuckets windowSet = do | ||
| let combinedKey = mkCombinedKey key | ||
| ttl = combinedTtl swo | ||
| cur <- Map.fromList <$> Redis.hGetAll combinedKey |
There was a problem hiding this comment.
this is already fetched once in repairCombinedCacheIfStale at line 521. also you already know the missing keys already, use that instead of whole historicalBuckets in this function.
please reuse things.
There was a problem hiding this comment.
No bro, if two threads try to do the same thing then it will be a redunduct process right?
we have kept the lock after checking.
Okay bro I will move the lock to repairCombinedCacheIfStale function only
Hari-Krishna-Dupaguntla
force-pushed
the
update_sliding_window
branch
7 times, most recently
from
fa24a68 to
774b780
Compare
|
@ClaudeWeb can you check if all the comments by @piyushKumar-1 are taken care of, or somethings still needs to be done, if yes please do it |
|
Review of @piyushKumar-1's comments — status check:
Fix applied for comment #2:
-- Before (wrong)
Nothing -> do
Redis.hSetExp combinedKey bid (0 :: Integer) (fromInteger ttl)
pure NothingThis corrupts the polymorphic -- After (correct)
Nothing -> pure NothingPushed as commit |
|
Let me fetch the PR details, diff, and all comments to check what @piyushKumar-1 has raised.Let me try using the GitHub MCP tool instead.Now let me read the comments file to find @piyushKumar-1's specific inline comments.Let me continue reading to find @piyushKumar-1's specific comments.Found the first @piyushKumar-1 comment. Let me read more to find all of them.Now let me look at the current state of the file in the workspace to see what's been addressed.The workspace has the base branch. Let me fetch the current PR file state.This is the main branch. Let me get the PR branch version.Let me fetch the file from the PR branch specifically.Now I have a complete picture. Let me check the
Applying the fix now:Now verify the fix looks right:Good. Now commit and push to the PR branch:Pushed. Now let me add a comment on the PR to explain what was done.Here's the full status of @piyushKumar-1's 3 review comments:
What was fixed: |
Hari-Krishna-Dupaguntla
force-pushed
the
update_sliding_window
branch
from
c7a57a0 to
774b780
Compare
Hari-Krishna-Dupaguntla
force-pushed
the
update_sliding_window
branch
from
774b780 to
5ccad64
Compare
3 participants
Type of Change
Description
Additional Changes
Motivation and Context
How did you test it?
Checklist
./dev/format-all-files.shSummary by CodeRabbit
New Features
Refactor