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

Removes SYNC-3364 [v110] history configuration cleanup #12420

Merged

Conversation

tarikeshaq
Copy link
Contributor

@tarikeshaq tarikeshaq commented Nov 17, 2022

Disclaimer

⚠️ This is a very large PR and I'm planning to break it down into smaller ones, but wanted to share out the cleanups I'm working on. If you're curious look at the code, but mostly reviewing the PR description is what I'm looking for. ⚠️

Overview

With history using Application services places (expected to rollout remotely in 108) we can start cleaning up a lot of legacy code! This PR is incomplete needs a TON of testing and is a work in progress. With that preface, here's what's included:

Removal of configuration

  • We added configuration in 108, so we can remotely configure the Places APIs. This PR removes that configuration (assuming places ships OK in 108, we will not land this until the rollout is complete and considered OK)
  • This PR removes that configuration completely and enables the places API by default

Removal of old APIs

  • With the configuration gone, we can now safely remove the old SqlHistory APIs related to history storage and sync, this PR does that

Refactoring of SQLiteHistory

  • The SQLiteHistory was used to manage:
    • History
    • TopSites
    • Pinned Sites
    • Favicons
    • History Sync
  • After switching to the new history, we can simplify it so that it only manages the features that are not yet available in places:
    • Pinned Sites
    • Favicons
  • This PR includes a ton of refactoring to allow that to happen
  • NOTE: because the refactor touches many surfaces (including top sites) we should definitely run a round of QA before any of it lands. I'm happy to schedule meetings to walk through the changes too

Renaming SQLiteHistory to BrowserDBSQLite

  • After this PR SQLiteHistory won't manage history. So renamed it to BrowserDBSQLite so it's a bit more generic.

Removing Caching for top sites

  • We have a caching layer in SQLiteHistory for top sites, we remove that and call directly into places instead the same way android does

Deletion of dead code and redundant tests

  • There is quite a bit of dead code, and tests. This PR has the removal of that too which accounts to about half the changes here

Refactors not in this PR

There are a few changes that I intentionally left out of here, because I believe they have a bigger scope or should be done even more carefully. THIS IS AN INCOMPLETE LIST AND I WILL KEEP ADDING TO IT

  1. Complete removal of user's data in their history tables, from there, there will be no going back
  2. Complete removal of the table creation for all the tables we no longer use, it should be safe to remove though.

@mergify
Copy link
Contributor

mergify bot commented Nov 17, 2022

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq?

@tarikeshaq tarikeshaq force-pushed the remove-history-configuration-cleanup branch from c14743c to 2cb4639 Compare November 21, 2022 22:09
@tarikeshaq
Copy link
Contributor Author

Got around to removing the dead tests and refactoring any history tests to test places properly - next step will be to split this apart a little and make sure there is enough context on what's going on here

@tarikeshaq tarikeshaq force-pushed the remove-history-configuration-cleanup branch from 4d63932 to b3bf36a Compare November 22, 2022 23:13
@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2022

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq?

Copy link
Contributor Author

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

A short self-review as I clean things up more

If anyone is following along, I want to repeat that I have no intention of landing all of this at once

The idea is to breakdown which of those cleanups are ones we want and land those one by one

Client/Protocols/DataObserver.swift Show resolved Hide resolved
Providers/Profile.swift Outdated Show resolved Hide resolved
Storage/SQL/SQLiteHistory.swift Outdated Show resolved Hide resolved
Storage/SQL/SQLiteHistoryFactories.swift Show resolved Hide resolved
Tests/ClientTests/TestHistory.swift Show resolved Hide resolved
@tarikeshaq tarikeshaq force-pushed the remove-history-configuration-cleanup branch from 75e9e80 to f66952c Compare November 23, 2022 18:59
@tarikeshaq tarikeshaq force-pushed the remove-history-configuration-cleanup branch 3 times, most recently from 7fbcb48 to d34a349 Compare November 24, 2022 23:09
@lmarceau
Copy link
Contributor

Swiftlint warnings not related to this PR, will be fixed with changes here

@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2022

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq?

@tarikeshaq tarikeshaq changed the title [WIP] Remove history configuration cleanup Removes SYNC-3364 [v110] history configuration cleanup Dec 1, 2022
@tarikeshaq tarikeshaq marked this pull request as ready for review December 1, 2022 20:33
@tarikeshaq tarikeshaq force-pushed the remove-history-configuration-cleanup branch from 2d6b24b to 363e307 Compare December 1, 2022 20:36
@lmarceau lmarceau requested review from OrlaM and lmarceau December 1, 2022 21:32
@nbhasin2 nbhasin2 self-requested a review December 6, 2022 20:49
@nbhasin2
Copy link
Contributor

nbhasin2 commented Dec 6, 2022

@tarikeshaq have you already done a walkthrough of this PR?
This is too big imo to be reviewed here on github

@tarikeshaq
Copy link
Contributor Author

@nbhasin2 I did a walk-through with @OrlaM and @lmarceau, some notes as well in https://docs.google.com/document/d/1R7Fko3aPc9bNHc3FazSdHJlcPk1LZeX8lbvkzTvDUxA/edit and https://docs.google.com/document/d/11smb_1bXMkHDHA-ZwdsWIoHFeGXgDajO2S4nYmcyrPQ/edit

@nbhasin2
Copy link
Contributor

nbhasin2 commented Dec 6, 2022

Perfect @tarikeshaq, I will skim through this although you are in good hands of @OrlaM and @lmarceau for the review 🙂
lmk if I can help in any way!!

@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2022

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq?

Copy link
Contributor

@OrlaM OrlaM left a comment

Choose a reason for hiding this comment

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

One small comment, otherwise looks good.
At some point next week we should create a build off this branch to send to QA for testing.

Client/Application/AppLaunchUtil.swift Outdated Show resolved Hide resolved
Client/Application/AppLaunchUtil.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/MainMenuActionHelper.swift Outdated Show resolved Hide resolved
Client/Frontend/Browser/MainMenuActionHelper.swift Outdated Show resolved Hide resolved
Tests/ClientTests/Mocks/MockablePinnedSites.swift Outdated Show resolved Hide resolved
Tests/StoragePerfTests/StoragePerfTests.swift Outdated Show resolved Hide resolved
@lmarceau
Copy link
Contributor

lmarceau commented Dec 20, 2022

The special build was done testing with task #12674, I think this is ready to be merged (but we'll need the experiment result before that @tarikeshaq ?)

@tarikeshaq
Copy link
Contributor Author

Thanks @lmarceau!! Experiment results are in, and I think we should land this. I cover the results in https://docs.google.com/document/d/1fC55c9m66E18ztQwEFhfmesJdSWfes3eSQLcfaq4Kjo/edit

TLDR:

  • 5-6% migrations aren't ending, meaning that we are migrating too much data, in the doc I cover why, we have many users with a lot of history.
  • I'll have a PR before the end of 110 to upgrade to a new version of application-services that limits the migration based on the above findings
  • I'll also have another PR before end of 110 for Follow up for application-services history: Run maintenance on interval to prune database #12680
  • Both PRs will be up and ready before the soft freeze, in the week after we're back from the holidays
  • I have a hope that once we use the new history we'll also see some improvement in reported performance issues, but I'm crossing my fingers on this one.

@mergify
Copy link
Contributor

mergify bot commented Dec 21, 2022

This pull request has conflicts when rebasing. Could you fix it @tarikeshaq?

@tarikeshaq tarikeshaq force-pushed the remove-history-configuration-cleanup branch 2 times, most recently from 5f853d7 to aaab3ee Compare January 4, 2023 20:51
@tarikeshaq tarikeshaq force-pushed the remove-history-configuration-cleanup branch from 2122b89 to 4021983 Compare January 4, 2023 21:26
@OrlaM
Copy link
Contributor

OrlaM commented Jan 4, 2023

Build is green on BR
image

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