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.

Sign up for GitHub

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

Jump to bottom

Full Sync: Don't allow more than one request to enqueue #14039

Merged
merged 8 commits into from
Nov 15, 2019
Merged

Full Sync: Don't allow more than one request to enqueue #14039

merged 8 commits into from
Nov 15, 2019

Conversation

lezama
Copy link
Contributor

@lezama lezama commented Nov 14, 2019 •
edited
Loading

This PR introduces a Lock so a site can't have two process enqueuing and modifying the enqueue status at the same time.
In consequence it allow us to remove unnecessary rechecks like can_add_to_queue while looping through the different Sync Modules.

This PR is extracting improvements we did in #13963 that are not related to the main problem that other PR is trying to solve.

How to Test

  • Make sure that generically full sync still works
  • Tests are passing
  • Add logging statements to Full_Sync->enqueue method and make it sleep() for some seconds.
  • Start a full sync
  • Reload wp-admin a couple of times
  • Make sure enque is not hit twice concurrently
    🤔

@lezama lezama requested a review from a team November 14, 2019 17:59
@lezama lezama requested a review from a team as a code owner November 14, 2019 17:59
@lezama lezama self-assigned this Nov 14, 2019
@lezama lezama added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 14, 2019
@jetpackbot
Copy link

jetpackbot commented Nov 14, 2019 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against d48faba

@lezama lezama added this to the 8.0 milestone Nov 14, 2019
@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 15, 2019
@kraftbj
Copy link
Contributor

kraftbj commented Nov 15, 2019

I tested per the instructions and tried to trick it up a bit through scheduling full syncs via the debugger. No errors, the syncs appeared complete.

@lezama lezama merged commit 28904d8 into master Nov 15, 2019
@lezama lezama deleted the update/full-sync-add-lock branch November 15, 2019 19:04
@matticbot matticbot added [Status] Needs Changelog [Status] Needs Package Release This PR made changes to a package. Let's update that package now. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 15, 2019
@lezama
Copy link
Contributor Author

lezama commented Nov 15, 2019

Thank you @kraftbj !

gravityrail pushed a commit that referenced this pull request Nov 16, 2019
@robertf4 @gravityrail
Add SingleProductBackupBody Component (#14027)
d08f20a 
* Add products endpoint

* [not verified] Add error code

* Update _inc/lib/class.core-rest-api-endpoints.php

Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* Update _inc/lib/class.core-rest-api-endpoints.php

Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* Add QueryProducts data component and corresponding Redux and rest API calls

* Update _inc/client/state/site/reducer.js

Co-Authored-By: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* [not verified] Add SingleProductBackupBody component

* [not verified] Rename plans-section__body to single_product_backup__body

* [not verified] Give class name to h4 tag

* [not verified] Remove single_product_backup__body

* [not verified] Check for upgradeLinks and upgradeTitles before using them

* [not verified] Switch function style

* [not verified] Add key to PlanRadioButton

* Remove duplicate definition of getProducts

* Allow TOS agreement before Jetpack is fully active so we track… (#14041)

* Allow TOS agreement before Jetpack is fully active so we track the connection flow

* Update packages/terms-of-service/src/class-terms-of-service.php

Co-Authored-By: Derek Smart <smart@automattic.com>

* Also update the action that is being called in jetpack

We renamed the action lets also rename it in Jetpack.

* [not verified] Revert "Also update the action that is being called in jetpack"

This reverts commit e9c0f21.


Co-authored-by: Brandon Kraft <public@brandonkraft.com>
Co-authored-by: Enej Bajgoric <enej.bajgoric@gmail.com>

* Removed Jetpack references in the IXR client. (#14046)

This change allows the package to be used outside of Jetpack, relying on the methods provided by the Manager, plus using the WP_Error class instead of the Jetpack_Error wrapper.

* Full Sync: Don't allow more than one request to enqueue (#14039)

* Spell checking CI integration (#13992)

* Adds spell checking and fixes files.

This implements a package to spellcheck files, and
does a pass to ensure everything is green.

* [not verified] Implements Travis check for spelling

Adds Yarn script and Travis CI for spell checking.

* [not verified] Fixes feedback.

Updates docs to prevent spelling exceptions for certain files.

* Export SingleProductBackupBody class

* [not verified] No need to export single product backup body
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Status] Needs Changelog labels Nov 19, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
@jeherve
Changelog: add #14039
271440c
jeherve added a commit that referenced this pull request Nov 25, 2019
@jeherve
8.0 Release: running changelog (#13964)
807ab89 
* 8.0 Release: running changelog

* Changelog: add #13921

* Changelog: add #13980

* Changelog: add #13905

* Changelog: add #13971

* Changelog: add #13984

* Changelog: add #14009

* Changelog: add #13620

* Remove things that will ship in 7.9.1

* Changelog: add 7.9.1 release (#14044)

* Changelog: add base for 7.9.1 release

* Update release date and post link

* Changelog: add #14066

* Update changelog for 7.9.1

* Changelog: add #13405

* Changelog: add #13841

* Changelog: add #13924

* Changelog: add #13986

* Changelog: add #14010, #14028, #14053, #14055.

* Changelog: add #14054

* Changelog: add #14031

* Changelog: add #14039

* Changelog: add #14050

* Changelog: add #14070

* Changelog: add #14082

* Changelog: add #14084

* Changelog: add #14111

* Changelog: add #13961

* Changelog: add #14047

* Changelog: add #14091

* Changelog: add #14108

* Changelog: add #14121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enejb enejb enejb left review comments

@kraftbj kraftbj kraftbj approved these changes

Assignees

@lezama lezama

@mdbitz mdbitz

Labels
[Package] Sync [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.
7 participants
@lezama @jetpackbot @kraftbj @enejb @jeherve @mdbitz @matticbot