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

[Performance]Cache whether a DB supports transaction level #18691

Merged
merged 19 commits into from Feb 23, 2022
Merged

[Performance]Cache whether a DB supports transaction level #18691

merged 19 commits into from Feb 23, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jan 26, 2022 •
edited

Description:

Fixes: #18138
Cache whether a DB supports transaction on all the DB adapter to improve perfomance.

Review

add check
a305765 
add check
@peterhashair peterhashair marked this pull request as ready for review January 26, 2022 23:43
sgiehl
sgiehl requested changes Jan 31, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Code looks fine. Left a comment for improving the tests

tests/PHPUnit/Integration/Db/TransactionLevelTest.php Outdated Show resolved Hide resolved
sgiehl
sgiehl approved these changes Feb 1, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Looks good to me.
@tsteur do you want to have a last quick view before this is merged?

@sgiehl sgiehl added this to the 4.8.0 milestone Feb 1, 2022
@sgiehl sgiehl added the c: Performance For when we could improve the performance / speed of Matomo. label Feb 1, 2022
tsteur
tsteur requested changes Feb 1, 2022
View reviewed changes
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

In the original issue in #18138 I mention this:

We should cache this eg on the $this->db as it's DB dependent as if someone configures different DBs for writer and reader for example then we cannot reuse / cache the same result.

@peterhashair @sgiehl the current solution wouldn't 100% work for us.

We want to cache it per DB (in case writer and reader are somehow different) and also to have better cache when many new instances of this class are created. Otherwise it be only cached in the instance itself. We are creating this class very often so it wouldn't really help.

core/Db/TransactionLevel.php Outdated Show resolved Hide resolved
@peterhashair
Copy link
Contributor Author

@tsteur right, so this $supportsUncommitted should be in the Db.php.

move supportsUncommitted to db level
8126490 
move supportsUncommitted to db level
Update Db.php
8cb1365 
append $supportsUncommitted to global Db
@sgiehl
Copy link
Member

sgiehl commented Feb 4, 2022

@peterhashair most tests are now failing with messages like Undefined property: Piwik\Db\Adapter\Pdo\Mysql::$supportsUncommitted. Maybe it needs to be added to the adapters as well...

Update Mysqli.php
98676fe 
append to mysqli supportsUncommitted
@peterhashair peterhashair changed the title Cache whether a DB supports transaction level [Performance]Cache whether a DB supports transaction level Feb 8, 2022
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Feb 8, 2022
@peterhashair
Copy link
Contributor Author

@sgiehl I think that one is good to go?

@sgiehl
Copy link
Member

sgiehl commented Feb 10, 2022

From my point of view this looks good to merge.

tsteur
tsteur requested changes Feb 10, 2022
View reviewed changes
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

There might be still an issue. Also can we add a test for the cache by setting the property on the db object in a test and then we get expected result or something?

core/Db/TransactionLevel.php Show resolved Hide resolved
Update TransactionLevel.php
272c44e 
update function
@peterhashair peterhashair added the Needs Review PRs that need a code review label Feb 13, 2022
Update TransactionLevel.php
7a3fd63 
update transaction
Peter added 2 commits February 16, 2022 10:13
Update TransactionLevel.php
5c4259b 
revert back to previous change
Update TransactionLevel.php
a5aae71 
update cache level key
@peterhashair
Copy link
Contributor Author

@sgiehl right, sorry wasn't thinking, update again, if that makes sense.

sgiehl
sgiehl requested changes Feb 16, 2022
View reviewed changes
core/Db/TransactionLevel.php Show resolved Hide resolved
core/Db/TransactionLevel.php Outdated Show resolved Hide resolved
core/Db/TransactionLevel.php Outdated Show resolved Hide resolved
core/Db/TransactionLevel.php Show resolved Hide resolved
Peter added 2 commits February 17, 2022 10:15
Update TransactionLevel.php
3de659a 
update supportsUncommitted
Update TransactionLevel.php
7aaff4c 
add some comments
sgiehl
sgiehl requested changes Feb 21, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left a comment. Otherwise guess it looks good now, as the status backup doesn't seem to be an issue.

core/Db/TransactionLevel.php Outdated Show resolved Hide resolved
@sgiehl
Update core/Db/TransactionLevel.php
3ff3ffd 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
sgiehl
sgiehl approved these changes Feb 22, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Should be good to merge now. @tsteur do you want to have another look before merging?

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Feb 22, 2022
tsteur
tsteur requested changes Feb 22, 2022
View reviewed changes
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

@peterhashair @sgiehl I believe there are still few issues. Could we get the these resolved?

core/Db/TransactionLevel.php Outdated Show resolved Hide resolved
core/Db.php Outdated Show resolved Hide resolved
core/Tracker/Db.php Show resolved Hide resolved
remove $supportsUncommitted reset
0c37a71 
remove $supportsUncommitted reset
tsteur
tsteur approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

I haven't tested it but looks all good to me now 👍

@peterhashair peterhashair merged commit d994558 into 4.x-dev Feb 23, 2022
@peterhashair peterhashair deleted the m-18138-cache-translation branch February 23, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

@tsteur tsteur tsteur approved these changes

Assignees
No one assigned
Labels
c: Performance For when we could improve the performance / speed of Matomo. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.8.0
Development

Successfully merging this pull request may close these issues.

Cache whether a DB supports transaction level or not
3 participants
@peterhashair @sgiehl @tsteur