Bug 32573 - background_jobs_worker.pl should ACK a message before it forks and runs the job
Summary: background_jobs_worker.pl should ACK a message before it forks and runs the job
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 32393 32558
  Show dependency treegraph
 
Reported: 2023-01-05 16:32 UTC by Kyle M Hall
Modified: 2023-01-31 14:10 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03


Attachments
Bug 32573: Send ACK message to RabbitMQ before handling the job (2.96 KB, patch)
2023-01-05 16:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32573: Send ACK message to RabbitMQ before handling the job (1.31 KB, patch)
2023-01-24 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32573: Send ACK message to RabbitMQ before handling the job (3.03 KB, patch)
2023-01-24 08:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32573: Send ACK message to RabbitMQ before handling the job (1.52 KB, patch)
2023-01-24 12:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32573: Send ACK message to RabbitMQ before handling the job (3.09 KB, patch)
2023-01-24 13:00 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 Kyle M Hall 2023-01-05 16:32:37 UTC
Splitting off this functionality from bug 32558. This is the comment that started this discussion: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558#c15

From the O'Reilly book Mobile and Web Messaging:
---
By default, the STOMP broker will consider the message automatically acknowledged when it is delivered to the consumer.

However, there are cases in which the consumer may prefer to explicitly acknowledge the message. It leaves a window of opportunity to determine whether it can handle the message or not. For example, the client needs to write the message payload in a data store. There may be issues with opening a connection to the data store and the client could choose to acknowledge the message only after having successfully written its body to the data store. In case of failure, it will instead nack the message (explicitly refuse to take ownership of it). When the STOMP broker is informed of this negative acknowledgment, it may then decide to deliver the message to another consumer subscribed to the destination or try again some time later depending on its configuration.
---

From https://www.rabbitmq.com/stomp.html

---
RabbitMQ STOMP plugin supports auto, client, and client-individual subscription headers that affect how ACK on NACK operations work.

The auto mode uses automatic acknowledgements. The client mode is manual (client-driven) acknowledgements of multiple messages at once. The client-individual is for message-by-message manual acknowledgement.
---

If ACK is meant to be sent post-work, why would automatic acknowledgement exist?  The interpretation that an ACK is meant to be sent after handling the processing of a job doesn't make sense.

In summary, we should be sending back ACK messages as soon as we've successfully processed ( e.g. read and decoded the data ) of a frame. Only then should we process it.
Comment 1 Kyle M Hall 2023-01-05 16:37:41 UTC
Created attachment 145066 [details] [review]
Bug 32573: Send ACK message to RabbitMQ before handling the job

Splitting off this functionality from bug 32558. This is the comment that started this discussion: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558#c15

From the O'Reilly book Mobile and Web Messaging:
---
By default, the STOMP broker will consider the message automatically acknowledged when it is delivered to the consumer.

However, there are cases in which the consumer may prefer to explicitly acknowledge the message. It leaves a window of opportunity to determine whether it can handle the message or not. For example, the client needs to write the message payload in a data store. There may be issues with opening a connection to the data store and the client could choose to acknowledge the message only after having successfully written its body to the data store. In case of failure, it will instead nack the message (explicitly refuse to take ownership of it). When the STOMP broker is informed of this negative acknowledgment, it may then decide to deliver the message to another consumer subscribed to the destination or try again some time later depending on its configuration.
---

From https://www.rabbitmq.com/stomp.html

---
RabbitMQ STOMP plugin supports auto, client, and client-individual subscription headers that affect how ACK on NACK operations work.

The auto mode uses automatic acknowledgements. The client mode is manual (client-driven) acknowledgements of multiple messages at once. The client-individual is for message-by-message manual acknowledgement.
---

If ACK is meant to be sent post-work, why would automatic acknowledgement exist?  The interpretation that an ACK is meant to be sent after handling the processing of a job doesn't make sense.

In summary, we should be sending back ACK messages as soon as we've successfully processed ( e.g. read and decoded the data ) of a frame. Only then should we process it.

Test Plan:
1) Apply this patch
2) Launch background_jobs_worker.pl
3) Note no changes in background job processing
Comment 2 Jonathan Druart 2023-01-06 09:25:56 UTC
I've tried this when working on bug 32481 but it didn't help.
Comment 3 David Cook 2023-01-08 23:49:15 UTC
I don't think there's 100% perfect answer for this topic.

If you look at "Message acknowledgement" on the following work queue tutorial, it explicitly says that you should ACK after the work task has completed. It does note how the 30 minute default timeout can be an issue.

https://www.rabbitmq.com/tutorials/tutorial-two-python.html

"Doing a task can take a few seconds, you may wonder what happens if a consumer starts a long task and it terminates before it completes. With our current code once RabbitMQ delivers message to the consumer, it immediately marks it for deletion. In this case, if you terminate a worker, the message it was just processing is lost. The messages that were dispatched to this particular worker but were not yet handled are also lost.

But we don't want to lose any tasks. If a worker dies, we'd like the task to be delivered to another worker.

In order to make sure a message is never lost, RabbitMQ supports message acknowledgments. An ack(nowledgement) is sent back by the consumer to tell RabbitMQ that a particular message had been received, processed and that RabbitMQ is free to delete it.

If a consumer dies (its channel is closed, connection is closed, or TCP connection is lost) without sending an ack, RabbitMQ will understand that a message wasn't processed fully and will re-queue it. If there are other consumers online at the same time, it will then quickly redeliver it to another consumer. That way you can be sure that no message is lost, even if the workers occasionally die."
Comment 4 David Cook 2023-01-08 23:51:01 UTC
If you were doing a big batch record modification and you suffered a power outage on the machine where the background job worker is running, then that task can't be re-started since the RabbitMQ message was already ACKed before the real work began.

(Of course, in the case of Koha, we have the "database failover", so in theory it might get recovered, but it's not standard design.)
Comment 5 Kyle M Hall 2023-01-09 11:59:06 UTC
(In reply to David Cook from comment #4)
> If you were doing a big batch record modification and you suffered a power
> outage on the machine where the background job worker is running, then that
> task can't be re-started since the RabbitMQ message was already ACKed before
> the real work began.
> 
> (Of course, in the case of Koha, we have the "database failover", so in
> theory it might get recovered, but it's not standard design.)

Considering batch record modifications are not atomic, I don't think that's necessarily a compelling argument.

I can't see any scenario where either:
A) We should ack before processing. We are stymying Koha's ability to do background processing by running extrememly long processes in serial with no way to fork multiple background jobs at one.

B) RabbitMQ/STOMP is the wrong tool for the job and we should remove it's use from Koha.
Comment 6 Kyle M Hall 2023-01-09 12:04:36 UTC
> I can't see any scenario where either:
> A) We should ack before processing. We are stymying Koha's ability to do
> background processing by running extrememly long processes in serial with no
> way to fork multiple background jobs at one.
> 
> B) RabbitMQ/STOMP is the wrong tool for the job and we should remove it's
> use from Koha.

Actually, I can revise A a bit. We can fork and give the existing rabbit connection to the child process, and have the parent start a new connection to pass off again. That mains we can parallel process without acking first. It doesn't solve the 30 minutes time limit ( which is why I'm going to continue advocating for acking first ) but it does improve the situation greatly.
Comment 7 Nick Clemens 2023-01-09 15:28:55 UTC
I think for Koha we should ack as soon as we receive the job from RabbitMQ

We maintain our own DB table for background jobs, and this is our source of truth. We update this table with the results of a job, and use this table to determine status.

If we maintained Rabbit as the source of truth, we might want to keep jobs there until processed, but would then need to check Rabbit for the job statuses

Most of the jobs we are sending won't benefit from a retry if they fail - a bad record won't index no matter how many times we try. If we have jobs that are network based, and may fail because of connection issues, we should handle the re-queuing in the job itself - so ack the job - try it, if fail then queue it as new - then we can note the failure/requeue in Koha as well

It looks like the Stomp protocol recommendations differ from the Net::Stomp and RabbitMQ docs, but I also don't see a negative side effect of ack before process
Comment 8 David Cook 2023-01-09 22:32:44 UTC
(In reply to Kyle M Hall from comment #5)
> Considering batch record modifications are not atomic, I don't think that's
> necessarily a compelling argument.

I think that shows that batch modifications are problematically designed. Consider my comment on bug 32278 that talks about how having item/record numbers saved into tracked batches. We could then process those batches in an atomic fashion. 

> I can't see any scenario where either:
> A) We should ack before processing. We are stymying Koha's ability to do
> background processing by running extrememly long processes in serial with no
> way to fork multiple background jobs at one.
> 
> B) RabbitMQ/STOMP is the wrong tool for the job and we should remove it's
> use from Koha.
>
> Actually, I can revise A a bit. We can fork and give the existing rabbit
> connection to the child process, and have the parent start a new connection
> to pass off again. That mains we can parallel process without acking first.
> It doesn't solve the 30 minutes time limit ( which is why I'm going to
> continue advocating for acking first ) but it does improve the situation
> greatly.

From day one, I've advocated for multiple worker processes. Frequently, you start multiple worker processes independently of the web app. I have other applications where I run the worker processes in standalone containers and I can start as many as I want to do the work based on the workload of that application (based on the computational power I have available of course). I also put them together in pipelines which means I can process lots of data very quickly. Those apps are way more data heavy than Koha. I still ACK after I process my RabbitMQ messages. 

In Koha, we shouldn't be having extremely long running processes. We don't do any 1 thing that is really computationally that heavy; we're not running complex image manipulation on high resolution medical images with terabytes worth of data. We're just writing inefficient suboptimal code. 

RabbitMQ isn't the wrong tool for the job, but I think the Koha community doesn't have the knowledge/experience to know how to use it (well). And I'll include myself in that camp. I think that I've probably used RabbitMQ more than anyone else in the Koha community, and I'd still consider myself a RabbitMQ newbie.
Comment 9 David Cook 2023-01-09 22:42:24 UTC
(In reply to Nick Clemens from comment #7)
> I think for Koha we should ack as soon as we receive the job from RabbitMQ
> 
> We maintain our own DB table for background jobs, and this is our source of
> truth. We update this table with the results of a job, and use this table to
> determine status.

As you say, it's the "result store". As far as task queues go, they typically have a database for a result store. That is separate from the messaging layer. 

> If we maintained Rabbit as the source of truth, we might want to keep jobs
> there until processed, but would then need to check Rabbit for the job
> statuses

RabbitMQ is the messaging layer. It's not a data layer. 

> Most of the jobs we are sending won't benefit from a retry if they fail - a
> bad record won't index no matter how many times we try. If we have jobs that
> are network based, and may fail because of connection issues, we should
> handle the re-queuing in the job itself - so ack the job - try it, if fail
> then queue it as new - then we can note the failure/requeue in Koha as well

If a bad record won't index, then you fail the job. That still has nothing to do with the messaging layer. 

Once you fail the job and record that in the result store, you ack the message to tell the messaging layer that you've handled that message. 

In terms of re-queuing, Koha doesn't really support that at the moment, since there is no task scheduler. You can't re-queue for 5 minutes later to see if the network has stabilized. That would be future functionality. 

> It looks like the Stomp protocol recommendations differ from the Net::Stomp
> and RabbitMQ docs, but I also don't see a negative side effect of ack before
> process

Can you refer to that recommendation specifically? I'm not sure what you mean.
Comment 10 David Cook 2023-01-09 23:28:30 UTC
But... as I note at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27783#c64 my libraries are not heavy users of background tasks at this point.

It sounds like Bywater are having the most struggles with the limitations of current background job implementation, and probably with the batch record modification?

With the current code, I can see how that could be a problem. It would probably take quite a bit of re-engineering to break up a batch record modification into chunks that could be easily digested. 

So I have a compromise...

Celery has an "acks_late" option for its tasks. Why don't we implement that option on a per-task basis? 

For tasks that are idempotent, we can keep them with acks_late like we have now. But for tasks that are more likely to run long (like the batch mods), then we could ack early? That would be fairly trivial to code.

--

For ack early jobs, we could also have a cronjob that marks them as failed after a configurable time (i.e. a time much longer than 30 minutes). Like Minion's stuck_after: https://metacpan.org/pod/Minion#stuck_after
Comment 11 Tomás Cohen Arazi 2023-01-10 13:51:07 UTC
(In reply to David Cook from comment #8)
> 
> RabbitMQ isn't the wrong tool for the job

The question is: are we better with just polling? What do we lose?
Comment 12 David Cook 2023-01-10 22:55:47 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> (In reply to David Cook from comment #8)
> > 
> > RabbitMQ isn't the wrong tool for the job
> 
> The question is: are we better with just polling? What do we lose?

For starters, I'd refer people to Jonathan's comment from July: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30654#c17

Database polling has a number of shortcomings. It's slower and less efficient. It's not easily scalable. You have to implement all the messaging and concurrent processing logic from scratch. It's a naïve method of queue processing that people resort to because they understand relational databases and don't understand message queues.

I think that one could argue that Koha is allowed to be slow, inefficient, and not scalable. The amount of background processing that Koha needs to do is going to be minor in comparison to many other systems. 

So I imagine that Koha could get along just fine with a bespoke database polling background job processing system. In hindsight, we probably should've done that originally. Perhaps we were too ambitious in our desire to use modern tools and standards. 

But I think that just using database polling would be a backwards step for the project overall at this point. Koha's usage of RabbitMQ is improving. We devs are collectively improving. 

I think we really only have 2 remaining problems:
1. How to handle jobs that run longer than the RabbitMQ default 30 minute ack timeout (I think the "ack_late" for some jobs is a good compromise to handle this - alternatively Koha implementors can tune their own RabbitMQ installations if they find the default is too low)
2. How to have multiple workers when using Koha packages (this is easy with RabbitMQ, but the existing database polling failover makes this significantly harder, but it's a solvable problem)
Comment 13 David Cook 2023-01-10 23:10:14 UTC
I was looking back at the origin of the database failover, and I think Julian makes an interesting point at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22417#c249

Maybe we could offer two options via configuration. Database polling or RabbitMQ.

However, for database polling, we should try to provide at least a minimum of similar functionality. That is to say, "misc/background_jobs_worker.pl" would just be a worker. Instead of connecting to RabbitMQ, it could connect to our own bespoke job server. 

That bespoke job server could use an event loop to poll the database for jobs and check that it has workers that can accept jobs. It's not that hard to write that code.
Comment 14 David Cook 2023-01-10 23:15:57 UTC
Regarding the subject of early ACK and late ACK, consider the following RabbitMQ discussion between a user and 2 key RabbitMQ devs (both I've spoken to personally in the past):

https://github.com/rabbitmq/rabbitmq-server/discussions/4030

Luke's recommendation is to update the global consumer timeout.

Michael mentions most consumers shouldn't need longer than 30 minutes to process a message, but for operations that are expected to take longer than 30 minutes - you can ACK early and use a separate progress metric. 

Since we're using the database result store, end users can use that as a progress metric. And if we have a cronjob that runs periodically, we can evaluate the start time and fail tasks that run longer than our own configurable timeout. 

I think that would resolve this ACK issue perfectly.
Comment 15 David Cook 2023-01-10 23:16:30 UTC
I'm curious what Kyle thinks about it. Whether he wants to update his patches, or if folk want me to provide alternative patches.
Comment 16 David Cook 2023-01-10 23:23:23 UTC
Another thought...

What about imposing limits on the size of batches? It's common to see this on major commercial systems. Some social media sites will only allow you to choose 50 items for an operation at a time. 

Related to that would be breaking up operations into multiple batches but I know that's easier said than done at this point.
Comment 17 Tomás Cohen Arazi 2023-01-10 23:32:10 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> (In reply to David Cook from comment #8)
> > 
> > RabbitMQ isn't the wrong tool for the job
> 
> The question is: are we better with just polling? What do we lose?

I didn't intend to restart this conversation. Just to start over on which benefits it offers and why we should go full MQ. I think we never entirely implement it correctly.

Regarding the ack discussion, it feels to me that many of our problems are related to the 'ack last' behavior. As you said, we have our own way of tracking progress and termination, so we are safe to just ack and leave that to Koha.

I've been pondering having separate workers for polling and rabbit respectively. It would simplify things a lot.
Comment 18 David Cook 2023-01-10 23:56:50 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> Regarding the ack discussion, it feels to me that many of our problems are
> related to the 'ack last' behavior. As you said, we have our own way of
> tracking progress and termination, so we are safe to just ack and leave that
> to Koha.

Well, I think the end user can track the progress, but we don't currently have a systematic way of catching "stuck" jobs. But that's a failure scenario that should hopefully be rarer than consumer acknowledgement timeouts. We'll get there. I'm curious to hear what Michael and Luke from RabbitMQ advise...
 
> I've been pondering having separate workers for polling and rabbit
> respectively. It would simplify things a lot.

That does sound interesting. Could you explain that one more?
Comment 19 David Cook 2023-01-11 00:03:23 UTC
(In reply to David Cook from comment #18)
> > I've been pondering having separate workers for polling and rabbit
> > respectively. It would simplify things a lot.
> 
> That does sound interesting. Could you explain that one more?

Actually, I think that I might know what you mean. If we had separate processes for polling and rabbit, people who prefer polling could use polling, and people who prefer rabbit could use rabbit. 

That could make things easier, as concurrent processing for polling and rabbit needs to be handled differently. And acknowledgements may or may not be used with polling...
Comment 20 Kyle M Hall 2023-01-11 15:50:56 UTC
Nick asked me to post this

https://stomp.github.io/stomp-specification-1.2.html#ACK
---
ACK is used to acknowledge consumption of a message from a subscription using client or client-individual acknowledgment. Any messages received from such a subscription will not be considered to have been consumed until the message has been acknowledged via an ACK.
---

The stomp protocol doesn't specify that an ACK is meant to indicate the completion of whatever activity a message would require. An ACK is only meant to verify that the client received the message.
Comment 21 Kyle M Hall 2023-01-11 18:38:25 UTC
(In reply to David Cook from comment #19)
> (In reply to David Cook from comment #18)
> > > I've been pondering having separate workers for polling and rabbit
> > > respectively. It would simplify things a lot.
> > 
> > That does sound interesting. Could you explain that one more?
> 
> Actually, I think that I might know what you mean. If we had separate
> processes for polling and rabbit, people who prefer polling could use
> polling, and people who prefer rabbit could use rabbit. 
> 
> That could make things easier, as concurrent processing for polling and
> rabbit needs to be handled differently. And acknowledgements may or may not
> be used with polling...

I get where you are coming from, but the failover to polling has definitely provided valuable. If we ACK first, it's not too difficult to handle both in one script. If we continue to ACK last, it's more complicated but still possible. I'd say we need to make the decision on where to ACK and the rest of the plan will follow from that.
Comment 22 David Cook 2023-01-15 23:32:19 UTC
(In reply to Kyle M Hall from comment #20)
> Nick asked me to post this
> 
> https://stomp.github.io/stomp-specification-1.2.html#ACK
> ---
> ACK is used to acknowledge consumption of a message from a subscription
> using client or client-individual acknowledgment. Any messages received from
> such a subscription will not be considered to have been consumed until the
> message has been acknowledged via an ACK.
> ---
> 
> The stomp protocol doesn't specify that an ACK is meant to indicate the
> completion of whatever activity a message would require. An ACK is only
> meant to verify that the client received the message.

The spec doesn't seem to strictly define "consume". I imagine that they've left it intentionally ambiguous so that they don't prescribe the behaviour of message consumers.
Comment 23 David Cook 2023-01-15 23:36:51 UTC
(In reply to Kyle M Hall from comment #21)
> I'd say we need to make the decision on where to ACK and the rest
> of the plan will follow from that.

Jonathan has put in the early ACK into bug 32393 at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32393#c24

I don't know that adding it to that particular patch set is the best idea, but I'll leave it up to you guys to sort it out. 

I'm not a heavy user of RabbitMQ in Koha at this point, so happy for you folk to make the decision and do the work. If it's an issue later, we can deal with it later.
Comment 24 Marcel de Rooy 2023-01-20 10:23:54 UTC
See also 32393. It also moves the ack before processing the job.
Comment 25 Jonathan Druart 2023-01-24 08:13:37 UTC
Created attachment 145594 [details] [review]
Bug 32573: Send ACK message to RabbitMQ before handling the job

Splitting off this functionality from bug 32558. This is the comment that started this discussion: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558#c15

From the O'Reilly book Mobile and Web Messaging:

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2023-01-24 08:14:21 UTC
Created attachment 145595 [details] [review]
Bug 32573: Send ACK message to RabbitMQ before handling the job

Splitting off this functionality from bug 32558. This is the comment that started this discussion: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558#c15

From the O'Reilly book Mobile and Web Messaging:
---
By default, the STOMP broker will consider the message automatically acknowledged when it is delivered to the consumer.

However, there are cases in which the consumer may prefer to explicitly acknowledge the message. It leaves a window of opportunity to determine whether it can handle the message or not. For example, the client needs to write the message payload in a data store. There may be issues with opening a connection to the data store and the client could choose to acknowledge the message only after having successfully written its body to the data store. In case of failure, it will instead nack the message (explicitly refuse to take ownership of it). When the STOMP broker is informed of this negative acknowledgment, it may then decide to deliver the message to another consumer subscribed to the destination or try again some time later depending on its configuration.
---

From https://www.rabbitmq.com/stomp.html

---
RabbitMQ STOMP plugin supports auto, client, and client-individual subscription headers that affect how ACK on NACK operations work.

The auto mode uses automatic acknowledgements. The client mode is manual (client-driven) acknowledgements of multiple messages at once. The client-individual is for message-by-message manual acknowledgement.
---

If ACK is meant to be sent post-work, why would automatic acknowledgement exist?  The interpretation that an ACK is meant to be sent after handling the processing of a job doesn't make sense.

In summary, we should be sending back ACK messages as soon as we've successfully processed ( e.g. read and decoded the data ) of a frame. Only then should we process it.

Test Plan:
1) Apply this patch
2) Launch background_jobs_worker.pl
3) Note no changes in background job processing

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Marcel de Rooy 2023-01-24 12:11:36 UTC
QAing
Comment 28 Marcel de Rooy 2023-01-24 12:25:06 UTC
Created attachment 145608 [details] [review]
Bug 32573: Send ACK message to RabbitMQ before handling the job

Splitting off this functionality from bug 32558.

In summary, we should be sending back ACK messages as soon as we've successfully processed
( e.g. read and decoded the data ) of a frame. Only then should we process it.

Test plan:
1) Apply this patch
2) Launch background_jobs_worker.pl
3) Note no changes in background job processing

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2023-01-24 12:26:23 UTC
Had an issue with the commit message being truncated. I only kept the summary now,  the arguments from O'Reilly etc. are on this report.
Comment 30 Jonathan Druart 2023-01-24 13:00:40 UTC
Created attachment 145612 [details] [review]
Bug 32573: Send ACK message to RabbitMQ before handling the job

Splitting off this functionality from bug 32558. This is the comment that started this discussion: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32558#c15

From the O'Reilly book Mobile and Web Messaging:
---
By default, the STOMP broker will consider the message automatically acknowledged when it is delivered to the consumer.

However, there are cases in which the consumer may prefer to explicitly acknowledge the message. It leaves a window of opportunity to determine whether it can handle the message or not. For example, the client needs to write the message payload in a data store. There may be issues with opening a connection to the data store and the client could choose to acknowledge the message only after having successfully written its body to the data store. In case of failure, it will instead nack the message (explicitly refuse to take ownership of it). When the STOMP broker is informed of this negative acknowledgment, it may then decide to deliver the message to another consumer subscribed to the destination or try again some time later depending on its configuration.
---

From https://www.rabbitmq.com/stomp.html

---
RabbitMQ STOMP plugin supports auto, client, and client-individual subscription headers that affect how ACK on NACK operations work.

The auto mode uses automatic acknowledgements. The client mode is manual (client-driven) acknowledgements of multiple messages at once. The client-individual is for message-by-message manual acknowledgement.
---

If ACK is meant to be sent post-work, why would automatic acknowledgement exist?  The interpretation that an ACK is meant to be sent after handling the processing of a job doesn't make sense.

In summary, we should be sending back ACK messages as soon as we've successfully processed ( e.g. read and decoded the data ) of a frame. Only then should we process it.

Test Plan:
1) Apply this patch
2) Launch background_jobs_worker.pl
3) Note no changes in background job processing

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Jonathan Druart 2023-01-24 13:00:53 UTC
(In reply to Marcel de Rooy from comment #29)
> Had an issue with the commit message being truncated. I only kept the
> summary now,  the arguments from O'Reilly etc. are on this report.

Reattached with the commit message.
Comment 32 Tomás Cohen Arazi 2023-01-27 18:22:09 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 33 Matt Blenkinsop 2023-01-31 14:10:22 UTC
Nice work everyone!

Pushed to stable for 22.11.x