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(notification): support send notification when schedule job failed #711

Merged
merged 31 commits into from
Nov 21, 2023

Conversation

LuckyPickleZZ
Copy link
Collaborator

What type of PR is this?

type-feature

What this PR does / why we need it:

  1. Support sending notifications when schedule job scheduling execution fails
  2. Change the matching rule of notification policy from string matching to map matching
  3. Fixed a bug where if a notification within a batch fails to be sent, the entire transaction will be rolled back, resulting in multiple message resents

Which issue(s) this PR fixes:

Copy link
Collaborator

@MarkPotato777 MarkPotato777 left a comment

Choose a reason for hiding this comment

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

I have some questions that we can discuss:

  • Would it be better if we do not enqueue events while there is no notification policy configured in the current organization? I doubt if all events are pushed into the event queue, the conversion from event to notification will be delayed and so does the notification sending.
  • Do we need a pressure test to make sure of the stability?
  • Do we need to refine the configuration values of NotificationProperties? or add some other configurations?

@yhilmare yhilmare modified the milestones: ODC 4.2.2, ODC 4.2.2-bp Nov 14, 2023
@LuckyPickleZZ
Copy link
Collaborator Author

I have some questions that we can discuss:

  • Would it be better if we do not enqueue events while there is no notification policy configured in the current organization? I doubt if all events are pushed into the event queue, the conversion from event to notification will be delayed and so does the notification sending.
  • Do we need a pressure test to make sure of the stability?
  • Do we need to refine the configuration values of NotificationProperties? or add some other configurations?

As a temporary solution, I believe that all of the above issues can be solved by reducing the scheduling period of periodic tasks.
If we want to completely solve it, we may be able to schedule events and notifications as asynchronous tasks in periodic tasks.

@MarkPotato777
Copy link
Collaborator

I have some questions that we can discuss:

  • Would it be better if we do not enqueue events while there is no notification policy configured in the current organization? I doubt if all events are pushed into the event queue, the conversion from event to notification will be delayed and so does the notification sending.
  • Do we need a pressure test to make sure of the stability?
  • Do we need to refine the configuration values of NotificationProperties? or add some other configurations?

As a temporary solution, I believe that all of the above issues can be solved by reducing the scheduling period of periodic tasks. If we want to completely solve it, we may be able to schedule events and notifications as asynchronous tasks in periodic tasks.

okay, you could conduct a quick test to confirm the scheduling and batch-size configurations~

@LuckyPickleZZ
Copy link
Collaborator Author

mocked 100W records for notification_event table, and fetch 1W records once.

image

Copy link
Collaborator

@MarkPotato777 MarkPotato777 left a comment

Choose a reason for hiding this comment

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

LGTM

@yhilmare yhilmare merged commit c2ade5e into dev/4.2.2 Nov 21, 2023
11 checks passed
@yhilmare yhilmare deleted the wenmu_422_feat_schedule_failed_notification branch November 21, 2023 01:55
yhilmare pushed a commit that referenced this pull request Jan 15, 2024
#711)

* fix NPE when no recipients configured

* add NotificationPolicyFilter to match expression

* enqueue event when job failed

* fix message would be resent on exception

* remove unused imports

* restore debug

* add region into message

* add UT

* format code

* fix UT failure

* resolve conflicts

* fix UT

* format code

* a failed notification shouldn't block others

* optimize performance

* format code

* response to CR

* use NotificationScheduleConfiguration to replace NotificationSchedules

* fix JPA error

* fix UT

* format code

* format code

* ignore UT

* fix

* rename script

* remove async

* add `taskId` into labels

* add `region` into labels

* remove unused code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants