fix: (2.39) notification trigger cause server halt [DHIS2-19452]#20576
fix: (2.39) notification trigger cause server halt [DHIS2-19452]#20576
Conversation
zubaira
changed the title
zubaira
changed the title
| return predicates; | ||
| } | ||
|
|
||
| public Pair<Date, Date> getStartAndEndOfDay(Date scheduledAt) { |
There was a problem hiding this comment.
Only for this branch. For higher branches, I will have a param mapper mapping scheduledAt to scheduledFrom and scheduledTo
There was a problem hiding this comment.
What do you mean by
Only for this branch. For higher branches
There was a problem hiding this comment.
Normally, we handle this in the parameter mapper class. However, for version 2.39, I implemented it directly in the store. For higher versions, I plan to follow the standard approach using the parameter validation mapper class.
| @@ -658,13 +661,13 @@ void testScheduledNotifications() { | |||
| return new BatchResponseStatus(Collections.emptyList()); | |||
There was a problem hiding this comment.
Can we delete this mock test which is what we generally try to do and rely on dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/program/notification/ProgramNotificationServiceIntegrationTest.java instead
There was a problem hiding this comment.
yes. I will do that after creating integration tests for ProgramNotificationService
| /* No assertions are made here because this test is intended to verify that | ||
| scheduled notifications are fetched and processed within the specified time limit | ||
| (performance check only) */ | ||
| programNotificationService.sendScheduledNotifications(NoopJobProgress.INSTANCE); |
There was a problem hiding this comment.
This test is not really needed is it? The notificationInstanceService.getAll() was the issue right? So if you add a test to the ProgramNotificationInstanceServiceTest for getAll and the method you are using now showing they do not timeout that would be enough. This would also allow you to assert on their results.
This test here could be useful but IMHO you would need to assert that the notifications you expected to be sent were actually sent.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.39 #20576 +/- ##
============================================
- Coverage 58.58% 57.62% -0.96%
+ Complexity 25817 25552 -265
============================================
Files 3207 3228 +21
Lines 121397 123292 +1895
Branches 14158 14371 +213
============================================
- Hits 71118 71053 -65
- Misses 44340 45849 +1509
- Partials 5939 6390 +451
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1307 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
...api/src/main/java/org/hisp/dhis/program/notification/ProgramNotificationInstanceService.java
Outdated
Show resolved
Hide resolved
...n/src/test/java/org/hisp/dhis/programrule/engine/ProgramNotificationInstanceServiceTest.java
Outdated
Show resolved
Hide resolved
|
4 participants
While testing this issue, we observed that the server occasionally halts or restarts when a user attempts to send scheduled notifications.
This PR will now be deployed to the IM environment for further testing. If everything works as expected there, we can proceed with merging it.
https://dhis2.atlassian.net/browse/DHIS2-19452
Filtering at the Database Level
Previously, all ProgramNotificationInstance records were loaded from the database and filtered in memory based on the scheduledAt date.
This PR moves that filtering to the database query itself, ensuring that only relevant notifications scheduled for the current day are fetched.
Reducing JSONB Payload Size
The ProgramNotificationTemplateSnapshot column was previously storing the full serialized template object, including redundant and unnecessary data.
This caused heavy query delays due to bloated row sizes.
The updated logic stores only the essential fields in the snapshot, significantly improving query performance and database efficiency.