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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance prunes command #35719

Closed
wants to merge 2 commits into from
Closed

Enhance prunes command #35719

wants to merge 2 commits into from

Conversation

gocanto
Copy link
Contributor

@gocanto gocanto commented Dec 26, 2020

This PR introduces small tweaks to enhance the feature introduced in #35694

  • Work with cursor to avoid memory issues reading from tables with a bunch of records.
  • Inject the prune interface instead to avoid checking for types to decide paths.

I will be happy to add the missing tests for it (couldn't find them) if the proposed changes are accepted.

Merry Christmas 馃馃徎

@gocanto gocanto changed the title Tweak prunes Enhance prunes command Dec 26, 2020
$totalDeleted += $deleted;
} while ($deleted !== 0);
foreach ($items as $item) {
$item->delete();
Copy link
Contributor

Choose a reason for hiding this comment

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

Memory issue should be solved, but will this not fire a query each time?

Copy link
Contributor

@dmason30 dmason30 Dec 26, 2020

Choose a reason for hiding this comment

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

I agree this will be much slower and I am also not convinced there is any memory issues with the previous version of the code which was taken from the official Laravel telescope package.

https://github.com/laravel/telescope/blob/65ba769986232053808d0abe3400a004e3f35b88/src/Storage/DatabaseEntriesRepository.php#L339

https://github.com/laravel/telescope/blob/master/src/Console/PruneCommand.php

Not sure if the original code was misread but is not loading any models into memory as it is just doing a delete query with a limit of 1000 records per delete.

* @return void
*/
public function handle()
public function handle(PrunableBatchRepository $repository)
Copy link
Contributor

@dmason30 dmason30 Dec 26, 2020

Choose a reason for hiding this comment

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

As far as I can tell, This interface is not in the container and won鈥檛 be resolved and the command will break.

The point of the original check was that some future repository may not be prunable so it should still use the BatchRepository interface to get the repository and I think the check is still required.

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

馃憥

@gocanto gocanto deleted the tweak-prunes branch December 27, 2020 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants