Added has_documents as an argument for shatter-job-rows tasks#4784
Added has_documents as an argument for shatter-job-rows tasks#4784
has_documents as an argument for shatter-job-rows tasks#4784Conversation
CrystalPea
left a comment
CrystalPea
left a comment
There was a problem hiding this comment.
I left a few comments - will review again when there are tests 🙌🏼
rparke
force-pushed
the
rp-add-queue-for-sendfiles-by-email-process-job-row
branch
from
29489a2 to
8f2b07e
Compare
…jobs-save` and `api-worker-jobs-save-documents` when adding the new queues and new worker types alphagov/notifications-aws#2884 we need to ensure that new workers can startup properly without any issues. This means temporarily configuring each new worker type to take tasks from existing queues, before upgrading `notifications-api` #4784 to populate, and take messages off the new queues.
app/celery/tasks.py
Outdated
| ( | ||
| template_type, | ||
| args_kwargs_seq, | ||
| with_file, |
There was a problem hiding this comment.
I think you have to split this line out and release it in a followup - you can't risk sending a shatter_job_rows call with a third argument that might be picked up by a worker that doesn't yet know about the third argument.
There was a problem hiding this comment.
rparke
force-pushed
the
rp-add-queue-for-sendfiles-by-email-process-job-row
branch
3 times, most recently
from
a26e16f to
197c1a0
Compare
rparke
changed the title
save_email task on a separate queue - ten…has_documents as an argument for shatter-job-rows tasks
rparke
force-pushed
the
rp-add-queue-for-sendfiles-by-email-process-job-row
branch
from
197c1a0 to
bd99067
Compare
CrystalPea
left a comment
CrystalPea
left a comment
There was a problem hiding this comment.
Hey, this looks good - I would love to chat about naming though. I feel some of the team knowledge/process got lost along the way 😳
app/celery/scheduled_tasks.py
Outdated
| current_app.logger.info("Processing missing row %(job_row_number)s for job %(job_id)s", extra, extra=extra) | ||
| process_job_row(template.template_type, task_args_kwargs, str(job.service_id)) | ||
| process_job_row( | ||
| template.template_type, task_args_kwargs, has_documents=False, message_group_id=str(job.service_id) |
There was a problem hiding this comment.
Calling the files documents didn't sit right with me, but I couldn't explain it well before. Now it came back to me.
We did this piece of work a few years back with @karlchillmaid where we found naming the same objects/processes differently when it's user facing, and differently in code has led to confusion and extra team effort with things like: untangling what is what, finding the right code, knowing how to write content like errors and documentation, write to users when on support etc.
So where we could, we renamed things so they are consistent between user-facing side and code side. We couldn't easily change things like repository names or database stuff, but we did change a lot of function and variable names, and had a policy to keep this consistency as much as possible going forward.
I think as the team got bigger a couple years ago, maybe this wasn't conveyed well. I will try to prepare something for the next team demo 🙌🏼 .
But yeah, I saw Rob wanted to call this arg with_documents instead of with_files, probably because that's how the new queue and the repository are named. But to the users, we always talk about files, not documents. For this reason, I would be up for us calling them files wherever possible, and that's why I tried calling the feature "Send files by email via UI" - because this is how we would present it to our users.
There was a problem hiding this comment.
I'm happy to change it to has_files do we need to change the queue name?
There was a problem hiding this comment.
It would be good to change it, but no biggie if it can't be done - those queue names are quite cryptic anyway 😅 . I was wondering about maybe adding comments to where they are defined, to explain what each one does.
…jobs-save` and `api-worker-jobs-save-documents` when adding the new queues and new worker types alphagov/notifications-aws#2884 we need to ensure that new workers can startup properly without any issues. This means temporarily configuring each new worker type to take tasks from existing queues, before upgrading `notifications-api` #4784 to populate, and take messages off the new queues.
adds branching logic to put `save-email` tasks with `has_documents` true onto the dedicated queue for emails with files, `database-tasks-documents` This needs deploying to all workers before #4795 can be deployed, otherwise workers will get called with an argument that they can't handle.
rparke
force-pushed
the
rp-add-queue-for-sendfiles-by-email-process-job-row
branch
from
45aae66 to
b4ba539
Compare
…jobs-save` and `api-worker-jobs-save-documents` when adding the new queues and new worker types alphagov/notifications-aws#2884 we need to ensure that new workers can startup properly without any issues. This means temporarily configuring each new worker type to take tasks from existing queues, before upgrading `notifications-api` #4784 to populate, and take messages off the new queues.
3 participants
adds branching logic to put
save-emailtasks withhas_documentstrue onto the dedicated queue for emails with files,database-tasks-documentsThis needs deploying to all workers before #4795 can be deployed, otherwise workers will get called with an argument that they can't handle.
TODO: