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

use RecordBuilders in VisitTime plugin #20766

Merged
merged 102 commits into from
Jul 4, 2023
Merged

use RecordBuilders in VisitTime plugin #20766

merged 102 commits into from
Jul 4, 2023

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented May 19, 2023 •
edited

Description:

This PR is based off of #20720 which should be reviewed and merged first.

Changes:

  • Move archiving logic to RecordBuilders in VisitTime plugin.

Review

@diosmosis
introduce RecordBuilder concept and re-organize Goals archiving code …
6a388a0 
…via RecordBuilders
@diosmosis
fix loop iteration bug
5f42975
@diosmosis
Merge branch '5.x-dev' into record-builders-poc
a9c3168
@diosmosis
split ecommerce records recordbuilder into 3 separate records
5921f9d
@diosmosis
make sure Goals::getRecordMetadata() behaves like old archiver code
b2d8e10
@diosmosis
make sure recordbuilder archive processor is restored after being use…
2be26f2 
…d since archiving is a recursive process
@diosmosis
just make ArchiveProcessor a parameter
0db7cb5
@diosmosis
check for plugin before calling buildMultiplePeriod()
fda7ee7
@diosmosis
do not invoke record builders if archiver has no plugin (happens duri…
b2ea252 
…ng tests)
@diosmosis
insert empty DataTables (as this appears to be the existing behavior …
6cfe3ee 
…before this change)
@diosmosis
add RecordBuilder class name to aggregation query hint
43bd346
@diosmosis
clear up in-source todo
c24d3aa
@diosmosis
attempt only archiving requested report if range archive and the reco…
c2de813 
…rd needed is created by a RecordBuilder
@diosmosis
refactor ArchiveSelector::getArchiveIds() to provide result with stri…
0139de3 
…ng keys
@diosmosis
when all found archives are partial archives, check that requested da…
f7e3831 
…ta is present within them. if some are not present, only archive those in a new partial archive.
@diosmosis
return correct value in Model::getRecordsContainedInArchives()
70b5bab
@diosmosis
fix if formatting
d05f243
@diosmosis
existingArchives can be falsy
1609d31
@diosmosis
existing archives can be null if the check is not relevant to the cur…
234467a 
…rent archive request
@diosmosis
do not archive dependent segments if only processing the specific req…
cb1cf08 
…uested report
@diosmosis
fix more tests
21b62d8
@diosmosis
fix LoaderTest
99599e9
@diosmosis
make sure if archiving specific reports for a single plugin that arch…
46abe79 
…iver class instances will not be created
@diosmosis
add filterRecordBuilders event
4c2701b
@diosmosis
if it looks like the requested records are numeric, prioritize the nu…
c817f79 
…meric archive table, otherwise blob archive table
@diosmosis
fix copy-paste error
d18b61e
@diosmosis
add dummy test for numeric values
f6714dc
@diosmosis
add test for partial archiving of numeric records for ranges and fix …
52c4e93 
…typo causing this to fail
@diosmosis
Merge branch '5.x-dev' into record-builders-poc
e1938de
@diosmosis
lessen code redundancy in Archive.php, use Piwik\\Request and do not …
ce8f7a0 
…yet mark RecordBuilder as api
@diosmosis diosmosis added this to the 5.0.0 milestone May 19, 2023
@michalkleiner
Copy link
Contributor

We might chain the PRs in the way they were done so that we only see the incremental changes. When the first one is merged the target branch would get automatically updated etc.

Having all the changes from all the other PRs make this quite hard to navigate.

@diosmosis
Copy link
Member Author

@michalkleiner Please review/merge the original RecordBuilder PR first. This one does not have the Needs Review tag for a reason.

@michalkleiner
Copy link
Contributor

Fair point, missed that missing tag. Sorry!

@diosmosis diosmosis marked this pull request as draft May 30, 2023 07:02
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 14, 2023
@michalkleiner michalkleiner removed the Stale The label used by the Close Stale Issues action label Jun 28, 2023
@diosmosis diosmosis marked this pull request as ready for review June 29, 2023 00:45
@diosmosis diosmosis added the Needs Review PRs that need a code review label Jun 29, 2023
sgiehl
sgiehl requested changes Jul 4, 2023
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.

@diosmosis there seems to be a related test failing. It works on 5.x-dev, so it seems to be connected to the changes in this PR.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jul 4, 2023
@diosmosis
Copy link
Member Author

@sgiehl should be fixed by latest commit. will fix the same issue for other PRs as well.

@sgiehl sgiehl merged commit ed42e45 into 5.x-dev Jul 4, 2023
18 of 20 checks passed
@sgiehl sgiehl deleted the visit-time-record-builder branch July 4, 2023 14:00
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Aug 1, 2023
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

Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@diosmosis @michalkleiner @sgiehl