[Data Cleanup] Speed up check for deferred duplicates#3793

imnasnainaec · 2025-05-13T14:34:54Z

This change is

codecov · 2025-05-13T14:37:43Z

Codecov Report

Attention: Patch coverage is 79.16667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 72.87%. Comparing base (dbbe87c) to head (f6d039d).
Report is 43 commits behind head on master.

Files with missing lines	Patch %	Lines
Backend/Services/MergeService.cs	81.25%	3 Missing ⚠️
Backend/Controllers/MergeController.cs	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3793      +/-   ##
==========================================
+ Coverage   72.82%   72.87%   +0.05%     
==========================================
  Files         286      286              
  Lines       10715    10737      +22     
  Branches     1330     1335       +5     
==========================================
+ Hits         7803     7825      +22     
+ Misses       2517     2515       -2     
- Partials      395      397       +2

Flag	Coverage Δ
backend	`81.92% <78.26%> (+0.08%)`	⬆️
frontend	`65.73% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hahn-kev · 2025-05-14T03:47:42Z

Backend/Controllers/MergeController.cs

+        /// <param name="userId"> Id of user whose merge graylist is to be used. </param>
+        [HttpGet("hasgraylist/{userId}", Name = "HasGraylistEntries")]
+        [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(bool))]
+        public async Task<IActionResult> getGraylistEntries(string projectId, string userId)


the name getGraylistEntries should be capitalized

hahn-kev · 2025-05-14T03:48:39Z

Backend/Repositories/WordRepository.cs

+            using var activity = OtelService.StartActivityWithTag(otelTagName, "checking if Frontier contains words");
+
+            var words = await _wordDatabase.Frontier.Find(GetProjectWordsFilter(projectId, wordIds))
+                .Limit(count).ToListAsync();


might I suggest CountAsync?

You might... it's a good suggestion. I opted for Find so I could use .Limit, which doesn't work with CountDocuments. But at your suggestion, I took a look at https://www.mongodb.com/docs/manual/reference/method/db.collection.countDocuments/ and discovered that it supports the limit in a different way!

imnasnainaec

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @andracc and @hahn-kev)

imnasnainaec · 2025-05-14T10:50:24Z

Backend/Controllers/MergeController.cs

+        /// <param name="userId"> Id of user whose merge graylist is to be used. </param>
+        [HttpGet("hasgraylist/{userId}", Name = "HasGraylistEntries")]
+        [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(bool))]
+        public async Task<IActionResult> getGraylistEntries(string projectId, string userId)


imnasnainaec · 2025-05-14T10:50:24Z

Backend/Repositories/WordRepository.cs

+            using var activity = OtelService.StartActivityWithTag(otelTagName, "checking if Frontier contains words");
+
+            var words = await _wordDatabase.Frontier.Find(GetProjectWordsFilter(projectId, wordIds))
+                .Limit(count).ToListAsync();


You might... it's a good suggestion. I opted for Find so I could use .Limit, which doesn't work with CountDocuments. But at your suggestion, I took a look at https://www.mongodb.com/docs/manual/reference/method/db.collection.countDocuments/ and discovered that it supports the limit in a different way!

andracc

Reviewed 1 of 7 files at r1, 1 of 7 files at r2, 1 of 3 files at r3.
Reviewable status: 3 of 13 files reviewed, 3 unresolved discussions (waiting on @hahn-kev and @imnasnainaec)

Backend.Tests/Mocks/WordRepositoryMock.cs line 81 at r3 (raw file):

        {
            return Task.FromResult(
                _frontier.Where(w => w.ProjectId == projectId && wordIds.Contains(w.Id)).Count() == count);

Since count represents a minimum and we are not limiting the results here, it looks like we can use >= instead of ==.

imnasnainaec

Dismissed @hahn-kev from 2 discussions.
Reviewable status: 3 of 13 files reviewed, all discussions resolved (waiting on @imnasnainaec)

Backend.Tests/Mocks/WordRepositoryMock.cs line 81 at r3 (raw file):

Previously, andracc (Andra) wrote…

Since count represents a minimum and we are not limiting the results here, it looks like we can use >= instead of ==.

We shouldn't have multiple words in the frontier with the same id, though we're not enforcing that in the test setting, so we could go either >= or == here. I'm inclined to keep it as == to closer mimic the expectations of the real thing.

imnasnainaec

Reviewable status: 3 of 13 files reviewed, all discussions resolved (waiting on @imnasnainaec)

Backend.Tests/Mocks/WordRepositoryMock.cs line 81 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

We shouldn't have multiple words in the frontier with the same id, though we're not enforcing that in the test setting, so we could go either >= or == here. I'm inclined to keep it as == to closer mimic the expectations of the real thing.

Oh, I'm sorry! Thanks for nudging me on this in Slack. (For some reason I was thinking about the case when count is the same as wordIds.Count.) I've fixed it now.

andracc

Reviewed 3 of 7 files at r1, 4 of 7 files at r2, 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)

imnasnainaec added 2 commits May 13, 2025 10:28

Speed up check for graylist

5c132d0

Clean up comments

8e73fdc

imnasnainaec self-assigned this May 13, 2025

imnasnainaec added backend performance goal: MergeDup goal labels May 13, 2025

imnasnainaec added 2 commits May 13, 2025 12:48

Update frontend tests

519360b

Fix HasGraylistEntries and add tests

b1ced41

imnasnainaec requested a review from andracc May 13, 2025 17:41

imnasnainaec added 3 commits May 13, 2025 14:07

Refine db query

055da67

Merge branch 'master' into has-graylist

95c3b6b

Merge branch 'master' into has-graylist

2f72a48

hahn-kev reviewed May 14, 2025

View reviewed changes

imnasnainaec added 4 commits May 14, 2025 06:36

Fix names

fa4c3e6

Add rudamentary tests

4f03cf9

Use CountDocumentsAsync with options

2e856ae

Make test names consistant accross file

fd0a5e3

imnasnainaec commented May 14, 2025

View reviewed changes

Merge branch 'master' into has-graylist

6faa546

andracc requested changes May 15, 2025

View reviewed changes

imnasnainaec commented May 15, 2025

View reviewed changes

Fix mock AreInFrontier

68a0f93

imnasnainaec commented May 16, 2025

View reviewed changes

Merge branch 'master' into has-graylist

f6d039d

andracc approved these changes May 19, 2025

View reviewed changes

imnasnainaec merged commit 9697905 into master May 19, 2025
18 checks passed

imnasnainaec deleted the has-graylist branch May 19, 2025 16:25

Uh oh!

Conversation

imnasnainaec commented May 13, 2025 • edited by jasonleenaylor Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hahn-kev May 14, 2025

Choose a reason for hiding this comment

Uh oh!

imnasnainaec May 14, 2025

Choose a reason for hiding this comment

Uh oh!

hahn-kev May 14, 2025

Choose a reason for hiding this comment

Uh oh!

imnasnainaec May 14, 2025

Choose a reason for hiding this comment

Uh oh!

imnasnainaec left a comment

Choose a reason for hiding this comment

Uh oh!

imnasnainaec May 14, 2025

Choose a reason for hiding this comment

Uh oh!

imnasnainaec May 14, 2025

Choose a reason for hiding this comment

Uh oh!

andracc left a comment

Choose a reason for hiding this comment

Uh oh!

imnasnainaec left a comment

Choose a reason for hiding this comment

Uh oh!

imnasnainaec left a comment

Choose a reason for hiding this comment

Uh oh!

andracc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

imnasnainaec commented May 13, 2025 •

edited by jasonleenaylor

Loading

codecov bot commented May 13, 2025 •

edited

Loading