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

Not skip the Notification but send it after one hour #33

Closed
noreengulappelit opened this issue Mar 19, 2024 · 4 comments
Closed

Not skip the Notification but send it after one hour #33

noreengulappelit opened this issue Mar 19, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@noreengulappelit
Copy link

noreengulappelit commented Mar 19, 2024

I want to send notification after one hour , if it is send before one hour then send it after one hour. don't skip it

@tibbsa
Copy link
Collaborator

tibbsa commented Mar 19, 2024

To clarify, you would want a sequence such as this, assuming a max rate of 1/hour (rate_limit_seconds=3600):

Timestamp Dispatches Results
00:00 MyNotification 1 Sent Mail MyNotification 1 to user
00:15 - -
00:30 MyNotification 2 Sent (deferred)
00:45 - -
01:00 - Mail MyNotification 2 to user
01:15 - -
01:30 - -
01:45 - -
02:00 - -

There is a possibility that this could result in a large backlog of really old notifications, if for example multiple notifications were sent within an hour. Would you have some cap or limit at which point notifications really would be skipped to avoid this?

Example of the problem:

Timestamp Dispatches Results
00:00 MyNotification 1 Mail MyNotification 1 to user
00:15 MyNotification 2 (deferred)
00:30 MyNotification 3 (2 and 3 deferred)
00:45 MyNotification 4 (2, 3, and 4 deferred)
01:00 - Mail MyNotification 2 to user
01:15 - -
01:30 - -
01:45 - -
02:00 - Mail MyNotification 3 to user
02:15 - -
02:30 - -
02:45 MyNotification 5 (4 and 5 deferred)
03:00 - Mail MyNotification 4 to user
03:15 - -
03:30 - -
03:45 - -
04:00 - Mail MyNotification 5 to user

... and so on. In such a case, MyNotification 4 is actually delayed more than 2 hours before being delivered. Perhaps that is fine, but if multiple of these notifications keep coming every hour, you may never catch up and wind up being consistently several hours behind. But perhaps I have misunderstood, or perhaps for your use case that isn't likely to happen.

Have I understood what you are asking, though?

@noreengulappelit
Copy link
Author

noreengulappelit commented Mar 19, 2024

@tibbsa you are right. in this case the Notification 2 , 3 and 4 should be skips and the last one before the hour complete should be send. any idea , how can i achieve this ?

@tibbsa
Copy link
Collaborator

tibbsa commented Mar 19, 2024

If it were just a matter of deferring, then we could (subject to limitation no. 1 below) use deferred/delayed queueing to accomplish this.

However, there are two limitations:

  1. Some queue drivers have a limited queuing horizon. For example, Amazon SQS will not allow defer times >15 minutes (as noted in the Laravel docs).
  2. Since Laravel provides no way that I know of to 'inspect' a queue, iterate over its items, and determine whether an existing Notification type is in the queue and waiting to be dispatched at a later time, there is no way for us to discard previously deferred notifications in favour of the 'most recent'.

Taken together, the better approach would likely be to use a database-backed 'holding pen' for deferred notifications before actually shipping them to the queue for sending.

Let think about this approach - and whether this is too big a leap for this package, or whether we can just provide the 'recipe' for how to do this in your app, etc.

@tibbsa tibbsa added the enhancement New feature or request label Mar 19, 2024
@tibbsa tibbsa self-assigned this Mar 19, 2024
@tibbsa
Copy link
Collaborator

tibbsa commented Mar 29, 2024

@noreengulappelit I have given this some thought, and think it is a bit beyond what we should add to this package specifically, given that it would require a means of storing notifications, etc. and the specifics of that will be very application-dependent.

However, I have added some additional context and information to the NotificationRateLimitedEvent (now available on the master branch) that can facilitate the implementation of a system like this within an application, and prepared a sample implementation for how this might be done.

See https://github.com/tibbsa/lnrl_deferral_example/ for a sample implementation. The (very atomic) commits were added to the repo in an order that should make it easy to follow how this was built up.

If you are trying to replicate this right away, you will need to use a composer repository override (see the composer.json in the above repo for example) to pull the latest 'dev' code for this package from Github, until the next release at least.

@tibbsa tibbsa added the wontfix This will not be worked on label Mar 29, 2024
@tibbsa tibbsa closed this as completed May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants