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

Daemon locking improvements #4735

Merged
merged 5 commits into from
Nov 27, 2023
Merged

Daemon locking improvements #4735

merged 5 commits into from
Nov 27, 2023

Conversation

dracos
Copy link
Member

@dracos dracos commented Nov 22, 2023

[skip changelog]

@dracos dracos requested a review from davea November 22, 2023 14:41
Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: 72 lines in your changes are missing coverage. Please review.

Comparison is base (6172310) 85.26% compared to head (d977312) 27.68%.

Files Patch % Lines
perllib/FixMyStreet/Roles/CobrandSLWP.pm 0.00% 19 Missing ⚠️
perllib/FixMyStreet/Cobrand/Brent.pm 0.00% 12 Missing ⚠️
perllib/FixMyStreet/App/Controller/Waste.pm 0.00% 9 Missing ⚠️
perllib/FixMyStreet/Cobrand/Bromley.pm 27.27% 7 Missing and 1 partial ⚠️
perllib/FixMyStreet/Cobrand/Peterborough.pm 0.00% 7 Missing ⚠️
perllib/FixMyStreet/Cobrand/Bexley.pm 0.00% 6 Missing ⚠️
perllib/FixMyStreet/Cobrand/CentralBedfordshire.pm 0.00% 5 Missing ⚠️
perllib/FixMyStreet/Script/Reports.pm 86.66% 0 Missing and 2 partials ⚠️
perllib/FixMyStreet/Cobrand/Merton.pm 0.00% 1 Missing ⚠️
perllib/FixMyStreet/Cobrand/Westminster.pm 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4735       +/-   ##
===========================================
- Coverage   85.26%   27.68%   -57.59%     
===========================================
  Files         340      335        -5     
  Lines       24428    24157      -271     
  Branches     4631     4585       -46     
===========================================
- Hits        20829     6688    -14141     
- Misses       2189    16766    +14577     
+ Partials     1410      703      -707     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@davea davea left a comment

Choose a reason for hiding this comment

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

Looks like a nice and neat solution to a thorny problem. I like the change that breaks open311_update_missing_data out into its own thing - it was definitely getting a bit heavy in places.

One little copy/paste thing in the SQL migration is all, otherwise 👍

db/schema_0086-add-processing-to-send-state.sql Outdated Show resolved Hide resolved
Add a processing state so the daemons still do not clash, but unlock the
row during actual processing in case that takes a long time. Changes to
prevent race conditions in processing coming up next.
Introduce a new open311_update_missing_data cobrand hook to be used to
add/save any missing data to the report before sending via Open311, that
is wrapped in a transaction. open311_extra_data_include should now only
be used to add columns for sending, not to edit the row in any way.

When sending via Open311, any changes made pre-send (after
update_missing_data) are now automatically discarded after sending.
@dracos dracos force-pushed the daemon-improve-locking branch from bfd6b2b to d977312 Compare November 27, 2023 14:35
@dracos dracos merged commit d977312 into master Nov 27, 2023
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants