[MergeDups] Implement SignalRHub for duplicate finder #3725
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3725 +/- ##
==========================================
- Coverage 73.12% 72.81% -0.31%
==========================================
Files 285 287 +2
Lines 10648 10707 +59
Branches 1328 1334 +6
==========================================
+ Hits 7786 7796 +10
- Misses 2467 2514 +47
- Partials 395 397 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I think you should investigate memory cache as an alternative to the ConcurrentDictionary.
https://learn.microsoft.com/en-us/dotnet/api/system.runtime.caching.memorycache?view=netframework-4.8
Reviewed 3 of 23 files at r1, 7 of 12 files at r4.
Reviewable status: 9 of 27 files reviewed, all discussions resolved (waiting on @imnasnainaec)
| internal async Task<bool> GetDuplicatesThenSignal(string projectId, int maxInList, int maxLists, string userId) | ||
| { | ||
| // Use counter to ensure that only the most recent duplicate request succeeds per user. | ||
| var counter = Interlocked.Increment(ref _mergeCounter); |
There was a problem hiding this comment.
it's not clear to me, is there an assumption that _mergeCounter is persisted between requests? I would verify this as I believe by default controllers are request scoped and not singletons meaning each request _mergeCounter would start at 0.
There was a problem hiding this comment.
Oh, good catch, thank you. Yes, it does need to be persisted, so I'll move that counter into the now-singleton Merge Service.
There was a problem hiding this comment.
(Investigated and discussed elsewhere.)
Reviewable status: 9 of 27 files reviewed, 1 unresolved discussion (waiting on @hahn-kev, @imnasnainaec, and @jasonleenaylor)
| internal async Task<bool> GetDuplicatesThenSignal(string projectId, int maxInList, int maxLists, string userId) | ||
| { | ||
| // Use counter to ensure that only the most recent duplicate request succeeds per user. | ||
| var counter = Interlocked.Increment(ref _mergeCounter); |
There was a problem hiding this comment.
Oh, good catch, thank you. Yes, it does need to be persisted, so I'll move that counter into the now-singleton Merge Service.
There was a problem hiding this comment.
Reviewable status: 8 of 27 files reviewed, 1 unresolved discussion (waiting on @hahn-kev and @jasonleenaylor)
| internal async Task<bool> GetDuplicatesThenSignal(string projectId, int maxInList, int maxLists, string userId) | ||
| { | ||
| // Use counter to ensure that only the most recent duplicate request succeeds per user. | ||
| var counter = Interlocked.Increment(ref _mergeCounter); |
There was a problem hiding this comment.
Reviewed 11 of 22 files at r2, 1 of 3 files at r3, 4 of 12 files at r4, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hahn-kev)
There was a problem hiding this comment.
Dismissed @hahn-kev from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @hahn-kev)
Resolves #3720
This change is