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.

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

Archiving with --force-all-websites now shares the site list with other processes #7682

Merged
merged 2 commits into from Apr 15, 2015
Merged

Archiving with --force-all-websites now shares the site list with other processes #7682

merged 2 commits into from Apr 15, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Apr 15, 2015

See #7614

Archiving with --force-all-websites now shares the site list. That allows to run a second archive process which will reuse the same (shared) site list.

To avoid a normal archiver sharing the site list with an archiver using --force-all-websites, we store the shared site list in a different option.

To sum up:

# share the same site list
./console cron:archive
./console cron:archive

# share the same site list
./console cron:archive --force-all-websites
./console cron:archive --force-all-websites

# don't share the same site list
./console cron:archive
./console cron:archive --force-all-websites

@mnapoli
#7614 Archiving with --force-all-websites now shares the site list
dd739fd 
That allows to run a second archive process which will reuse the same (shared) site list.
@mnapoli mnapoli added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Apr 15, 2015
@mnapoli mnapoli added this to the Piwik 2.13.0 milestone Apr 15, 2015
@mnapoli mnapoli mentioned this pull request Apr 15, 2015
Closed
diosmosis
diosmosis reviewed Apr 15, 2015
View reviewed changes
core/CronArchive.php
|| !SharedSiteIds::isSupported()) {
if (!empty($this->shouldArchiveAllSites) && SharedSiteIds::isSupported()) {
$this->websites = new SharedSiteIds($websitesIds, SharedSiteIds::OPTION_ALL_WEBSITES);
} elseif (!empty($this->shouldArchiveSpecifiedSites) || !SharedSiteIds::isSupported()) {
Copy link
Member

Choose a reason for hiding this comment

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

A suggestion to improve clarity (at least for this small part of CronArchive.php): could the site ID list creation logic be in a private method? ie $this->websites = $this->createSitesToArchiveQueue(...); then instead of an if/elseif, could have several if(s) w/ return statements.

@diosmosis
Copy link
Member

Verified that this works as expected. Once the single review comment is dealt w/, it can be merged.

@mnapoli
Copy link
Contributor Author

mnapoli commented Apr 15, 2015

Done, moved the code to a private method

@diosmosis
Copy link
Member

I was hoping you'd also clear up the logic a bit. I find it a bit confusing, and since you're in there, you could do some cleaning. Eg, if the if/elseif is split up, the first && SharedSiteIds::isSupported (or whatever) would be redundant and could be removed.

diosmosis added a commit that referenced this pull request Apr 15, 2015
@diosmosis
Merge pull request #7682 from piwik/archiving-share-sites-force-all
0add70e 
Archiving with `--force-all-websites` now shares the site list with other processes that are run w/ `--force-all-websites`.
@diosmosis diosmosis merged commit 0add70e into master Apr 15, 2015
@diosmosis diosmosis deleted the archiving-share-sites-force-all branch April 15, 2015 23:57
@diosmosis diosmosis removed the Needs Review PRs that need a code review label Apr 16, 2015
diosmosis pushed a commit that referenced this pull request Apr 16, 2015
Refs #7682, slight clarification of code in CronArchive::createSitesT…
626747a 
…oArchiveQueue method.
@mattab
Copy link
Member

mattab commented Apr 16, 2015

FYI @mgazdzik we changed the meaning of --force-all-websites so it always uses a shared queue

@mattab
Copy link
Member

mattab commented Apr 16, 2015

@mnapoli could you update the --force-all-websites param documentation to explain the ilst will be shared across multiple processes?

mnapoli added a commit that referenced this pull request Apr 16, 2015
@mnapoli
#7682 Documentation
355e2bd
@mnapoli
Copy link
Contributor Author

mnapoli commented Apr 16, 2015

@mattab done

@mattab mattab mentioned this pull request Jul 15, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mnapoli @diosmosis @mattab