[3.x] Prevent 'memory exhausted' when deleting monitored tag

[gdebrauwer]

Fixes #625.

What?

When you monitor a tag and a lot of jobs are attached to tag, you will get a 'memory exhausted' php error when you remove the tag from monitoring list. This is caused by fetching all jobs of that tag in one query (in order to delete them all).

How?

Instead of fetching all jobs of the tag in one query, I used the paginate() method that is already present in TagRepository. By fetching and deleting small batches of jobs we will keep the php memory usage low at all times.

By default the pagination has a limit of 25 per page. Maybe we should use a limit that's a bit higher in this case (50, 100, ...)?

[derekmd]

Can you add a feature test that deletes 3 or more pages of tag results and then asserts all of them are no longer monitored? When looping via paginate() or chunk() with a destructive action, half of the loop iterations can be skipped over.

e.g., 75 tag results which would be 3 pages at 25 each.

• Fetch page 1 (25 items, starting offset 0)
• Delete page 1
• Fetch page 2 (25 items, starting offset 25)
• Delete page 2
• Done.

Page 2 in the above operation can actually be the expected page 3. Page 2 is skipped over since the page 1 batch delete shifts items expected in page 2 into page 1 (offset 0). Page 2 (offset 25) becomes the page 3 items.

RedisJobRepository calls commands https://redis.io/commands/zrevrange and https://redis.io/commands/expireat. Do you know if calling them subsequently will have the above behaviour?

[gdebrauwer]

@derekmd I tested it locally with more than 100 tag results, and it looped over all those results. The deleteMonitored method uses the redis expireat command which does not delete the tag results immediately, so pagination works in that case.
I don't have time in the following days, but I will try to add the test this weekend.

[taylorotwell]

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

[driesvints]

@gdebrauwer you might want to try again but with tests this time.