Bug 1897264 - interruption support for ingestion #6246

bendk · 2024-05-16T19:25:41Z

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.

bendk · 2024-05-16T19:27:00Z

components/suggest/src/store.rs

@@ -601,6 +639,8 @@ struct SuggestStoreDbs {
    writer: SuggestDb,
    /// A read-only connection used to query the database.
    reader: SuggestDb,
+    /// A read-write connection used specifically for ingestion
+    ingestion: SuggestDb,


I like the idea of adding a 3rd connection, but maybe we should wait on this. We could put out a quick fix for the iOS issue and tackle this separately. WDY2T?

Oh, let's not have two read-write connections if we could, please...having multiple writers is a bag of hurt in SQLite. We unfortunately learned that the hard way with Rust Places, where we saw contention between the writer and syncer—and when we tried that on Desktop, we could even wedge one writer permanently, so that it always returned SQLITE_BUSY 😱

Seems good for now, I updated the PR to do that.

Long-term, I still kind of like separating the connections since it allows you to cancel each separately. The current interrupt_ingestion method could also interrupt a call to dismiss_suggestion. I think there's a way to do it safely, although maybe it's just because I haven't been burned by SQLite yet.

(https://bugzilla.mozilla.org/show_bug.cgi?id=1897299)

Thanks, @bendk, and I'm truly sorry if my reaction came off as short in any way—I was running a little late for an appointment earlier, and you deserved a more thoughtful response with more time. I left some more noodlings on https://bugzilla.mozilla.org/show_bug.cgi?id=1897299 for your consideration 🧠

codecov-commenter · 2024-05-16T19:32:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.85%. Comparing base (1ebbd1a) to head (767350e).

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6246       +/-   ##
===========================================
- 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.

linabutler

The shape of the API looks great (thanks for doing this!), but having a second writer is inviting dragons 🐉

Would you mind dropping that in favor of having interrupt_ingestion() just interrupt the existing writer?

linabutler · 2024-05-16T23:43:05Z

components/suggest/src/db.rs

@@ -651,10 +659,11 @@ impl<'a> SuggestDao<'a> {
        &mut self,
        record_id: &SuggestRecordId,
        suggestions: &[DownloadedAmoSuggestion],
+        ingest_interrupt_scope: &SqlInterruptScope,


Ahh, I had SuggestDao hold an interrupt scope specifically so that we could avoid passing it around 😄

The Dao is a transient object that only lives for the lifetime of a read() or write() call, so I was thinking that, if you wanted a separate interrupt scope for just ingestion, you could create a SuggestDao with a separate interrupt scope, and that SuggestDao would only live for the lifetime of the ingestion.

WDYT? This isn't blocking by any means; I just wanted to make sure I understood if there were other reasons for passing the scope around explicitly.

The transaction gets committed at end of write and I didn't want to change that behavior. But it does seem better to just pass around the SuggestDao. I'll update the PR to create a single DAO for the entire ingestion and add a SuggestDao::commit method that we can call. This seems better all-around, I prefer seeing explicit commits in the code.

linabutler

Just a comment about passing the interrupt scope around explicitly now, but no need to block on that. Thanks again, @bendk!

bendk · 2024-05-17T15:42:39Z

Updated the change so that instead of creating a new scope at the top of ingest, we create the SuggestDao and re-use it for the entire ingestion. This feels a lot nicer to me, although there's a minor difference because before we would commit the transaction after each individual write call. I don't think this is a big deal for now, we're not getting many reports of failures to ingest.

I'm thinking we can merge this, which will unblock Lina to fix things on iOS, then create a follow-up PR that adds back some extra commits. I also think we could further simplify some code now that we only have one SuggestDao.

bendk · 2024-05-17T16:45:23Z

Pushing one more change that adds back the transaction support here. I did this because a) I realized it wasn't that hard to add it back and b) the code is not quite ready for the other cleanups I wanted to do. Also, I renamed the method back to interrupt_everything since that's the most accurate. Maybe we can add an interrupt_ingestion later on after we discuss it more.

Sorry for moving the target with this one, but I think it's ready for review again now.

linabutler

Still LGTM, thanks!

linabutler · 2024-05-17T17:09:48Z

components/suggest/src/db.rs

+    /// if [Self::interrupt_handle::interrupt] is called after the operation starts.  Calling
+    /// [Self::write] multiple times during the operation risks missing a call that happens after
+    /// between those calls.
+    pub fn write_scope(&self) -> Result<WriteScope> {


Oooo, I really like this!

ncloudioj

LGTM, could you also update the README.md to reflect the change? It only covers how to use interrupt() for query().

ncloudioj · 2024-05-17T17:57:33Z

Also, I renamed the method back to interrupt_everything since that's the most accurate. Maybe we can add an interrupt_ingestion later on after we discuss it more.

I was gonna ask if it makes sense to consolidate those interrupt calls as consumers almost certainly need to come here to check the document to tell which one should be used. Would interrupt(scope: InterruptScope) sounds better? Where InterruptScope can be read, write, and read-write.

bendk · 2024-05-17T21:03:46Z

I was gonna ask if it makes sense to consolidate those interrupt calls as consumers almost certainly need to come here to check the document to tell which one should be used. Would interrupt(scope: InterruptScope) sounds better? Where InterruptScope can be read, write, and read-write.

I like this idea, but I feel like "scope" is already somewhat overloaded so I called it InterruptKind. For backwards compatibility, I made this param optional. Hopefully we can update consumers to pass it in and drop the optional part.

linabutler · 2024-05-19T15:17:19Z

components/suggest/src/suggest.udl

+enum InterruptKind {
+    "Read",
+    "Write",
+    "Everything",


Absolute nitpick: What do you think of ReadWrite instead of Everything as a name? 😅

This new API is so elegant!

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.

linabutler · 2024-05-29T07:04:49Z

@Mergifyio backport release-v127

mergify · 2024-05-29T07:04:55Z

backport release-v127

✅ Backports have been created

#6256 Bug 1897264 - interruption support for ingestion (backport #6246) has been created for branch release-v127 but encountered conflicts

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

bendk requested review from ncloudioj and linabutler May 16, 2024 19:25

bendk commented May 16, 2024

View reviewed changes

linabutler suggested changes May 16, 2024

View reviewed changes

bendk force-pushed the suggest-ingestion-interrupt branch 2 times, most recently from 401dd7f to 319bc7f Compare May 16, 2024 20:42

linabutler reviewed May 16, 2024

View reviewed changes

linabutler approved these changes May 16, 2024

View reviewed changes

bendk force-pushed the suggest-ingestion-interrupt branch from 319bc7f to 863f41d Compare May 17, 2024 15:38

bendk force-pushed the suggest-ingestion-interrupt branch 4 times, most recently from a1ab648 to 2358b87 Compare May 17, 2024 16:44

linabutler approved these changes May 17, 2024

View reviewed changes

ncloudioj approved these changes May 17, 2024

View reviewed changes

bendk force-pushed the suggest-ingestion-interrupt branch from 2358b87 to 50a6710 Compare May 17, 2024 21:02

bendk force-pushed the suggest-ingestion-interrupt branch from 50a6710 to e358269 Compare May 17, 2024 21:08

linabutler reviewed May 19, 2024

View reviewed changes

bendk force-pushed the suggest-ingestion-interrupt branch from e358269 to 767350e Compare May 20, 2024 14:46

bendk added this pull request to the merge queue May 20, 2024

Merged via the queue into mozilla:main with commit 8bd7ba4 May 20, 2024
15 of 16 checks passed

bendk deleted the suggest-ingestion-interrupt branch May 20, 2024 17:31

mergify bot mentioned this pull request May 29, 2024

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

Merged

5 tasks

pascalchevrel added a commit that referenced this pull request May 29, 2024

Merge pull request #6256 from mozilla/mergify/bp/release-v127/pr-6246

Loading
Loading status checks…

36ef28d

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1897264 - interruption support for ingestion #6246

Bug 1897264 - interruption support for ingestion #6246

bendk commented May 16, 2024 •

edited

Loading

bendk May 16, 2024 •

edited

Loading

linabutler May 16, 2024 •

edited

Loading

bendk May 16, 2024 •

edited

Loading

linabutler May 17, 2024

codecov-commenter commented May 16, 2024 •

edited

Loading

linabutler left a comment

linabutler May 16, 2024

bendk May 17, 2024 •

edited

Loading

linabutler left a comment

bendk commented May 17, 2024

bendk commented May 17, 2024

linabutler left a comment

linabutler May 17, 2024

ncloudioj left a comment

ncloudioj commented May 17, 2024

bendk commented May 17, 2024

linabutler May 19, 2024

linabutler commented May 29, 2024

mergify bot commented May 29, 2024 •

edited

Loading

Bug 1897264 - interruption support for ingestion #6246

Bug 1897264 - interruption support for ingestion #6246

Conversation

bendk commented May 16, 2024 • edited Loading

Pull Request checklist

bendk May 16, 2024 • edited Loading

Choose a reason for hiding this comment

linabutler May 16, 2024 • edited Loading

Choose a reason for hiding this comment

bendk May 16, 2024 • edited Loading

Choose a reason for hiding this comment

linabutler May 17, 2024

Choose a reason for hiding this comment

codecov-commenter commented May 16, 2024 • edited Loading

Codecov Report

linabutler left a comment

Choose a reason for hiding this comment

linabutler May 16, 2024

Choose a reason for hiding this comment

bendk May 17, 2024 • edited Loading

Choose a reason for hiding this comment

linabutler left a comment

Choose a reason for hiding this comment

bendk commented May 17, 2024

bendk commented May 17, 2024

linabutler left a comment

Choose a reason for hiding this comment

linabutler May 17, 2024

Choose a reason for hiding this comment

ncloudioj left a comment

Choose a reason for hiding this comment

ncloudioj commented May 17, 2024

bendk commented May 17, 2024

linabutler May 19, 2024

Choose a reason for hiding this comment

linabutler commented May 29, 2024

mergify bot commented May 29, 2024 • edited Loading

✅ Backports have been created

bendk commented May 16, 2024 •

edited

Loading

bendk May 16, 2024 •

edited

Loading

linabutler May 16, 2024 •

edited

Loading

bendk May 16, 2024 •

edited

Loading

codecov-commenter commented May 16, 2024 •

edited

Loading

bendk May 17, 2024 •

edited

Loading

mergify bot commented May 29, 2024 •

edited

Loading