Minor auto_refresh cleanup #5813
Conversation
Sovietaced
force-pushed
the
auto-refresh-cleanup
branch
from
f6c93f4 to
eeabd91
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5813 +/- ##
=======================================
Coverage 37.05% 37.05%
=======================================
Files 1318 1318
Lines 132583 132604 +21
=======================================
+ Hits 49122 49139 +17
- Misses 79211 79215 +4
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. |
Sure |
Sovietaced
force-pushed
the
auto-refresh-cleanup
branch
from
b0d678a to
1ae220c
Compare
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Sovietaced
force-pushed
the
auto-refresh-cleanup
branch
from
1ae220c to
9a40f0a
Compare
|
Should be good for another look @pvditt |
Code Review Agent Run #0468bfActionable Suggestions - 2
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| go func(ctx context.Context) { | ||
| err := w.sync(ctx) |
There was a problem hiding this comment.
Consider adding error handling for goroutine panics using defer recover(). The current implementation may lead to unhandled panics in worker goroutines.
Code suggestion
Check the AI-generated fix before applying
| go func(ctx context.Context) { | |
| err := w.sync(ctx) | |
| go func(ctx context.Context) { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| logger.Errorf(ctx, "Panic in worker goroutine: %v\n%s", r, debug.Stack()) | |
| } | |
| }() | |
| err := w.sync(ctx) |
Code Review Run #0468bf
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| updatedBatch, err := w.syncCb(ctx, newBatch) | ||
|
|
||
| if err != nil { | ||
| w.metrics.SyncErrors.Inc() | ||
| logger.Errorf(ctx, "failed to get latest copy of a batch. Error: %v", err) | ||
| t.Stop() | ||
| continue | ||
| } |
There was a problem hiding this comment.
The sync latency metric timer t is stopped in the error path but continues timing in the success path until line 370. Consider stopping the timer immediately after the sync callback to ensure accurate latency measurements.
Code suggestion
Check the AI-generated fix before applying
| updatedBatch, err := w.syncCb(ctx, newBatch) | |
| if err != nil { | |
| w.metrics.SyncErrors.Inc() | |
| logger.Errorf(ctx, "failed to get latest copy of a batch. Error: %v", err) | |
| t.Stop() | |
| continue | |
| } | |
| updatedBatch, err := w.syncCb(ctx, newBatch) | |
| t.Stop() | |
| if err != nil { | |
| w.metrics.SyncErrors.Inc() | |
| logger.Errorf(ctx, "failed to get latest copy of a batch. Error: %v", err) | |
| continue | |
| } |
Code Review Run #0468bf
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Why are the changes needed?
This pull request is some follow up work from #5767 to clean up auto refresh so that it has a constructor that returns a pointer to the implementation instead of an interface. This gives the user of the constructor more flexibility (ie. for intra-package testing) while still adhering to the auto_refresh interface.
What changes were proposed in this pull request?
The default auto_refresh cache implementation was updated to the
InMemoryAutoRefreshand moved into its own file.Some additional fields were added to improve assertions given the nature of the asynchronous enqueue/syncing that occurs with the auto refresh cache.
Lastly, the new constructor for the in memory auto refresh cache was updated to use the options pattern so that test-only values or non-default options only have to be added in those situations.
The preexisting helper functions to create refresh caches have been left in place for backwards compatibility.
Check all the applicable boxes
Summary by Bito
Refactoring of auto-refresh cache implementation with core functionality moved to InMemoryAutoRefresh type. Introduces flexible constructor pattern using options while maintaining backward compatibility. Adds new fields for improved testing of async operations. Changes are structural without modifying core functionality.Unit tests added: False
Estimated effort to review (1-5, lower is better): 3