-
Notifications
You must be signed in to change notification settings - Fork 76
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
Introduce Persistent Email Reminders for Producers of Datasets #1248
Comments
Please review this design and help answer pending questions at the bottom: Objective: The objective of this feature is to implement persistent email notifications to remind dataset producers to approve/reject share requests that they have not responded to in a timely manner. The aim is to streamline the share request workflow and ensure that dataset producers remain engaged and responsive to pending requests. Overview: When a share request is initiated and remains pending for an extended period, dataset producers will receive automated email reminders at predefined intervals. These reminders will prompt producers to either approve or extend the share request, thereby preventing delays in accessing datasets. Technical Implementation: Backend Logic: Database Schema: Workflow:
Pending Questions:
|
Only send reminders to producers. Our goal is to make sure they are reminded they need to approve.
Should be a separate task to notify ONCE the requester and producer team that the share failed
Separate task but I believe if share health verifier finds that a share becomes unhealthy it should start sending reminders.
If easy to do then yes. Default should be daily. We should probably also let users define the time of day when notifications are sent. For us it may be early EU hours but for someoene else in Asia they might want to choose different hours it also depends what Timezone the reminder service runs with. Perhaps the service should also allow to configure the timezone + time of day.
Agree. We can allow multiple strategies: single_reminder, repeating_reminder
I think we should keep it as simple as possible and for share reminders I don't think we need a new table. We can see which shares are pending approval and can figure out if they need a reminder or not. I don't see a benefit to a new table to track reminders. Additionally: We should not design this service just for share reminders. There's many things we should start sending nagging reminders:
My thinking would be that we have some interface of reminder strategies let's call it iReminderStrategy and we have ReminderService which would kinda work like ReminderService.remind(reminders_strategies: List[iReminderStrategy]) and we can have multiple implementations of iReminderStrategy like: FailingStackReminderStrategy, UnhealthyShareReminderStategy, PendingShareReminderStrategy.. For this task I would focus just on building generic support for reminders and implementing PendingShareReminderStrategy Also we should if possible use some sort of iNotifier interface so that reminders could be sent to multiple places like slack channels, alerting systems, email etc.. For now just having email notifier would be enough. |
Hi @anushka-singh ,
I don't think we would need another RDS table for storing information about statuses of email notification. I think we can leverage the
I agree we should have this config. We can use the current email notification config and add this config Regarding comment from Zilvinas,
I agree on having a daily schedule to sent reminder emails ( for shares requests, stack failures, etc). Making it configurable based on timezone might involve more complex design ( for e.g. like creating an event which is triggered based on datetime which then calls lambda to execute the share request notification email) I think we can iterate on this in stages and keep it simple for now. Currently, email notifications for share notifications are sent via AWS workers lambda. For this we could use an scheduled ECS task for trigger email notifications for various strategies like Zilvinas mentioned ( " FailingStackReminderStrategy, UnhealthyShareReminderStategy, PendingShareReminderStrategy " ) OR create a recurring event which triggers lambda to send email notification but I am not sure if the 900 sec Runtime will be enough. One suggestion on the subject of email reminders, we should have something which notifies the user to take actions. Something like : [ACTION REQUIRED] Share Request pending for dataset - |
I agree with Zi's suggestion for the long term. It will be good to have a plug and play model like: ReminderStrategy and ReminderService which would kinda work like ReminderService.remind(reminders_strategies: List[iReminderStrategy]) I have a question that maybe @noah-paige @dlpzx can best answer: For this particular design strategy, should I create a new module called persistent_notifications? Or do you just see this be a part of the current notification modules? |
Not to repeat every thing that is said above but I agree with most (if not all) of the points:
For how to design notification system in the long run (not sure if this needs to be prioritized for this particular feature request but potentially) - the below is a WIP similar a bit to how @zsaltys is thinking but just trying to get some ideas out there... The issues I see with Notifications currently:
Ideally we have a Notification system that can create Notifications of any type and delivery them through any channel without having to edit the foundational Notification Module
|
I agree with all the points above. The most important decision points that I would remark:
In my opinion it might be an overkill to go for the second option. But I am not strong opinionated |
Design after meeting on 5/15 with @dlpzx and @noah-paige
Wrapping up thoughts and comments here:
Design proposal:
The cron job expression "0 9 * * 1" means the reminder will be triggered at 9:00 AM every Monday (1 represents Monday in the cron syntax). This will ensure that the reminder runs weekly on the specified day and time. If you want the reminder to be triggered on a different day of the week, you can adjust the value after the day of the week field (1 in this case) accordingly. For example, 0 9 * * 2 for Tuesday, 0 9 * * 3 for Wednesday, and so on, up to 7 for Sunday.
Option 1. EventBridge + SQS + Trigger a Lambda (ECS Task)
Option 2: Reuse AWS Worker Lambda
Option 3: Create Brand New Lambda
|
above design looks good - for implementation of the backend logic, encourage to re-use the code we have already in place to send email to also send the reminder emails I think in our talk Option1 and Option2 are the same - adding an EventBridge trigger and re-using the existing SQS Queue and worker lambda to handle email notifications But the more I think of it and with the max execution time constraints it may be easiest just to to spin up a brand new ECS Task (you can schedule task definitions to run at given times like how we've done with share-verifier or stack-updater ecs tasks) And the code to send email notifications I think is bundled into both ECS and lambda container images so should be good to use in either. |
Implement persistent email notifications to remind producers to approve or extend share requests that they dont respond to in time.
We want to be able to nudge producers of datasets to take actions if they have been pending on their ledger for a while.
Extend this feature's usage for expiration of shares.
The text was updated successfully, but these errors were encountered: