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

feat(storage): use keep config for trim() method in database storage #395

Closed
wants to merge 1 commit into from
Closed

feat(storage): use keep config for trim() method in database storage #395

wants to merge 1 commit into from

Conversation

fabpl
Copy link

@fabpl fabpl commented Jul 15, 2024

Like Redis Ingest, use keep config key in database storage trim() functionality.

@taylorotwell taylorotwell requested a review from jessarcher July 15, 2024 15:02
@taylorotwell taylorotwell marked this pull request as draft July 15, 2024 15:03
@timacdonald
Copy link
Member

timacdonald commented Jul 15, 2024

@fabpl, any particular reason why you want to keep less than a weeks worth of data in storage?

@@ -124,15 +124,16 @@ public function store(Collection $items): void
public function trim(): void
{
$now = CarbonImmutable::now();
$keep = $this->config->get('pulse.ingest.trim.keep');
Copy link
Member

Choose a reason for hiding this comment

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

We probably need to introduce a custom storage key here: pulse.storage.keep

Then we need to decide if we fallback to the ingest value if the storage value is not specified.

Choose a reason for hiding this comment

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

Hi @timacdonald, we thought about that because the pulse.ingest.trim.lottery key is already used in the case of DatabaseStorage, so why not use the pulse.ingest.trim.keep too, to customize if we want to keep the data longer ? Or as you said, create a new key in the storage config part.

$odds = $this->app->make('config')->get('pulse.ingest.trim.lottery') ?? $this->app->make('config')->get('pulse.ingest.trim_lottery');

@fabpl
Copy link
Author

fabpl commented Jul 16, 2024

@fabpl, any particular reason why you want to keep less than a weeks worth of data in storage?

Hi @timacdonald,
Perhaps I didn't quite understand the difference between redis ingest and database storage, but I think that if the duration is configured on one, it should be configured on the other?

@timacdonald
Copy link
Member

@fabpl, not necessarily. On Forge, we want to limit the Redis ingest to 24 hours while we want to keep 30 days in the database. This ensures that if the pulse:work command goes down, we don't attempt to store 30 days worth of data in Redis and have it fall over causing issues with our incoming requests.

Can you share what specific problem you are experiencing that this PR is hoping to solve for your application?

@fabpl
Copy link
Author

fabpl commented Jul 19, 2024

I've no specific problem.
I'm trying to understand why ingest (keep delay) are configurable while storage is not.

@fabpl fabpl closed this Jul 19, 2024
@timacdonald
Copy link
Member

Gotcha. Because your ingest is often an in-memory system, like Redis, it can often fall over if the queue:work command was to go down, simply because it is restricted to memory limits.

Where as the final storage destination, a database, is designed to hold much more information - so we don't trim the storage data until it is outside the window

@DamienToscano
Copy link

Gotcha. Because your ingest is often an in-memory system, like Redis, it can often fall over if the queue:work command was to go down, simply because it is restricted to memory limits.

Where as the final storage destination, a database, is designed to hold much more information - so we don't trim the storage data until it is outside the window

That makes sense.

Thanks @timacdonald for the explanation

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.

3 participants