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] Process notification requests and cancellations asynchronously #438

Open
peyman-mohtashami opened this issue Sep 18, 2023 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@peyman-mohtashami
Copy link
Member

Is your feature request related to a problem? Please describe.
Processing notification requests and cancellations can be time-consuming, particularly when there is a significant delay in waiting. For instance, when we transmit requests from a distant region to the app server, the response time is notably slow.

Describe the solution you'd like
To enhance efficiency, we should implement a feature that allows multiple requests and cancellations to be bundled into a single request. This solution will require modifications to be made to the Questionnaire app accordingly.

Describe alternatives you've considered

Priority
3

Difficulty
2

Additional context

@yatharthranjan
Copy link
Member

Hi @peyman-mohtashami, thanks for adding the issue.

Can you and @mpgxvii give an overview of what requests bundling is required?

The appserver already has a few endpoints which support this, such as

  1. Adding notifications in bulk - here
  2. Get all notifications for user - here
  3. Remove all notifications for a user - here

I agree these are not async and perhaps we can look into that for quicker responses, but Joris already made a start on that here where async is useful.

@mpgxvii
Copy link
Member

mpgxvii commented Sep 18, 2023

Yes I think on the appserver side we already have the batch notification adding, so I think we would just need to implement this on the Questionnaire app side? I think initially we wanted to schedule these individually because we needed to store the notification ids back to each task, but maybe we can process this afterwards instead.

@peyman-mohtashami
Copy link
Member Author

Thanks @yatharthranjan. Are these endpoints used in the Questionnaire app? @mpgxvii, I checked the alpha version of the Questionnaire app and couldn't find usage of these endpoints.

@mpgxvii
Copy link
Member

mpgxvii commented Sep 18, 2023

Thanks @yatharthranjan. Are these endpoints used in the Questionnaire app? @mpgxvii, I checked the alpha version of the Questionnaire app and couldn't find usage of these endpoints.

@peyman-mohtashami Yes these are not implemented yet on the questionnaire app side.

@peyman-mohtashami
Copy link
Member Author

Thanks @mpgxvii.
So, the only thing that is needed is to add to the Questionnaire app. Could you give me a short docs on it, or point me to the resource files to find the paths?

@peyman-mohtashami
Copy link
Member Author

peyman-mohtashami commented Sep 18, 2023

I think this should be the resource where paths are defined, right?
https://github.com/RADAR-base/RADAR-Appserver/blob/25d53a82cade28c66492713101371a0470c349bb/src/main/java/org/radarbase/appserver/controller/FcmNotificationController.java

@mpgxvii
Copy link
Member

mpgxvii commented Sep 18, 2023

I think this should be the resource where paths are defined, right? https://github.com/RADAR-base/RADAR-Appserver/blob/25d53a82cade28c66492713101371a0470c349bb/src/main/java/org/radarbase/appserver/controller/FcmNotificationController.java

Yes, that's correct.

@Bdegraaf1234
Copy link
Contributor

Bdegraaf1234 commented Oct 3, 2023

While working on this I encountered some odd behaviour:

The endpoint /projects/{projectId}/users/{subjectId}/messaging/notifications/schedule returns all scheduled notifications for the user.

While doing this we log Scheduling {} messages. From the code here I got the impression that this endpoint schedules multiple notifications, because it calls this function.

I propose we make this endpoint a GET request to avoid confusion, remove the logic involving the schedulerservice (because it doesn't seem to do anything) and rename the function getAllScheduledUserNotifications.

But maybe I am missing something here? If not, let me know and I can get started on a pull-request.

@yatharthranjan
Copy link
Member

Hi @Bdegraaf1234 , i can understand the confusion.

There are 2 steps related to the notifications in the appserver -

  1. Just adding the notifications for a user with the correct details such as title, time, etc. These are not triggered by themselves but store valuable information about the notification.
  2. Scheduling the notifications (using Quartz jobs) at the correct time so they are are delivered to the users.

The /schedule endpoint is to do the point 2 above in case the notifications were added previously using point 1 but were not scheduled. This is possible when adding the notifications with schedule=false query parameter. When the schedule=true is used, the schedule step is also done after adding the notification, so an additional request to /schedule endpoint is not required.

Since the /schedule endpoint is adding Quartz triggers and jobs in the database for existing notification entities, hence we use POST instead of GET.

When you are getting Scheduling {} messages it could be because there are no notifications for the user to schedule present in the database or that all of them already are scheduled.

Hope it is clear, but let me know if you need more info.

@Bdegraaf1234
Copy link
Contributor

Hi @yatharthranjan

That clarifies things indeed, thank you for the explanation. Will leave this as is then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants