Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1897264 - interruption support for ingestion (backport #6246) #6256

Merged
merged 2 commits into from
May 29, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented May 29, 2024

Added an interrupt_everything method that interrupts both the read and write connection. This is intended for iOS to use to interrupt the suggest component during shutdown. See https://bugzilla.mozilla.org/show_bug.cgi?id=1897299 for a discussion on next steps here.

Created a new WriteScope type that stores an interrupt scope that can be used for multiple write calls. This allows the ingestion code to catch all interruptions that happened after the ingestion started. With the old system, if interrupt_everything was called in-between ingestion two types then we might miss it.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.


This is an automatic backport of pull request #6246 done by [Mergify](https://mergify.com).

@mergify mergify bot added the conflicts label May 29, 2024
Copy link
Contributor Author

mergify bot commented May 29, 2024

Cherry-pick of 8bd7ba4 has failed:

On branch mergify/bp/release-v127/pr-6246
Your branch is up to date with 'origin/release-v127'.

You are currently cherry-picking commit 8bd7ba4ea.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   components/suggest/README.md
	modified:   components/suggest/src/suggest.udl

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   components/suggest/src/db.rs
	both modified:   components/suggest/src/lib.rs
	both modified:   components/suggest/src/store.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.85%. Comparing base (5e7b702) to head (c97e7ec).

Additional details and impacted files
@@                Coverage Diff                @@
##           release-v127    #6256       +/-   ##
=================================================
- Coverage         83.68%   50.85%   -32.84%     
=================================================
  Files               117      112        -5     
  Lines             15653    11811     -3842     
=================================================
- Hits              13099     6006     -7093     
- Misses             2554     5805     +3251     

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

bendk added 2 commits May 29, 2024 00:15
Made the API more high-level and not just a copy of the remote settings
client API.  It now inputs a `SuggestRemoteSettingsRecordRequest` which
is specific to the kinds of requests we make.  It outputs a list of
records linked to the attachment data.  This way:

  - It's simpler to mock up in the tests since the mocks can match the
    actual requests more directly.  I'm hoping to use this to simplify
    the store tests which have gotten really out of hand.
  - We abstract away some of the remote settings client details, which
    makes it easer to switch how we make requests in the future.

(cherry picked from commit 1df5028)
Added an `interrupt_everything` method that interrupts both the read and
write connection.  This is intended for iOS to use to interrupt the
suggest component during shutdown.  See
https://bugzilla.mozilla.org/show_bug.cgi?id=1897299 for a discussion on
next steps here.

Created a new `WriteScope` type that stores an interrupt scope that can
be used for multiple `write` calls.  This allows the ingestion code to
catch all interruptions that happened after the ingestion started. With
the old system, if `interrupt_everything` was called in-between
ingestion two types then we might miss it.

(cherry picked from commit 8bd7ba4)
@linabutler linabutler force-pushed the mergify/bp/release-v127/pr-6246 branch from c97e7ec to 0a7792f Compare May 29, 2024 07:16
@linabutler
Copy link
Member

Alrighty—I backported #6236 as part of this PR, too, because it was a lot easier to rebase #6246 atop it! 😅

@bendk Would you mind double-checking this just in case, please, before I request uplift? Thanks!

Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thanks for doing this uplift.

@linabutler
Copy link
Member

The SwiftLint failures aren't caused by this PR (but, unfortunately, that means it's failing on all our PRs now)—#6254 fixes them for everyone!

@pascalchevrel pascalchevrel merged commit 36ef28d into release-v127 May 29, 2024
14 of 15 checks passed
@pascalchevrel pascalchevrel deleted the mergify/bp/release-v127/pr-6246 branch May 29, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants