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 1848200 - add migration flag to init #1751

Merged
merged 1 commit into from Aug 11, 2023

Conversation

rosahbruno
Copy link
Contributor

Conditionally migrate IDB data only if it already exists. If the project has never used an older version of Glean, then we want to avoid migration because of a potential race condition when submitting pings on first page load.

Pull Request checklist

  • Quality: Make sure this PR builds and runs cleanly.
    • Inside the glean/ folder, run:
      • npm run test Runs all tests
      • npm run lint Runs all linters
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
  • Documentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work

Conditionally migrate IDB data only if it already exists. If the project
has never used an older version of Glean, then we want to avoid
migration because of a potential race condition when submitting pings on
first page load.
@rosahbruno rosahbruno force-pushed the 1848200-conditional-migration branch from 8c3273d to 5183342 Compare August 10, 2023 19:59
@rosahbruno rosahbruno marked this pull request as ready for review August 10, 2023 20:17
@auto-assign auto-assign bot requested a review from Dexterp37 August 10, 2023 20:17
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

What happens if Local Storage is cleared? will this continue to try and migrate metrics? How long does the migration code need to be in tree, have you considered this and planned (and filed a bug) to remove it at some time in the future?

glean/src/core/glean/sync.ts Show resolved Hide resolved
@rosahbruno
Copy link
Contributor Author

rosahbruno commented Aug 11, 2023

@travis79

What happens if Local Storage is cleared? will this continue to try and migrate metrics?

If LocalStorage is cleared, it will try and run just once. Currently, we aren't migrating most metrics - we are just migrating client_id and first_run_date. So on a second migration the flag will be set and the values will be set to the same thing as before.

How long does the migration code need to be in tree, have you considered this and planned (and filed a bug) to remove it at some time in the future?

That is something I've been thinking about. I'd like to maybe chat with you during our next 1:1 to get some thoughts here. In the mean time, I will file a bug so that this isn't lost in the shuffle.

@Dexterp37
Copy link
Contributor

Dexterp37 commented Aug 11, 2023

How long does the migration code need to be in tree, have you considered this and planned (and filed a bug) to remove it at some time in the future?

That is something I've been thinking about. I'd like to maybe chat with you during our next 1:1 to get some thoughts here. In the mean time, I will file a bug so that this isn't lost in the shuffle.

Only as long as MDN hasn't migrated (or any other project requiring migration).

Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

Okay, let's discuss in our next 1:1 how to track when everyone using Glean.js has migrated to the new version and we will file a bug about cleaning up the migration code then! Other than that, this looks good to me.

@rosahbruno rosahbruno merged commit 7675462 into mozilla:main Aug 11, 2023
9 checks passed
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.

None yet

3 participants