Add option to disable synchronization on creation for auto refresh cache #5940
Conversation
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Paul Dittamo <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5940 +/- ##
=======================================
Coverage 37.06% 37.06%
=======================================
Files 1318 1318
Lines 132638 132646 +8
=======================================
+ Hits 49158 49165 +7
- Misses 79230 79231 +1
Partials 4250 4250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Code Review Agent Run #8a272aActionable Suggestions - 2
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| // Cache should be processing | ||
| assert.NotEmpty(t, cache.processing) |
There was a problem hiding this comment.
Consider adding assertions to verify the initial state of cache items before triggering the sync. This would help validate that items are properly initialized with val: 0 before being synced to val: 10.
Code suggestion
Check the AI-generated fix before applying
@@ -97,6 +97,12 @@
}
// Cache should be processing
assert.NotEmpty(t, cache.processing)
+ // Verify initial values
+ for i := 1; i <= 10; i++ {
+ item, err := cache.Get(fmt.Sprintf("%d", i))
+ assert.NoError(t, err)
+ assert.Equal(t, 0, item.(fakeCacheItem).val)
+ }
Code Review Run #8a272a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| if w.syncOnCreate { | ||
| batch := make([]ItemWrapper, 0, 1) | ||
| batch = append(batch, itemWrapper{id: id, item: item}) | ||
| w.workqueue.AddRateLimited(&batch) |
There was a problem hiding this comment.
Consider adding a check for w.workqueue being nil before using it in GetOrCreate. The workqueue field could potentially be nil if initialization failed.
Code suggestion
Check the AI-generated fix before applying
| if w.syncOnCreate { | |
| batch := make([]ItemWrapper, 0, 1) | |
| batch = append(batch, itemWrapper{id: id, item: item}) | |
| w.workqueue.AddRateLimited(&batch) | |
| if w.syncOnCreate { | |
| if w.workqueue == nil { | |
| return item, fmt.Errorf("workqueue not initialized") | |
| } | |
| batch := make([]ItemWrapper, 0, 1) | |
| batch = append(batch, itemWrapper{id: id, item: item}) | |
| w.workqueue.AddRateLimited(&batch) |
Code Review Run #8a272a
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Why are the changes needed?
We have an interesting usage pattern of the auto refresh cache.
When items are synchronized in single item batches upon creation it disrupts our batch synchronization strategy and puts a lot of load on the server where task state is being read from. You could tune the rate limiter but it will also affect both single item batch and regular batch synchronization. So the solution we found was to add this option which just doesn't try to add a single item batch when items are created in the cache.
What changes were proposed in this pull request?
Adds an option to the auto refresh cache
SyncOnCreatewhich defaults to true for backwards compatibility.How was this patch tested?
We run this in production but I also added unit tests.
Check all the applicable boxes
Related PRs
Built on top of #5813
Summary by Bito
Introduces 'SyncOnCreate' configuration option for auto-refresh cache system to control immediate synchronization behavior. The feature allows disabling immediate item sync on creation, defaulting to true for backward compatibility. This optimization targets high-load scenarios by enabling batch synchronization instead of individual syncs. Implementation includes configuration changes and corresponding test coverage.Unit tests added: True
Estimated effort to review (1-5, lower is better): 1