Skip to content

Conversation

@tyxla
Copy link
Member

@tyxla tyxla commented Jul 17, 2019

This implements syncing of the term relationships in full sync, which currently we don't seem to be doing.

Turns out it is a bit specific use case, because that table the only one that doesn't have a unique ID column. So to be able to still chunk properly like we do for other sync modules, we use 2 columns, and wrap the functionality in a new sync module, Term_Relationships.

We're introducing some tests to verify that missing or obsolete term relationships are synced, and add some tests to the full sync to ensure that it's properly syncing all term relationships, and is properly chunking them when there are a lot of them to sync.

Changes proposed in this Pull Request:

  • Sync: Full sync the term relationships

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Part of the effort to fix term syncing and the overall state of sync.

Testing instructions:

  • Apply D30623-code to your WP.com sandbox.
  • Apply this PR to your local Jetpack dev env.
  • Enable WP_DEBUG_LOG on your site if it's disabled.
  • Silently create a term relationship between a post and a term on your site (see below for how to do that).
  • Trigger a full sync.
  • Make sure the term relationship got synced to the cache site.
  • Verify full and incremental sync for other items still works.
  • Try testing with some other use cases you can think of?
  • Verify the tests pass.
  • Verify there are no errors.

Additional testing help

  • Silent creation term relationship can be done with this piece of code:
global $wpdb;
$wpdb->insert(
	$wpdb->term_relationships,
	array(
		'object_id'        => 1234,
		'term_taxonomy_id' => 123,
		'term_order'       => 0,
	),
	array(
		'%d',
		'%d',
		'%d',
	)
);

Proposed changelog entry for your changes:

  • Sync: Full sync the term relationships

@tyxla tyxla added this to the 7.6 milestone Jul 17, 2019
@tyxla tyxla requested a review from a team July 17, 2019 15:31
@tyxla tyxla requested a review from a team as a code owner July 17, 2019 15:31
@tyxla tyxla self-assigned this Jul 17, 2019
@jetpackbot
Copy link
Collaborator

jetpackbot commented Jul 17, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against abf3714

@tyxla tyxla added [Status] Needs Review This PR is ready for review. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] In Progress labels Jul 19, 2019
@tyxla
Copy link
Member Author

tyxla commented Jul 19, 2019

This is ready for review now.

// phpcs:ignore WordPress.CodeAnalysis.AssignmentInCondition.FoundInWhileCondition, WordPress.DB.PreparedSQL.InterpolatedNotPrepared
while ( $ids = $wpdb->get_results( "SELECT object_id, term_taxonomy_id FROM $wpdb->term_relationships WHERE object_id <= {$previous_interval_end['object_id']} AND term_taxonomy_id < {$previous_interval_end['term_taxonomy_id']} ORDER BY object_id DESC, term_taxonomy_id DESC LIMIT {$items_per_page}", ARRAY_A ) ) {
// Request term relationships in groups of N for efficiency.
$chunked_ids = array_chunk( $ids, self::ARRAY_CHUNK_SIZE );
Copy link
Member Author

Choose a reason for hiding this comment

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

The bit in the while is kind of the same as the one in Automattic\Jetpack\Sync\Modules\Module::enqueue_all_ids_as_action, so we can eventually refactor them to share the same method, but it feels like this can be done in a follow-up PR, hence the @todo in the docblock.

Copy link
Contributor

@roccotripaldi roccotripaldi 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 good. Thanks for working on it.

One thing is missing from the testing instructions:

  • you need to set JETPACK__API_BASE to your sandbox in jetpack.php

After testing, i realized we had to make a small change to D30623.

It works well when you silently add a term_relationship: I see it appear in the cache site after a full sync.

When i silently delete a term_relationship, the entry remains on the cache site after a full sync.

I think we can fix that on the server-side, so you have my approval.

@roccotripaldi
Copy link
Contributor

This PR is not needed any more. In D30684-code, we ensured that we were properly getting term relationships during a full sync.

@lezama
Copy link
Contributor

lezama commented Jul 23, 2019

Not 100% sure about closing it, I think it might be useful to have it anyways, even if full syncing posts should fix terms, maybe "fullsyncing" just this table could be faster in some cases, plus we might need some of the logic here for the validator, what do you think?

@lezama lezama reopened this Jul 23, 2019
@jeherve jeherve removed this from the 7.6 milestone Jul 25, 2019
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jul 25, 2019
@gravityrail
Copy link
Contributor

@lezama @tyxla @roccotripaldi

Let's merge this, even if we don't use it right now, just in case we discover we need to revert to this approach during testing with very large sites on VIP.

@gravityrail gravityrail added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jul 26, 2019
@gravityrail gravityrail added this to the 7.6 milestone Jul 26, 2019
Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Although this might need a lint pass after, let's do it in another PR.

@zinigor zinigor merged commit 9db34fc into master Jul 29, 2019
@zinigor zinigor deleted the try/full-sync-term-relationships branch July 29, 2019 13:12
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 29, 2019
jeherve added a commit that referenced this pull request Jul 29, 2019
jeherve added a commit that referenced this pull request Jul 30, 2019
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
jeherve added a commit that referenced this pull request Jul 30, 2019
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Sync [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants