Page MenuHomePhabricator
Create Task
Maniphest T232040

Add label and description collision detectors for new terms store
Closed, ResolvedPublic
Actions

Description

Context
TermSqlIndex is still the only implementation of LabelConflictFinder that is used to find conflicts in labels and descriptions when creating properties and items, that is used in few places such as LabelUniquenessValidator.

Problem
We will not be able to stop writing to both term stores before we provide an implementation that looks up possible conflicts in the new store.

Solution
Provide a new means (interfaces + implementations) that allows writing uniqueness validators using the new store

Check if there are indexes on new tables to support this.

Todo

  • allow validating on ChangeOpResult for optimized validation on only changing parts patch for review
  • add ChangeOpFingerprint and ChangeOpFingerprintResult to allow identifying and attaching validators to only fingerprint changes patch for review
  • use ChangeOpFingerprint everywhere to wrap change ops to terms patch for review
  • wire up PrefetchingItemTermLookup
  • add implementation for detecting label and label+descripton collisions patch for review
  • add validator implementations and wire them up when writing to new store (migration values anything other than MIGRATE_OLD) patch for review
  • fix SingleEntitySourceServices::getPrefetchingTermLookup to respect item terms migration stages WIP patch
  • investigate the usage of SingleEntitySourceServices::getTermSearchInteractorFactory and, if necessary, update it to respect property and item term migration stages
  • change SpecialNewEntity so that it uses ChangeOp instead of creating an entity and saving it directly using EditEntity.
  • call ChangeOpResult::validate in EditEntity right after obtaining it patch for review

Details

SubjectRepoBranchLines +/-
wbterms: Update SingleEntitySourceServices PrefetchingTermLookupmediawiki/extensions/Wikibasewmf/1.35.0-wmf.14+140 -75
Validate ChangeOpResult in ModifyEntitymediawiki/extensions/Wikibasemaster+633 -112
wbterms: Update SingleEntitySourceServices PrefetchingTermLookupmediawiki/extensions/Wikibasemaster+139 -76
Update SingleEntitySourceServices to respect item terms migration stagesmediawiki/extensions/Wikibasemaster+117 -16
Add Fingerprint uniqueness validation based on new term storemediawiki/extensions/Wikibasemaster+1 K -151
Add Fingerprint uniqueness validation based on new term storemediawiki/extensions/Wikibasemaster+1 K -151
Add TermsCollisionDetector and db implementations for properties and itemsmediawiki/extensions/Wikibasemaster+774 -0
Use ChangeOpFingerprint to wrap fingerprint (terms) change operationsmediawiki/extensions/Wikibasemaster+295 -86
Add ChangeOpFingerprint and ChangeOpFingerprintResult classesmediawiki/extensions/Wikibasemaster+253 -0
Add ChangeOpResult::validatemediawiki/extensions/Wikibasemaster+84 -91
Customize query in gerrit

Related Objects
Search...

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
• alaa_wmde claimed this task.Oct 30 2019, 1:31 PM
• alaa_wmde moved this task from Ready to pick up to Wikidata-Campsite-Iteration-∞ (On Hold) on the Wikidata-Campsite board.
• alaa_wmde edited projects, added Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)); removed Wikidata-Campsite.
• alaa_wmde moved this task from To Do (prioritised from top to bottom) to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Oct 30 2019, 3:05 PM
• alaa_wmde updated the task description. (Show Details)Oct 30 2019, 3:08 PM
• alaa_wmde updated the task description. (Show Details)Oct 30 2019, 10:17 PM
• alaa_wmde updated the task description. (Show Details)
• alaa_wmde moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.

first patch for collision detector interfance+implementations is up for review

Addshore added a parent task: T236979: Fix remaining usecases still reading from old wb_terms schema.Oct 31 2019, 7:38 AM
Addshore mentioned this in T208425: [EPIC] Kill the wb_terms table.Oct 31 2019, 7:44 AM
• alaa_wmde moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 1 2019, 8:23 AM
• alaa_wmde moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 3 2019, 2:22 PM

First patch reworked and ready for peer review again

• alaa_wmde moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 7 2019, 10:06 AM

I'm doing some cleanup work on first patch now

• alaa_wmde moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 7 2019, 7:26 PM

first patch ready for review again .. left some comments where I'd love to have a second (or more) thought

• alaa_wmde moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.EditedNov 11 2019, 10:43 PM
• alaa_wmde added subscribers: Lucas_Werkmeister_WMDE, Ladsgroup, Addshore, hoo.

@Ladsgroup @Addshore @Lucas_Werkmeister_WMDE @hoo

For the new store, we are saving terms in a deferred update. This makes uniqueness checks:
a) if done during request, they might not catch possible collisions with values that are in deferred updates at the moment but haven't been written to db yet.
b) if done as part of the deferred update, they don't have the chance to report back collisions to the user.

Our options are:

  1. live with (a) and believe that those race conditions are rare. Pretty bad as it introduces uniqueness-breaches defying the whole purpose of those checks.
  2. live with (b) and believe that those race conditions are rare. This is pretty bad too, as it won't report collisions with already persisted terms.
  3. do both (a) and (b) .. extra load, can solve both issues with (a) and (b) alone, except for when a collision is going to happen between current edit and some deferred update. In that case, we only avoid breaking uniqueness but the user won't know.
  4. move saving terms to the request, instead of doing it in a deferred update. not sure how bad that is yet. can we tell already how longs those queries are taking already (from some db stats)? or will we need to setup a separate monitoring to know that?
  5. During request A, we store hashed fingerprint in suitable cache, and then deferred update A would remove it when finished. That way, request B can check during request both db and cache (with its hashed fingerprint). Probably the best option of both worlds, but need your opinions.
  6. Store hashed fingerprints in a separate table that is used only for uniqueness checks. extra storage, but is it going to be too much extra? if hash size is 512 bits then we would need for the 70m items we currently have: ( 512 bits for hash + 64 bits for entity id) * 70m ~= 5GB which sounds a lot.
  7. ... any other options you see?
Lucas_Werkmeister_WMDE added a subscriber: Lydia_Pintscher.Nov 12 2019, 11:47 AM

For the new store, we are saving terms in a deferred update.

This is also true for the old term store, I believe. I’m inlined to go with option 1 (which as I understand it is the status quo).

  1. live with (a) and believe that those race conditions are rare. Pretty bad as it introduces uniqueness-breaches defying the whole purpose of those checks.

Well, what is the purpose of those checks? Is it really wholly defeated by even rare race conditions? I view it more as a warning mechanism to the user than an absolute invariant of the data whose integrity must be preserved under all circumstances, so I think those rare race conditions would be acceptable. (That might be a question for @Lydia_Pintscher.)

Ladsgroup added a comment.Nov 12 2019, 11:49 AM

As I mentioned in person, given that DefferedUpdates happen practically at the same time of the edit, I'm fine with 1. If it was done in a job, I would be against it.

• alaa_wmde added a comment.Nov 12 2019, 12:21 PM

True. Option 1 is the status quo, and since those race-conditions are quite rare to begin with, and the point made by @Lucas_Werkmeister_WMDE about this validation not being a strict integrity check (@Lydia_Pintscher please correct us if we are making wrong assumptions here) sounds reasonable, I will continue with option 1 for now.

Thanks for the input!

Lydia_Pintscher added a comment.Nov 12 2019, 7:47 PM

Yeah it's not a super strict integrity check but if we can avoid Items and Properties with the same label/description combination we really should try.

gerritbot added a comment.Nov 13 2019, 3:24 PM

Change 550703 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/Wikibase@master] Add ChangeOpResult::validate

https://gerrit.wikimedia.org/r/550703

gerritbot added a comment.Nov 13 2019, 6:01 PM

Change 550730 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/Wikibase@master] Add ChangeOpFingerprint and ChangeOpFingerprintResult classes

https://gerrit.wikimedia.org/r/550730

• alaa_wmde moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Nov 22 2019, 2:20 PM

First patch is ready for review again

• alaa_wmde updated the task description. (Show Details)Nov 22 2019, 2:22 PM
• alaa_wmde updated the task description. (Show Details)Nov 22 2019, 2:45 PM
• alaa_wmde updated the task description. (Show Details)Nov 25 2019, 8:51 PM
gerritbot added a comment.Nov 25 2019, 10:39 PM

Change 552905 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/Wikibase@master] [WIP] Use ChangeOpFingerprint to wrap fingerprint change operations wherever created.

https://gerrit.wikimedia.org/r/552905

• alaa_wmde updated the task description. (Show Details)Nov 26 2019, 11:32 AM
• alaa_wmde updated the task description. (Show Details)Nov 26 2019, 11:43 AM
gerritbot added a comment.Nov 26 2019, 11:49 PM

Change 553213 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/Wikibase@master] Add Fingerprint uniqueness validation based on new term store

https://gerrit.wikimedia.org/r/553213

• alaa_wmde updated the task description. (Show Details)Nov 27 2019, 2:18 PM
gerritbot added a comment.Nov 27 2019, 4:57 PM

Change 550703 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add ChangeOpResult::validate

https://gerrit.wikimedia.org/r/550703

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.10; 2019-12-10).Nov 27 2019, 5:00 PM
• alaa_wmde updated the task description. (Show Details)Nov 28 2019, 10:07 PM
• alaa_wmde updated the task description. (Show Details)Nov 29 2019, 12:03 AM
gerritbot added a comment.Nov 29 2019, 12:16 AM

Change 553524 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/Wikibase@master] Validate ChangeOpResult in EditEntity

https://gerrit.wikimedia.org/r/553524

• alaa_wmde updated the task description. (Show Details)Nov 29 2019, 12:16 AM
• alaa_wmde updated the task description. (Show Details)
gerritbot added a comment.Dec 3 2019, 12:03 PM

Change 550730 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add ChangeOpFingerprint and ChangeOpFingerprintResult classes

https://gerrit.wikimedia.org/r/550730

gerritbot added a comment.Dec 3 2019, 5:20 PM

Change 552905 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use ChangeOpFingerprint to wrap fingerprint (terms) change operations

https://gerrit.wikimedia.org/r/552905

• alaa_wmde updated the task description. (Show Details)Dec 4 2019, 4:36 PM
• alaa_wmde updated the task description. (Show Details)Dec 4 2019, 6:10 PM
gerritbot added a comment.Dec 9 2019, 12:36 PM

Change 546898 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add TermsCollisionDetector and db implementations for properties and items

https://gerrit.wikimedia.org/r/546898

• alaa_wmde updated the task description. (Show Details)Dec 9 2019, 1:10 PM
gerritbot added a comment.Dec 9 2019, 9:57 PM

Change 556063 had a related patch set uploaded (by Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/Wikibase@master] Add Fingerprint uniqueness validation based on new term store

https://gerrit.wikimedia.org/r/556063

gerritbot added a comment.Dec 9 2019, 10:06 PM

Change 556063 abandoned by Sarhan:
Add Fingerprint uniqueness validation based on new term store

Reason:
duplicate

https://gerrit.wikimedia.org/r/556063

sarhan.alaa claimed this task.Dec 10 2019, 9:52 AM
gerritbot added a comment.Dec 10 2019, 3:29 PM

Change 553213 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add Fingerprint uniqueness validation based on new term store

https://gerrit.wikimedia.org/r/553213

sarhan.alaa updated the task description. (Show Details)Dec 12 2019, 9:48 PM

Last patch turning on validation of uniqueness in new store is ready for review now
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/553524
It has 3 little fixes below it too

Ladsgroup moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Dec 18 2019, 7:16 PM
sarhan.alaa added a comment.Dec 19 2019, 10:46 AM

Not sure @Ladsgroup your comments in the review do make sense.. can you please double check them and read through the code again? I may of course have misunderstood them, maybe little more elaboration will help

sarhan.alaa moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Dec 19 2019, 10:46 AM
Addshore closed subtask T239839: Wire up PrefetchingItemTermLookup as Resolved.Dec 23 2019, 1:02 PM
sarhan.alaa added a comment.EditedDec 30 2019, 12:36 PM

@Addshore @Ladsgroup
Are we waiting on this until we start reading a range (first 1000 items iirc) from new store? Is there more code review to be done?

In case there are any questions or concerns that are easier to be done directly in a call/meeting just let me know and I can be available.

sarhan.alaa added a comment.Dec 30 2019, 12:45 PM

If reviewing the patch in the current state is too much effort or difficult (it can be very much the case), also let me know I can try to split it up further into couple of smaller patches

Ladsgroup added a comment.Dec 30 2019, 10:45 PM

I'm on vacation until next Monday.

Addshore added a comment.EditedJan 3 2020, 7:28 PM

While reviewing https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Wikibase/+/553524/ I noticed that both at least 2 more things need fixing:

  • SingleEntitySourceServices::getTermSearchInteractorFactory
  • SingleEntitySourceServices::getPrefetchingTermLookup

Until that happens its hard to really verify the final patch etc.

sarhan.alaa moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jan 5 2020, 2:30 PM
In T232040#5774510, @Addshore wrote:

While reviewing https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Wikibase/+/553524/ I noticed that both at least 2 more things need fixing:

  • SingleEntitySourceServices::getTermSearchInteractorFactory
  • SingleEntitySourceServices::getPrefetchingTermLookup

Until that happens its hard to really verify the final patch etc.

I will look into this tomorrow

Addshore mentioned this in T241971: useEntitySourceBasedFederation true by default in Wikibase.Jan 6 2020, 9:20 AM
Addshore added a comment.Jan 6 2020, 9:25 AM

I just filed a chain of tickets under T241971 to finally finish switching over to the new service containers too.

sarhan.alaa added a comment.EditedJan 6 2020, 10:32 AM

I added two test cases to the integration test editentity fingerprint uniqueness. They pass for API calls, which I could also confirm sending API requests manually.

But, I can also reproduce it through special pages: I can create conflicting properties (didn't test items). I will next investigate the mentioned SingleEntitySourceServices::getTermSearchInteractorFactory and SingleEntitySourceServices::getPrefetchingTermLookup.

sarhan.alaa added a comment.Jan 6 2020, 11:07 AM

Same goes for items. API calls fail manually as expected, but I can create conflicting items through special pages. I also confirm that SingleEntitySourceServices::getTermSearchInteractorFactory and SingleEntitySourceServices::getPrefetchingTermLookup need fixing. Will be working on that this evening.

note: Seems like that API calls somehow are configure to respect federation, using PerReopsitoryServiceWiring that was updated to respect item migration phase too in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/554562, but Special Page would use SingleEntitySourceServices. Though conflicting property creation should have failed as SingleEntitySourceServices was updated to respect property migration stage. So there could be more buggy code down the call stack.

gerritbot added a comment.Jan 6 2020, 6:32 PM

Change 562320 had a related patch set uploaded (by Sarhan; owner: Sarhan):
[mediawiki/extensions/Wikibase@master] Update SingleEntitySourceServices to respect item terms migration stages

https://gerrit.wikimedia.org/r/562320

sarhan.alaa updated the task description. (Show Details)Jan 6 2020, 6:35 PM
sarhan.alaa added a comment.Jan 6 2020, 8:16 PM

Oh, apparently SpecialNewEntity creates entities without even creating change ops. That would explain it, as then it will by-pass the whole validation of fignreprint uniqueness that is only executed inside ChangeOpFingerprintResult::validate(). Guess that needs to change too.

sarhan.alaa updated the task description. (Show Details)Jan 6 2020, 8:17 PM
Addshore mentioned this in T242095: TermSearchInteractor should have a new implementation supporting new terms storage.Jan 7 2020, 11:18 AM
Addshore mentioned this in T242096: SingleEntitySourceServices PrefetchingTermLookup should use new terms storage.
Addshore removed sarhan.alaa as the assignee of this task.Jan 7 2020, 11:21 AM
Addshore claimed this task.
Addshore added a subscriber: sarhan.alaa.
Restricted Application added a project: User-Addshore. · View Herald TranscriptJan 7 2020, 11:21 AM
Addshore added subtasks: T242096: SingleEntitySourceServices PrefetchingTermLookup should use new terms storage, T242095: TermSearchInteractor should have a new implementation supporting new terms storage.Jan 7 2020, 11:38 AM
Addshore added a comment.Jan 7 2020, 11:41 AM

Added the 2 things we found as sub tasks as they need to be done to be able to test this code in WRITE_NEW only

Addshore moved this task from Unsorted 💣 to Active 🚁 on the User-Addshore board.Jan 9 2020, 9:49 AM
gerritbot added a comment.Jan 9 2020, 4:22 PM

Change 563204 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@master] Update SingleEntitySourceServices to respect item terms migration stages

https://gerrit.wikimedia.org/r/563204

gerritbot added a comment.Jan 9 2020, 4:23 PM

Change 563204 abandoned by Addshore:
Update SingleEntitySourceServices to respect item terms migration stages

https://gerrit.wikimedia.org/r/563204

gerritbot added a comment.Jan 10 2020, 7:43 AM

Change 562320 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] wbterms: Update SingleEntitySourceServices PrefetchingTermLookup

https://gerrit.wikimedia.org/r/562320

gerritbot added a comment.Jan 13 2020, 8:39 AM

Change 563958 had a related patch set uploaded (by Ladsgroup; owner: Addshore):
[mediawiki/extensions/Wikibase@wmf/1.35.0-wmf.14] wbterms: Update SingleEntitySourceServices PrefetchingTermLookup

https://gerrit.wikimedia.org/r/563958

Addshore closed subtask T242204: Update SpecialNewProperty and SpecialNewItem to use new store conflict detection as Resolved.Jan 14 2020, 9:47 AM
Addshore moved this task from Doing to Test (Verification) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jan 14 2020, 9:58 AM
gerritbot added a comment.Jan 14 2020, 10:34 AM

Change 553524 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Validate ChangeOpResult in ModifyEntity

https://gerrit.wikimedia.org/r/553524

ReleaseTaggerBot edited projects, added MW-1.35-notes (1.35.0-wmf.15; 2020-01-14); removed MW-1.35-notes (1.35.0-wmf.10; 2019-12-10).Jan 14 2020, 11:00 AM
gerritbot added a comment.Jan 14 2020, 11:20 AM

Change 563958 abandoned by Addshore:
wbterms: Update SingleEntitySourceServices PrefetchingTermLookup

Reason:
Abandoning as I figure we are not backporting this!

https://gerrit.wikimedia.org/r/563958

Ladsgroup mentioned this in T242880: Create a decorator delagating MatchingTermsLookup for the old and the new term store..Jan 15 2020, 4:08 PM
Addshore moved this task from Test (Verification) to Done on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jan 15 2020, 4:59 PM

Verified on beta

Addshore closed this task as Resolved.Jan 15 2020, 4:59 PM
Addshore closed subtask T242096: SingleEntitySourceServices PrefetchingTermLookup should use new terms storage as Resolved.Jan 15 2020, 5:08 PM
Addshore removed a subtask: T242095: TermSearchInteractor should have a new implementation supporting new terms storage.
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL