Skip to content

Campaign Trigger aommand and Contact export command made interruptible via the signals #14745

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Apr 11, 2025
Merged

Campaign Trigger aommand and Contact export command made interruptible via the signals #14745

merged 17 commits into from
Apr 11, 2025

Conversation

dadarya0
Copy link
Contributor

@dadarya0 dadarya0 commented Mar 17, 2025 •
edited
Loading

Q A
Bug fix? (use the a.b branch) ❌
New feature/enhancement? (use the a.x branch) ✔️
Deprecations? ❌
BC breaks? (use the c.x branch) ❌
Automated tests included? ✔️
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

An exception for when signal is caught.

This improves the commands handling when a SIGTERM signal is sent. This happens when for example you run a command manually, it takes a while and you kill it with ^c. We don't want to kill the commands immediately but we want them to finish gracefully. In other words, we don't want to end up in some inconsistent state. We want the work for 1 batch to be finished and end the command there.

This is also useful when you have some autoscaling mechanism with your workers. When the workers scale down you want to kill the processes in progress ASAP so you save some resources. But you also don't want to cause problems and you want the killed processes to resume work in other workers. This is what this PR is about.

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Testing step would be same as Contact import made interruptible via the signals #12845 for following commands :-
  • mautic:campaigns:trigger
  • mautic:contacts:scheduled_export

@Copilot Copilot AI review requested due to automatic review settings March 17, 2025 05:51
Copilot
Copilot AI reviewed Mar 17, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

@rahuld-dev @dadarya0
Merge pull request mautic#2118 from acquia/MAUT-10807
82e0ab8 
MAUT-10807 : Make long running job resumable - lead.contact_export
@dadarya0 dadarya0 mentioned this pull request Mar 17, 2025
Closed
@codecov Codecov
Copy link

codecov bot commented Mar 17, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 65.71%. Comparing base (383f5fe) to head (1f6cba3).
Report is 18 commits behind head on 7.x.

Files with missing lines Patch % Lines
.../CampaignBundle/Command/TriggerCampaignCommand.php 77.77% 8 Missing ⚠️
...es/CoreBundle/ProcessSignal/ProcessSignalState.php 92.85% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                7.x   #14745      +/-   ##
============================================
+ Coverage     65.70%   65.71%   +0.01%     
- Complexity    34966    34978      +12     
============================================
  Files          2300     2302       +2     
  Lines        140658   140693      +35     
============================================
+ Hits          92420    92462      +42     
+ Misses        48238    48231       -7
Files with missing lines Coverage Δ
...CampaignBundle/Executioner/InactiveExecutioner.php 94.95% <100.00%> (+0.04%) ⬆️
.../CampaignBundle/Executioner/KickoffExecutioner.php 100.00% <100.00%> (ø)
...ampaignBundle/Executioner/ScheduledExecutioner.php 95.23% <100.00%> (+0.03%) ⬆️
app/bundles/CoreBundle/Helper/ExportHelper.php 99.03% <100.00%> (+<0.01%) ⬆️
.../ProcessSignal/Exception/InvalidStateException.php 100.00% <100.00%> (ø)
.../ProcessSignal/Exception/SignalCaughtException.php 100.00% <100.00%> (+100.00%) ⬆️
.../CoreBundle/ProcessSignal/ProcessSignalService.php 88.00% <100.00%> (+46.33%) ⬆️
...adBundle/Command/ContactScheduledExportCommand.php 93.75% <100.00%> (+1.15%) ⬆️
...es/CoreBundle/ProcessSignal/ProcessSignalState.php 92.85% <92.85%> (ø)
.../CampaignBundle/Command/TriggerCampaignCommand.php 86.91% <77.77%> (-1.98%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dadarya0 dadarya0 requested review from a team, fedys and aarohiprasad and removed request for a team March 17, 2025 07:14
fedys
fedys requested changes Mar 17, 2025
View reviewed changes
Copy link
Member

@fedys fedys left a comment

Choose a reason for hiding this comment

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

Please set the target branch to 7.x as this PR is an enhancement.

@dadarya0 dadarya0 changed the base branch from 6.x to 7.x March 17, 2025 08:51
@dadarya0 dadarya0 force-pushed the signal-caught-exception branch from 7a9c6b8 to 38b9ad8 Compare March 17, 2025 09:31
@dadarya0 dadarya0 requested a review from fedys March 17, 2025 09:45
fedys
fedys requested changes Mar 18, 2025
View reviewed changes
Copy link
Member

@fedys fedys left a comment

Choose a reason for hiding this comment

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

Thanks @dadarya0! Please see my suggestion.

@dadarya0 dadarya0 requested a review from fedys March 18, 2025 10:03
fedys
fedys approved these changes Mar 18, 2025
View reviewed changes
Copy link
Member

@fedys fedys left a comment

Choose a reason for hiding this comment

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

Thanks @dadarya0! Good job 👍
Code changes look good to me.

@dadarya0 dadarya0 added the unforking Used for PRs in the Acquia's unforking initiative label Mar 19, 2025
aarohiprasad
aarohiprasad approved these changes Mar 21, 2025
View reviewed changes
Copy link
Contributor

@aarohiprasad aarohiprasad left a comment

Choose a reason for hiding this comment

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

Thanks @dadarya0!

@aarohiprasad
Copy link
Contributor

This PR cannot be directly tested as specified by @escopecz here: #14632 (comment)
Marking as ready-to-commit

@aarohiprasad aarohiprasad added the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Mar 21, 2025
escopecz
escopecz requested changes Mar 21, 2025
View reviewed changes
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

@aarohiprasad Saurabh added the implementation of the Sigterm signal to the commands so now it can be tested. @dadarya0 can you please update the steps to test?

@dadarya0 dadarya0 requested a review from escopecz March 24, 2025 10:26
@dadarya0 dadarya0 changed the title Signal caught exception Campaign Trigger aommand and CXontact export command made interruptible via the signals Mar 24, 2025
@dadarya0
Copy link
Contributor Author

2. Contact import made interruptible via the signals #12845

@aarohiprasad follow testing steps from #12845

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged enhancement Any improvement to an existing feature or functionality and removed ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Mar 24, 2025
@aarohiprasad
Copy link
Contributor

aarohiprasad commented Mar 28, 2025 •
edited
Loading

@dadarya0 this has not been implemented for app/bundles/LeadBundle/Command/ContactScheduledExportCommand.php hence it is not working for mautic:contacts:scheduled_export command

@dadarya0 dadarya0 changed the title Campaign Trigger aommand and CXontact export command made interruptible via the signals Campaign Trigger aommand and Contact export command made interruptible via the signals Apr 1, 2025
@dadarya0
Copy link
Contributor Author

dadarya0 commented Apr 1, 2025

@dadarya0 this has not been implemented for app/bundles/LeadBundle/Command/ContactScheduledExportCommand.php hence it is not working for mautic:contacts:scheduled_export command

@aarohiprasad I have verified that all changes are pushed which code you did not see here ?

@aarohiprasad
Copy link
Contributor

Sorry, this is my fault. I had the other branch with the same name checked out. I verified again.
For mautic:contacts:scheduled_export the following steps were followed:

  1. Scheduled more than 10 exports.
  2. Ran this command and hit Ctrl+C.
  3. Ran it again.
    This was the output.
$ bin/console mautic:contacts:scheduled_export
^CSignal 2 caught.
13:41:38 WARNING   [mautic] Command `mautic:contacts:scheduled_export` exited with status code 143 ["hostname" => "mautic-web","pid" => 27168]
$ bin/console mautic:contacts:scheduled_export
Contact export email(s) sent: 9

For mautic:campaigns:trigger the following steps were followed:

  1. Created a segment with 1000 contacts
  2. Created a campaign to delete all contacts in the segment.
  3. Ran this command and hit Ctrl+C.
  4. Ran it again, interrupted it once again and then ran it again.
    This was the output.
$ bin/console mautic:c:t
Triggering events for campaign 1
Triggering events for newly added contacts
1000 total events(s) to be processed in batches of 100 contacts
    0/1000 [>---------------------------]   0%^CSignal 2 caught.
 1000/1000 [============================] 100%13:50:37 WARNING   [mautic] Command `mautic:campaigns:trigger` exited with status code 143 ["hostname" => "mautic-web","pid" => 27409]
$ bin/console mautic:campaigns:trigger
^CSignal 2 caught.
Triggering events for campaign 1
Triggering events for newly added contacts
900 total events(s) to be processed in batches of 100 contacts
 900/900 [============================] 100%13:50:57 WARNING   [mautic] Command `mautic:campaigns:trigger` exited with status code 143 ["hostname" => "mautic-web","pid" => 27414]
$ bin/console mautic:campaigns:trigger
Triggering events for campaign 1
Triggering events for newly added contacts
800 total events(s) to be processed in batches of 100 contacts
 800/800 [============================] 100%
800 total events were executed
0 total events were scheduled

Triggering scheduled events
0 total events(s) to be processed in batches of 100 contacts

0 total events were executed
0 total events were scheduled

Triggering events for inactive contacts

0 total events were executed
0 total events were scheduled

The PR is working as expected and can be merged. Thanks @dadarya0!

@aarohiprasad aarohiprasad removed the pending-test-confirmation PR's that require one test before they can be merged label Apr 3, 2025
@aarohiprasad aarohiprasad assigned escopecz and unassigned aarohiprasad and dadarya0 Apr 3, 2025
@aarohiprasad aarohiprasad added the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Apr 3, 2025
@escopecz
Copy link
Member

escopecz commented Apr 4, 2025

@nileshlohar there are some conflicts. Please take a look

@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Apr 4, 2025
@nileshlohar nileshlohar removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Apr 7, 2025
@escopecz escopecz added this to the 7.0.0-alpha milestone Apr 11, 2025
@escopecz escopecz merged commit 9c89a81 into mautic:7.x Apr 11, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedys fedys fedys approved these changes

Copilot code review Copilot Copilot left review comments

@escopecz escopecz Awaiting requested review from escopecz

+1 more reviewer

@aarohiprasad aarohiprasad aarohiprasad approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@escopecz escopecz

Labels
enhancement Any improvement to an existing feature or functionality ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged unforking Used for PRs in the Acquia's unforking initiative
Projects
None yet
Milestone
7.0.0-alpha
Development

Successfully merging this pull request may close these issues.
6 participants
@dadarya0 @aarohiprasad @escopecz @fedys @nileshlohar @rahuld-dev