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

Adds ios history migration and example #5077

Merged
merged 7 commits into from
Sep 15, 2022
Merged

Adds ios history migration and example #5077

merged 7 commits into from
Sep 15, 2022

Conversation

tarikeshaq
Copy link
Contributor

@tarikeshaq tarikeshaq commented Aug 3, 2022

While running some performance tests for places on iOS, I realized that the best way to keep the data consistent between the databases is to move the same data from one to the other... that is pretty much what we need for the migration work so I decided I'll push it up.

TODOs:

  • Add tests in places-tests to test that the migration logic works beyond the testing against the simulator db I've been doing

I'll be using this + a swift script to populate the BrowserDB for performance testing. (bonus points it'll also be performance testing of the migration)

Branch builds: add [ac: android-components-branch-name] and/or [fenix: fenix-branch-name] to the PR title.

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2022

Codecov Report

Base: 41.45% // Head: 41.45% // No change to project coverage 👍

Coverage data is based on head (4859a61) compared to base (d850347).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5077   +/-   ##
=======================================
  Coverage   41.45%   41.45%           
=======================================
  Files         167      167           
  Lines       12620    12620           
=======================================
  Hits         5232     5232           
  Misses       7388     7388           
Impacted Files Coverage Δ
components/support/types/src/lib.rs 0.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

This is looking great!

@tarikeshaq tarikeshaq force-pushed the adds-ios-import branch 2 times, most recently from 5eb88e9 to 8b57f6d Compare September 7, 2022 18:46
@tarikeshaq tarikeshaq marked this pull request as ready for review September 7, 2022 18:50
@tarikeshaq tarikeshaq requested a review from mhammond September 7, 2022 18:53
Copy link
Contributor

@lougeniaC64 lougeniaC64 left a comment

Choose a reason for hiding this comment

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

Other than the removal of the sql formatting changes in ios/bookmarks.rs, this looks good to me. A lot of the concerns I have are around the duration of the migration and how often it gets interrupted by app crashes and closures, or the app being placed in the background. But all of these concerns will be clarified by dry run data.

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

This looks great to me, but what I don't quite understand is how this is actually a "dry run"? Is it just that we aren't actually switching iOS over to this data? What's the plan for doing the "real" run later?

Another question re the metrics: I guess the iOS PR for this, which is also going to record the metrics, will want to make sure it successfully recorded the migration starting before calling this? My vague concern is that we don't want miss any migrations which started but which got killed before we completed.

But r=me for this migration code and we can discuss the final process for doing it in our meeting!

@tarikeshaq
Copy link
Contributor Author

but what I don't quite understand is how this is actually a "dry run"?

It'll be the app that runs this as a "dry run", the app will construct a separate instance of RustPlaces with a path to a fake database and run this function on that.

The idea is that what we are shipping here can be both the dry run or the real based on remote configuration set by nimbus

I'll demonstrate more in the meeting, but here's the iOS side of the story: mozilla-mobile/firefox-ios#11896

@tarikeshaq tarikeshaq merged commit 1094f87 into main Sep 15, 2022
@tarikeshaq tarikeshaq deleted the adds-ios-import branch September 15, 2022 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants