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

fix prune method #951

Closed
wants to merge 6 commits into from
Closed

fix prune method #951

wants to merge 6 commits into from

Conversation

giolaza
Copy link

@giolaza giolaza commented Jun 26, 2024

No description provided.

Copy link
Contributor

@willpower232 willpower232 left a comment

Choose a reason for hiding this comment

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

please see progress in #948

@giolaza
Copy link
Author

giolaza commented Jun 26, 2024

please see progress in #948

#948 is better because it more dynamic

@giolaza
Copy link
Author

giolaza commented Jun 26, 2024

@willpower232 changes made,
fixed #946
added config audit.placeholders_limit with default 50k, mysql supports 65,535
used keyname idea from #948

->delete() > 0;
->get()
->slice($threshold)
->pluck($auditKeyName);

Choose a reason for hiding this comment

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

It seems to me that it is more efficient to put the pluck above the slice, replacing the get

$forRemovalChunks = $forRemoval->chunk(Config::get('audit.placeholders_limit', 50000));
$answer = false;
foreach ($forRemovalChunks as $chunk) {
$answer = $answer || $model->audits()

Choose a reason for hiding this comment

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

Maybe I don't understand this,
after the first chunk, answer would be true,
then in the second chunk(and later), the delete would no longer be executed

Copy link

@parallels999 parallels999 Jun 26, 2024

Choose a reason for hiding this comment

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

Also, if the first chunk is executed, and the second for some reason is not, would it return true even though only a portion of the records were deleted?

Copy link
Author

Choose a reason for hiding this comment

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

any ideas? I thought if some deletion was performed in any case return true

Copy link

@parallels999 parallels999 left a comment

Choose a reason for hiding this comment

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

You also need to add tests to corroborate that the chunks are working.

->pluck($auditKeyName);

if (!$forRemoval->isEmpty()) {
$forRemovalChunks = $forRemoval->chunk(Config::get('audit.placeholders_limit', 50000));
Copy link

@parallels999 parallels999 Jun 26, 2024

Choose a reason for hiding this comment

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

If you have thousands of records, they will all be stored in memory as models, wouldn't that be less efficient than a leftJoin?

Also, there would be two queries versus just one.

->latest()
->offset($threshold)->limit(PHP_INT_MAX)
->delete() > 0;
->skip($threshold)
Copy link
Contributor

Choose a reason for hiding this comment

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

skip and take are alias of offset and limit

@giolaza
Copy link
Author

giolaza commented Jun 27, 2024

think this topic now is closed

@giolaza giolaza closed this Jun 27, 2024
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.

4 participants