[flags]: evaluation aggregation followup#3178
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies the lock handling in the evaluation aggregation system and ensures the periodic flush timer is restarted after auto-drain events. The changes address a performance optimization by removing redundant lock acquisitions and fix a missing timer restart when aggregations are automatically flushed due to reaching the maximum threshold.
Changes:
- Simplified lock acquisition in
EvaluationAggregator.record()to use a single critical section instead of releasing and re-acquiring the lock - Added
reschedulePeriodicFlush()call inEvaluationEventsProcessor.processEvaluation()when auto-drain occurs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/aggregation/EvaluationAggregator.kt | Simplified lock handling by consolidating the record and drain operations into a single critical section, removing the previous pattern that released and re-acquired the lock |
| features/dd-sdk-android-flags/src/main/kotlin/com/datadog/android/flags/internal/EvaluationEventsProcessor.kt | Added timer restart after auto-drain to ensure periodic flush scheduling is reset, making behavior consistent with manual flush |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val drained = mapLock.withLock { | ||
| @Suppress("UnsafeThirdPartyFunctionCall") // Only throws if null is passed | ||
| val existing = aggregationMap.get(key) ?: AggregationStats( | ||
| aggregationKey = key, |
There was a problem hiding this comment.
The documentation at lines 28-30 states "Uses double-check locking to avoid spurious flushes when multiple threads reach the threshold simultaneously. The size is checked without a lock first (fast path), then re-checked while holding the write lock before draining." However, this change removes the double-check locking pattern - the implementation now acquires the lock once and performs all operations (record + size check + drain) within a single critical section. The documentation should be updated to reflect this change.
| writer.writeAll(drainedEvents) | ||
| reschedulePeriodicFlush() |
There was a problem hiding this comment.
minor: we don't need to wait for write to finish, we can reschedule first (similar to how we do in regular flush)
| writer.writeAll(drainedEvents) | |
| reschedulePeriodicFlush() | |
| reschedulePeriodicFlush() | |
| writer.writeAll(drainedEvents) |
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3178 +/- ##
===========================================
+ Coverage 71.27% 71.37% +0.10%
===========================================
Files 929 929
Lines 34450 34455 +5
Branches 5817 5816 -1
===========================================
+ Hits 24554 24592 +38
+ Misses 8257 8247 -10
+ Partials 1639 1616 -23
🚀 New features to boost your workflow:
|
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
|
Base changed (feature is merged) and merged the latest from developer. PTAL, @0xnm |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
and removed
mergequeue-status: waiting
mergequeue-status: queued
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
What does this PR do?
This PR simplifies one of the aggregation lock holds and restarts the memory->storage flush timer.
Motivation
@dd-oleksii, legend
Additional Notes
Review checklist (to be filled by reviewers)