Add database term ids acquirer #11

AlaaSarhan · 2019-05-09T20:45:35Z

Phab task: https://phabricator.wikimedia.org/T223963

Database implementation of TermIdsAcquirer.

The implementation acquires ids bottom-up (starting from wbt_text table towards wbt_term_in_lang table), inserting new records as needed. The pseudo steps for each table are:

find existing ids that can be reused on read database
insert new non-existing data into write database, if any
fetch inserted records' ids from write database, if any
use the acquired ids to link up with the next table

This implementation optimizes the number of queries, effectively making them 1 select per table on replicas, and if needed a follow up 1 insert followed by 1 select per table on master.

Todo:

test case for reusing existing TermInLangIds
handle insertion failures on duplicate insertions into dbw due to lagging replica
add logger
make code more readable where possible (minimum split the huge class per table functionality)
reduce duplication in code
check for low-performance pitfalls

Along with it, TypeIdsAcquirer interface with in-momory implementation were introduced. wbt_type is likely to be loaded and stored in memory, for which NameTableStore class in MW core can be used. So we will likely have implementation of TypeIdsAcquirer that uses NameTableStore class internally and pass it to this class.

src/PackagePrivate/TypeIdsAcquirer.php

src/PackagePrivate/DatabaseTermIdsAcquirer.php

mariushoch · 2019-05-10T09:05:47Z

(I haven't read up on the architecture, so this might not actually be applicable)

Are there unique indices on these tables (probably)? In that case, you need to make sure to handle race conditions (duplicate keys) where the replicas don't yet have rows already inserted on master…

(Unlikely) Are rows ever removed/ purged from these tables? If so, that needs to be handled here as well.

Also I find the functional style sometimes a little obstructive to read, especially the array_reduce class that probably could be replace by fairly simple foreach loops.

lucaswerkmeister · 2019-05-10T11:42:26Z

Are there unique indices on these tables (probably)? In that case, you need to make sure to handle race conditions (duplicate keys) where the replicas don't yet have rows already inserted on master…

Currently, there are, yes. (You can see them in AddNormalizedTermsTablesDDQ.sql, by the way.) We can either handle the error on the master, or make the indices non-unique again and tolerate duplicate rows. The latter is what MediaWiki does for the comment table:

-- Deduplication is currently best-effort to avoid locking on inserts that
-- would be required for strict deduplication. There MAY be multiple rows with
-- the same comment_text and comment_data.

(Unlikely) Are rows ever removed/ purged from these tables? If so, that needs to be handled here as well.

They are (implemented in #10) – after all, terms from deleted revisions might contain sensitive data that should not remain on the Cloud VPS replicas. (MediaWiki core solves this with a whole bunch of custom views for its own normalization tables, as far as I’m aware, but since wb_terms is only a secondary data store, we can completely delete the rows instead.)

lucaswerkmeister · 2019-05-10T12:29:39Z

We can either handle the error on the master, or make the indices non-unique again and tolerate duplicate rows.

Or we can query the master for IDs if we don’t find them on the replicas, and only insert if we don’t find the IDs on the master either, though that would probably need a FOR UPDATE on the master select, and it’s currently unclear if we should use that or not (see also discussion in #10).

AlaaSarhan

Also I find the functional style sometimes a little obstructive to read, especially the array_reduce class that probably could be replace by fairly simple foreach loops.

Interesting, I find reduce more readable than foreach, but maybe just because I'm more used to functional style. Fair point though, our paradigm is mainly OO so I'll happily change it so that it is more likely to be more readable by more Wikidata developers :)

src/PackagePrivate/DatabaseTermIdsAcquirer.php

lucaswerkmeister · 2019-05-10T12:56:51Z

src/PackagePrivate/DatabaseTermIdsAcquirer.php

+	 * @return array
+	 *	[
+	 *		'type' => [
+	 *			[ 'language' => [ <textId1>, <textId2>, ... ] ], ...


Given [ 'type' => [ 'language' => 'term' ] ], will this return [ 'type' => [ 'language' => 1 ] ] or [ 'type' => [ 'language' => [ 1 ] ] ]? That is, if the input has a single term instead of a list, is that preserved in the output?

it will not be preserved iirc, I convert them to arrays in the output even for single elements

I tried it out, it looks like it’s preserved after all (because array_walk_recursive preserves the structure) – I think that’s why DatabaseTermIdsAcquirerTest::assertTermsArrayExistInDb needs the cast (array)$texts too.

AlaaSarhan · 2019-05-10T13:44:11Z

We can either handle the error on the master, or make the indices non-unique again and tolerate duplicate rows.

Or we can query the master for IDs if we don’t find them on the replicas, and only insert if we don’t find the IDs on the master either, though that would probably need a FOR UPDATE on the master select, and it’s currently unclear if we should use that or not (see also discussion in #10).

We certainly do not want to allow duplicate rows here as the main purpose of this normalization is to remove duplication. We can query master for non existing ones, and only insert those that do not exist eventually, but yeah that's then another select query added.

In a previous implementation, we tried to use IGNORE option in the insert so that it ignores duplication errors and still proceed to insert the rest. Though the issue here is that it will increment the value for the next primary key even for those failing inserts. We wanted to avoid that too.

A third, in-the-middle solution can be to do the following for non-existing ids in replica:

prepare non existing records
insert into master
select ids of non existing records
if selected ids do not count as the ones we asked for, find again the new set of non-existing records (this then should exclude the ones that already exist and made the previous insert fail due to duplicate constraint violation) and go back to 2

what do you think?

lucaswerkmeister · 2019-05-10T13:50:43Z

We certainly do not want to allow duplicate rows here as the main purpose of this normalization is to remove duplication.

That’s the main purpose of the comment table too, and it still allows duplicate rows. In practice, those would be pretty rare (and we could even clean them up periodically if we really wanted to, though the query to find duplicate rows would probably be very inefficient), so I’m not yet convinced we should dismiss this idea.

I was going to ask about IGNORE – good that you already tested that :)

I don’t understand your third solution yet, can you clarify what “prepare” means and where which queries are run (except 2., which already says “master”)?

AlaaSarhan · 2019-05-10T14:02:14Z

That’s the main purpose of the comment table too, and it still allows duplicate rows. In practice, those would be pretty rare (and we could even clean them up periodically if we really wanted to, though the query to find duplicate rows would probably be very inefficient), so I’m not yet convinced we should dismiss this idea.

Sure, atm I feel that should still be the last resort.. but let's see which solution might be cheaper on performance and maintenance.

I don’t understand your third solution yet, can you clarify what “prepare” means and where which queries are run (except 2., which already says “master”)?

I'll put it in the full pseudo steps:

prepare a list of needed records -> needed
query needed records from replica with their ids -> existing
prepare a list of non-existing records in replica: needed - existing -> non_existing
insert non_existing into master
query non_existing from master with their ids -> existing_master
if count(existing_master) != count(non_exiting) then non_existing - existing_master -> non_existing and go to 4.

that is a loop solution, that accounts of inserted terms from in-parallel requests.. not sure how often that will happen, but one run of the loop is at least needed in case the very first set of non-existing records in replica might have some of the records in master already

mariushoch · 2019-05-10T16:01:12Z

That’s the main purpose of the comment table too, and it still allows duplicate rows. In practice, those would be pretty rare (and we could even clean them up periodically if we really wanted to, though the query to find duplicate rows would probably be very inefficient), so I’m not yet convinced we should dismiss this idea.

Sure, atm I feel that should still be the last resort.. but let's see which solution might be cheaper on performance and maintenance.

I don’t understand your third solution yet, can you clarify what “prepare” means and where which queries are run (except 2., which already says “master”)?

I'll put it in the full pseudo steps:
1. prepare a list of needed records -> `needed`

2. query `needed` records from replica with their ids -> `existing`

3. prepare a list of non-existing records in replica: `needed - existing -> non_existing`

4. insert `non_existing` into master

This needs to INSERT IGNORE to guard against races.

5. query `non_existing` from master with their ids -> `existing_master`

I think here you need to query all needed (and lock them until you "use" them), as some might have been/ will be deleted from master.

6. if `count(existing_master) != count(non_exiting)` then `non_existing - existing_master -> non_existing` and go to 4.
that is a loop solution, that accounts of inserted terms from in-parallel requests.. not sure how often that will happen, but one run of the loop is at least needed in case the very first set of non-existing records in replica might have some of the records in master already

In generally you will have to make sure all entries (rows) you want to refer to are present (and ensure that by locking them on the master) until you put your references in place (so that no one will delete them because they are not referred to from anywhere).

If doing all of that in a transaction is a performance problem, you could go for a multi step process: 1) Find/ create entries you want to reference 2) put references in place 3) make sure the entries you just referenced are still there 4) fix now invalid references if needed. This should work fairly well, unless the runner dies after step 2) and you're not in a transaction.

I know that this is all fairly ugly, but doing this without using proper foreign keys is not easy (unless you heavily compromise on consistency, I guess).

AlaaSarhan · 2019-05-10T16:32:42Z

Thanks @mariushoch

I will then try to sleep on it a bit and check whether there's a "simpler" solution that can be done. (top of my head screams at me "staging table", no clue what it means though so hope I will shut it up with some reasoning haha)

AlaaSarhan · 2019-05-13T05:22:51Z

@mariushoch FYI

I'm continuing with this to finish it first without considering the race-condition problem (= assuming there's no cleanup logic running in-parallel). We will get back to race-conditions between clean up #10 and acquirer logic, and try to reproduce in a test, once both logics are merged and tested to be functional in test env.

mariushoch · 2019-05-13T09:20:45Z

I'm continuing with this to finish it first without considering the race-condition problem (= assuming there's no cleanup logic running in-parallel).

Sounds good to me :) Do you plan to handle insertion failures already (duplicate key) or not yet?

AlaaSarhan · 2019-05-13T09:24:38Z

@mariushoch yup I'm going to handle that, while implementing the logic I stated in the latest pseudo steps more or less

lucaswerkmeister · 2019-05-13T11:51:30Z

src/PackagePrivate/DatabaseTermIdsAcquirer.php

+	 * @return array
+	 *	[
+	 *		'type' => [
+	 *			[ 'language' => [ <textId1>, <textId2>, ... ] ], ...


I tried it out, it looks like it’s preserved after all (because array_walk_recursive preserves the structure) – I think that’s why DatabaseTermIdsAcquirerTest::assertTermsArrayExistInDb needs the cast (array)$texts too.

lucaswerkmeister · 2019-05-13T11:55:19Z

src/PackagePrivate/DatabaseTermIdsAcquirer.php

+				return $carry;
+			},
+			[]
+		);


I think this whole reduce business can be replaced with:

$existingTextIds = $this->dbr->selectFieldValues( self::TABLE_TEXT, 'wbx_id', [ 'wbx_text' => $texts ], __METHOD__ // unrelated but also nice );

lucaswerkmeister · 2019-05-13T11:55:37Z

src/PackagePrivate/DatabaseTermIdsAcquirer.php

+			}, $nonExistingTexts )
+		);
+
+		$insertedTextRows = $this->dbw->select(


Can also use selectFieldValues(), I think.

lucaswerkmeister · 2019-05-13T11:55:44Z

src/PackagePrivate/DatabaseTermIdsAcquirer.php

+			}
+		}
+
+		$existingLangTextIdRows = $this->dbr->select(


Can also use selectFieldValues(), I think.

src/PackagePrivate/TypeIdsAcquirer.php

lucaswerkmeister · 2019-05-13T12:03:11Z

tests/Unit/PackagePrivate/DatabaseTermIdsAcquirerTest.php

+
+					$this->assertNotEmpty(
+						$textId,
+						sprintf( 'Expected record for text "%s" was not found in wbt_text', $text )


Why sprintf instead of normal string interpolation? "Expected record for text '$text' was not found in wbt_text"

lucaswerkmeister · 2019-05-13T12:03:40Z

tests/Unit/PackagePrivate/DatabaseTermIdsAcquirerTest.php

+
+		$acquiredTermIds = $dbTermIdsAcquirer->acquireTermIds( $termsArray );
+
+		$this->assertTermSArrayExistInDb( $termsArray, $alreadyAcquiredTypeIds );


TermS → Terms

lucaswerkmeister · 2019-05-13T12:05:30Z

tests/Unit/PackagePrivate/DatabaseTermIdsAcquirerTest.php

+
+		$acquiredTermIds = $dbTermIdsAcquirer->acquireTermIds( $termsArray );
+
+		$this->assertEquals(


Would be nicer to use assertSame() instead of assertEquals() (this also applies to two more places below).

It might also be worth it extracting a separate function, à la $this->assertRowCount( 'wbt_text', '*' ).

AlaaSarhan · 2019-05-15T09:42:18Z

I'm close to wanting this to be reviewed and merged to not keep it open forever .. I will today go over it to add a separate tests for the new class ReplicaMasterAwareRecordIdsAcquirer (perhaps renaming the class if anyone have a better suggestion) and a test case for duplication failures on master insertions .. but I'd like to stop expanding this PR at this point and then we have a look again on how different things can be improved/continued in-parallel/separately

AlaaSarhan · 2019-05-15T09:43:08Z

I will also respond to the reviews from before of course ;)

lucaswerkmeister

not a full review, but some hopefully useful comments

src/PackagePrivate/Util/ReplicaMasterAwareRecordIdsAcquirer.php

src/PackagePrivate/TypeIdsAcquirer.php

src/PackagePrivate/Util/ReplicaMasterAwareRecordIdsAcquirer.php

mariushoch · 2019-05-15T22:48:36Z

src/PackagePrivate/Util/ReplicaMasterAwareRecordIdsAcquirer.php

+				$existingRecords,
+				$this->findExistingRecords( $this->dbMaster, $neededRecords )
+			);
+			$neededRecords = $this->filterNonExistingRecords( $neededRecords, $existingRecords );


When would this be non-empty? AFAIS only if something you just inserted into master got deleted… is it really worth guarding just for this specific case? (If you want to be safe in all cases, you need to SELECT … FOR UPDATE all record-rows on master)

this would be non-empty in the following case:
replica has records 1, 2 but not 3, 4 and 5
master has records 3 and 4 but not 5 (the previous insert will fail due to 3 and 4 duplicatation)
there fore 5 is still needed and to be inserted again into master

I thought I added a test case for that one, but the one I have under testWhenSomeRecordsDoNotExistInReplicaButExistInMaster tests the case where all $neededRecords spilling here exist in master .. will add another test case for that

I changed the existing test case in testWhenSomeRecordsDoNotExistInReplicaButExistInMaster so that master have only a subset of those neededRecrods already, in which case the first attempt to save will fail resulting in the $neededRecords to be non-empty on this line.

using infinite loop here might still not be the best option .. we can make it just two steps checks procedural-style .. but I don't know if that would look better so I'd rather not change it until we find a third better improvement

Ok, that makes sense… but given we only care for equivalent rows anyway, couldn't we just go with INSERT IGNORE and be done?

yup we could've but we tried that before and the problem with using IGNORE Is that it will still increment the next primary key per every failing record .. we concluded that we do not want that side-effect in tables with high cardinality as our primary keys will have gaps and might risk reaching much more quickly the maximum value of id column

mariushoch · 2019-05-15T22:54:45Z

Had a quick glance… I think this is mostly good to go now (although I only checked superficially and didn't look at the tests).

One thing I noticed is that sometimes you seem to manually create sub-arrays (like $a = []; if (!isset($a['a'])) { $a['a'] = []; } $a['a'][] = 4;) and sometimes you "autocreate" them (like $a = []; $a['a'][] = 4;). I'm not sure which one we should chose… but I guess we should stick with one way.

AlaaSarhan · 2019-05-15T23:33:20Z

now I remember how rebasing was annoying in Github .. with these commits now appearing after all the previous comments .. a slight annoyance not a big deal if you ignore it, though in one team we did use merge over rebase mainly for that reason (which I'd probably not do again)

AlaaSarhan · 2019-05-16T12:22:31Z

Had a quick glance… I think this is mostly good to go now (although I only checked superficially and didn't look at the tests).

One thing I noticed is that sometimes you seem to manually create sub-arrays (like $a = []; if (!isset($a['a'])) { $a['a'] = []; } $a['a'][] = 4;) and sometimes you "autocreate" them (like $a = []; $a['a'][] = 4;). I'm not sure which one we should chose… but I guess we should stick with one way.

Good catch .. we surely should stick to one. I prefer auto-create (not more evel in php than creating variables without defining them). Will update accordingly, unless there're reasons against that approach you know of (and I don't)

…y existed

…ized tables

…r class

AlaaSarhan · 2019-05-21T11:10:38Z

src/TermStoreSchemaUpdater.php

@@ -8,6 +8,10 @@ class TermStoreSchemaUpdater {



Changes to this file (and related change in MediaWikiNormalizedTermCleanerTest.php) were not necessary .. they slipped through when I rebased .. but I'll avoid creating a commit to revert them unless reviewers would prefer that

AlaaSarhan · 2019-05-21T11:15:49Z

Had a quick glance… I think this is mostly good to go now (although I only checked superficially and didn't look at the tests).
One thing I noticed is that sometimes you seem to manually create sub-arrays (like $a = []; if (!isset($a['a'])) { $a['a'] = []; } $a['a'][] = 4;) and sometimes you "autocreate" them (like $a = []; $a['a'][] = 4;). I'm not sure which one we should chose… but I guess we should stick with one way.

Good catch .. we surely should stick to one. I prefer auto-create (not more evel in php than creating variables without defining them). Will update accordingly, unless there're reasons against that approach you know of (and I don't)

Checking again the code, I can't seem to find any sub-arrays pre-created where that can be ignored.. there are few cases where it was necessary as the sub-array will be used and can cause an exception if it wasn't created, as in this example (https://github.com/wmde/mediawiki-term-store/pull/11/files#diff-318c7c56290dcb03989fe814a679f4aeR146 lines 146 to 155):

				if ( !isset( $flattenedLangTextIds[$lang] ) ) {
					$flattenedLangTextIds[$lang] = [];
				}
				$flattenedLangTextIds[$lang] = array_unique(
					array_merge(
						(array)$textIds,
						(array)$flattenedLangTextIds[$lang]
					)
				);

lucaswerkmeister

Left some comments on the tests. (Some of them are duplicates, but I don’t know how readable “same here” comments are on GitHub, so I copy+pasted the comments in those cases.)

tests/Integration/PackagePrivate/DatabaseTermIdsAcquirerTest.php

lucaswerkmeister · 2019-05-21T13:49:04Z

tests/Integration/PackagePrivate/DatabaseTermIdsAcquirerTest.php

+			[ 'wbtl_text_in_lang_id' => $enSameTextInLangId,
+			  'wbtl_type_id' => $alreadyAcquiredTypeIds['alias'] ]
+		);
+		$aliasEnSameTermInLangId = (string)$this->db->insertId();


Why the cast to string?

because the interface expects string ids only (Note 2 https://github.com/wmde/mediawiki-term-store/pull/11/files#diff-8b29a0bea7b8ed10130868f7ba1dc7cdR75)

while InMemory implementation return integer ids.. I can perhaps change InMemory to return string ids as well

tests/Integration/PackagePrivate/Util/ReplicaMasterAwareRecordIdsAcquirerTest.php

lucaswerkmeister

Let’s merge this, it’s been waiting in review for quite long enough by now.

AlaaSarhan requested review from lucaswerkmeister and mariushoch May 9, 2019 20:45

mariushoch reviewed May 10, 2019

View reviewed changes

src/PackagePrivate/TypeIdsAcquirer.php Show resolved Hide resolved

mariushoch reviewed May 10, 2019

View reviewed changes

src/PackagePrivate/DatabaseTermIdsAcquirer.php Outdated Show resolved Hide resolved

src/PackagePrivate/DatabaseTermIdsAcquirer.php Outdated Show resolved Hide resolved

AlaaSarhan commented May 10, 2019

View reviewed changes

lucaswerkmeister reviewed May 10, 2019

View reviewed changes

lucaswerkmeister reviewed May 13, 2019

View reviewed changes

AlaaSarhan force-pushed the add-database-term-ids-acquirer branch from 1a99c4c to 6dae465 Compare May 15, 2019 07:19

lucaswerkmeister reviewed May 15, 2019

View reviewed changes

AlaaSarhan changed the title ~~[WIP] Add database term ids acquirer~~ Add database term ids acquirer May 15, 2019

mariushoch reviewed May 15, 2019

View reviewed changes

src/PackagePrivate/TypeIdsAcquirer.php Outdated Show resolved Hide resolved

mariushoch reviewed May 15, 2019

View reviewed changes

src/PackagePrivate/Util/ReplicaMasterAwareRecordIdsAcquirer.php Show resolved Hide resolved

mariushoch reviewed May 15, 2019

View reviewed changes

src/PackagePrivate/Util/ReplicaMasterAwareRecordIdsAcquirer.php Outdated Show resolved Hide resolved

mariushoch reviewed May 15, 2019

View reviewed changes

src/PackagePrivate/Util/ReplicaMasterAwareRecordIdsAcquirer.php Outdated Show resolved Hide resolved

mariushoch reviewed May 15, 2019

View reviewed changes

AlaaSarhan force-pushed the add-database-term-ids-acquirer branch from 6e3b56b to 489f81c Compare May 15, 2019 23:29

alaasarhan added 17 commits May 21, 2019 12:59

add Database implementation of TermIdsAcquirer

8668254

add database impl. of TermIdsAcquirer

2a84582

code style fixes

f71b381

improve test assertions

623bc78

add more tests to cover uniqueness and reusing existing records in db

964fbf7

short-circut acquire*Ids method evaluation when all needed ids alread…

08488b5

…y existed

change example to not imply implementation detail

5d8b017

move inserting non existing records into separate functions

edd2565

extract utility class for acquiring Ids from replica/master on normal…

787a3d0

…ized tables

log insertion failures

ab99212

move test under integration folder to match test setup

5d5e03f

add documentation on ReplicaMasterAwareRecrodIdsAcquirer class

d22d559

document the two piculuarities in ReplicaMasterAwareRecordsIdsAcquire…

9217e1c

…r class

fix space-tabs and trailing whitespaces

35bcadf

add unit test for ReplicaMasterAwareRecordIdsAcquirer

5200111

little changes according to comments on PR

cc86da2

improve test case. little typo and type hint fixes

ce2bf3f

AlaaSarhan force-pushed the add-database-term-ids-acquirer branch from 489f81c to ce2bf3f Compare May 21, 2019 11:00

AlaaSarhan commented May 21, 2019

View reviewed changes

lucaswerkmeister mentioned this pull request May 21, 2019

Add service to turn term IDs back into terms #25

Merged

3 tasks

lucaswerkmeister reviewed May 21, 2019

View reviewed changes

fix type and outdated variable names

d97f0dc

lucaswerkmeister approved these changes May 21, 2019

View reviewed changes

lucaswerkmeister merged commit 36913a5 into master May 21, 2019

lucaswerkmeister deleted the add-database-term-ids-acquirer branch May 21, 2019 15:49


		$acquiredTermIds = $dbTermIdsAcquirer->acquireTermIds( $termsArray );

		$this->assertTermSArrayExistInDb( $termsArray, $alreadyAcquiredTypeIds );


		$acquiredTermIds = $dbTermIdsAcquirer->acquireTermIds( $termsArray );

		$this->assertEquals(

Add database term ids acquirer #11

Add database term ids acquirer #11

Conversation

AlaaSarhan commented May 9, 2019 • edited Loading

mariushoch commented May 10, 2019

lucaswerkmeister commented May 10, 2019

lucaswerkmeister commented May 10, 2019

AlaaSarhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlaaSarhan commented May 10, 2019 • edited Loading

lucaswerkmeister commented May 10, 2019

AlaaSarhan commented May 10, 2019 • edited Loading

mariushoch commented May 10, 2019

AlaaSarhan commented May 10, 2019 • edited Loading

AlaaSarhan commented May 13, 2019 • edited Loading

mariushoch commented May 13, 2019

AlaaSarhan commented May 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlaaSarhan commented May 15, 2019 • edited Loading

AlaaSarhan commented May 15, 2019

lucaswerkmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariushoch commented May 15, 2019

AlaaSarhan commented May 15, 2019

AlaaSarhan commented May 16, 2019

Choose a reason for hiding this comment

AlaaSarhan commented May 21, 2019

lucaswerkmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlaaSarhan May 21, 2019 • edited Loading

Choose a reason for hiding this comment

lucaswerkmeister left a comment

Choose a reason for hiding this comment

AlaaSarhan commented May 9, 2019 •

edited

Loading

AlaaSarhan commented May 10, 2019 •

edited

Loading

AlaaSarhan commented May 10, 2019 •

edited

Loading

AlaaSarhan commented May 10, 2019 •

edited

Loading

AlaaSarhan commented May 13, 2019 •

edited

Loading

AlaaSarhan commented May 15, 2019 •

edited

Loading

AlaaSarhan May 21, 2019 •

edited

Loading