Bug 32395 - Why do we send jobs params to the broker
Summary: Why do we send jobs params to the broker
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 32305 22417 32393
Blocks: 35092
  Show dependency treegraph
 
Reported: 2022-12-02 14:07 UTC by Jonathan Druart
Modified: 2023-10-18 14:50 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 32395: Do not send job's parameters to the broker (2.63 KB, patch)
2022-12-06 09:31 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-12-02 14:07:43 UTC
We are:
- Building the parameters
- Insert a job and its parameters in DB
- Send a message to the broken with the parameters
- Retrieve the message from the worker
- Process the job using the parameter from the broker

Why don't we simply pass the job_id to the broker, then retrieve the parameters from the DB in the worker?
Comment 1 Jonathan Druart 2022-12-02 14:12:21 UTC
We would have avoided all the JSON encoding/decoding stuffs we had recently, and bug 32393.
Comment 2 David Cook 2022-12-04 22:41:11 UTC
(In reply to Jonathan Druart from comment #0)
> Why don't we simply pass the job_id to the broker, then retrieve the
> parameters from the DB in the worker?

In theory, a loosely coupled message consumer might not have database access to the app, so you'd want to encapsulate everything it needs in the message.

However, the system designed for Koha is tightly coupled, so yeah... we could just pass the job_id in the message and have the worker fetch the details. It adds more database I/O, but it probably wouldn't be significant. In some cases, it could actually be more efficient. 

I don't know what the best practice is, and I imagine we should just do whatever makes sense for us.
Comment 3 David Cook 2022-12-04 22:44:48 UTC
In the context of bug 32393 I see your point even more clearly.

Since we're tracking jobs in the database and not doing a fire and forget pattern, it probably does make sense to just pass the job_id, because the message really is  just "oh hey, I've got a job. Here's it's ID. Grab it and do stuff with it."
Comment 4 Jonathan Druart 2022-12-06 09:31:05 UTC
Created attachment 144434 [details] [review]
Bug 32395: Do not send job's parameters to the broker
Comment 5 Jonathan Druart 2022-12-06 09:33:00 UTC
The logic is here, but I don't think this is enough. It must be more robust and clear. Do we allow other parameters than job_id? Should we simply pass the id as a string an completely remove JSON?

I don't think so, we should keep flexibility and keep in mind we could have jobs that don't go through the DB.
Comment 6 Tomás Cohen Arazi 2023-01-27 10:53:36 UTC
(In reply to Jonathan Druart from comment #5)
> The logic is here, but I don't think this is enough. It must be more robust
> and clear. Do we allow other parameters than job_id? Should we simply pass
> the id as a string an completely remove JSON?
> 
> I don't think so, we should keep flexibility and keep in mind we could have
> jobs that don't go through the DB.

Let's pass a JSON with the job_id.