Bug 30943 - Make background job classes use helpers
Summary: Make background job classes use helpers
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 30360 31351 32779
Blocks: 32370 32781
  Show dependency treegraph
 
Reported: 2022-06-10 13:52 UTC by Tomás Cohen Arazi
Modified: 2023-12-28 20:43 UTC (History)
8 users (show)

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


Attachments
Bug 30943: Simplify background jobs code using helpers (15.42 KB, patch)
2022-09-08 00:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30943: Simplify background jobs code using helpers (15.48 KB, patch)
2022-11-30 08:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30943: Adjust CreateEHoldingsFromBiblios (1.72 KB, patch)
2023-02-01 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30943: Fix test (890 bytes, patch)
2023-02-01 15:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30943: Simplify background jobs code using helpers (15.67 KB, patch)
2023-03-04 23:44 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30943: Adjust CreateEHoldingsFromBiblios (1.72 KB, patch)
2023-03-04 23:45 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30943: Fix test (888 bytes, patch)
2023-03-04 23:45 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30943: Simplify background jobs code using helpers (15.72 KB, patch)
2023-03-10 15:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30943: Adjust CreateEHoldingsFromBiblios (1.77 KB, patch)
2023-03-10 15:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30943: Fix test (943 bytes, patch)
2023-03-10 15:48 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-06-10 13:52:03 UTC
Bug 30360 introduced helper methods for dealing with starting, marking as finished and also handling progress. Our tasks should use them.
Comment 1 Tomás Cohen Arazi 2022-09-08 00:41:07 UTC
Created attachment 140329 [details] [review]
Bug 30943: Simplify background jobs code using helpers

This patch makes the current background jobs classes use the added
helpers in order to simplify them.

No behavior change is expected.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2022-11-29 19:04:37 UTC
I forgot about this one, should've been on 22.11...
Comment 3 Martin Renvoize 2022-11-30 08:02:03 UTC
Created attachment 144340 [details] [review]
Bug 30943: Simplify background jobs code using helpers

This patch makes the current background jobs classes use the added
helpers in order to simplify them.

No behavior change is expected.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2022-11-30 08:05:13 UTC
Oops, I forgot about this one too.

So.. I've signed off.. but I have a query.

This is great that it now ensures we consistently catch the cases where a job has already started and thus we shouldn't continue.. however, we're throwing an exception.

Should we be wrapping calls to start in a try/catch to just skip the job or something instead..?
Comment 5 Tomás Cohen Arazi 2022-11-30 11:26:52 UTC
(In reply to Martin Renvoize from comment #4)
>
> Should we be wrapping calls to start in a try/catch to just skip the job or
> something instead..?

Yes, absolutely. In theory, as written right now, we are hidding those situations by skipping in some cases. My idea was to just explode and handle things correctly.

One example: if a task has already been started, and it is asked to start again, should we proceed? Should we alert?
Comment 6 Jonathan Druart 2022-11-30 12:16:13 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> One example: if a task has already been started, and it is asked to start
> again, should we proceed? Should we alert?

See also bug 32305.
Comment 7 Martin Renvoize 2022-12-02 08:28:48 UTC
OK.. I think this patchset if solid as is if we push the process fix first that checks for status...

Having both process and start methods is slightly confusing at first ;P
Comment 8 Tomás Cohen Arazi 2022-12-02 11:59:57 UTC
(In reply to Martin Renvoize from comment #7)
> OK.. I think this patchset if solid as is if we push the process fix first
> that checks for status...
> 
> Having both process and start methods is slightly confusing at first ;P

I agree the terminology is confusing. But they are really different things, as ->process is really the overloaded method in the job class, and ->start is just marking the task as started. i.e. if we rename process => start, we will still want a (say) ->mark_started method that does what this method does.
Comment 9 Marcel de Rooy 2023-01-27 09:32:11 UTC
Also looked at 32305. Please indicate the dependency (reading the comments)?

Moving to ID for now.
Comment 10 Tomás Cohen Arazi 2023-01-27 09:50:22 UTC
(In reply to Marcel de Rooy from comment #9)
> Also looked at 32305. Please indicate the dependency (reading the comments)?
> 
> Moving to ID for now.

I don't understand why you put it in discussion because someone filed a bug on a different method. It doesn't look like it is related.
Comment 11 Marcel de Rooy 2023-01-27 09:55:12 UTC
Please read the history on both bugs.
Comment 12 Tomás Cohen Arazi 2023-01-27 10:42:14 UTC
(In reply to Marcel de Rooy from comment #11)
> Please read the history on both bugs.

On bug 32305, they discuss the Koha::BackgroundJob->process method, making it not initiate a background job unless its status is 'new()', with different approaches and in discussion, as you noticed.

This bug is only taking care of making already existing helpers be used to reduce the amount of unnecessary code, like this:

-    my $job_progress = 0;
-    $self->started_on(dt_from_string)->progress($job_progress)
-      ->status('started')->store;
+    $self->start;

the place in which they might have an intersection, is that ->start will raise an exception if the status is not new().

We don't have a 'retry' mechanism on failure and we don't have a proper way to deal with stale/wrong messages in rabbit. But I'm pretty sure we'd prefer to explode earlier if a job is asked to be started when it shouldn't.

I'd say, as per comment 4, that we could rename start() => mark_started(). But that's out of the scope of this bug.
Comment 13 Marcel de Rooy 2023-01-30 10:50:02 UTC
Back to SO
Comment 14 Jonathan Druart 2023-02-01 09:03:50 UTC
I am all for this change, but I don't think it should be pushed before we stabilize this area a bit. There are bug fixes with higher priority.

I would like to note 2 things here:
1. the tidy in Koha/BackgroundJob/BatchUpdateItem.pm goes against my config, so we have a problem here if people go after others to tidy differently their code..

2. I was intrigued by the removal of
-    $self->discard_changes;
and it seems safe.
Comment 15 Jonathan Druart 2023-02-01 09:05:15 UTC
Also, as discussed earlier, I would suggest to rename (on a separate bug report) ->start and ->finish to ->mark_as_started and ->mark_as_finished (or anything similar) to remove the ambiguity.
Comment 16 Jonathan Druart 2023-02-01 15:15:54 UTC
Created attachment 145957 [details] [review]
Bug 30943: Adjust CreateEHoldingsFromBiblios
Comment 17 Jonathan Druart 2023-02-01 15:18:24 UTC
Created attachment 145958 [details] [review]
Bug 30943: Fix test
Comment 18 Jonathan Druart 2023-02-01 15:19:28 UTC
Last patch is based on bug 32779. Adding the dependency here to avoid conflicts later. I assume 32779 is going to be pushed before this one as it's a bugfix that needs to be backported.
Comment 19 Victor Grousset/tuxayo 2023-03-04 23:44:05 UTC
Created attachment 147765 [details] [review]
Bug 30943: Simplify background jobs code using helpers

This patch makes the current background jobs classes use the added
helpers in order to simplify them.

No behavior change is expected.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 20 Victor Grousset/tuxayo 2023-03-04 23:45:03 UTC
Created attachment 147766 [details] [review]
Bug 30943: Adjust CreateEHoldingsFromBiblios
Comment 21 Victor Grousset/tuxayo 2023-03-04 23:45:06 UTC
Created attachment 147767 [details] [review]
Bug 30943: Fix test
Comment 22 Victor Grousset/tuxayo 2023-03-04 23:47:33 UTC
Rebased, tests still pass.
Comment 23 Kyle M Hall 2023-03-10 15:48:33 UTC
Created attachment 148078 [details] [review]
Bug 30943: Simplify background jobs code using helpers

This patch makes the current background jobs classes use the added
helpers in order to simplify them.

No behavior change is expected.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2023-03-10 15:48:46 UTC
Created attachment 148079 [details] [review]
Bug 30943: Adjust CreateEHoldingsFromBiblios

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2023-03-10 15:48:49 UTC
Created attachment 148080 [details] [review]
Bug 30943: Fix test

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Tomás Cohen Arazi 2023-03-13 19:05:35 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 27 Matt Blenkinsop 2023-03-14 10:41:28 UTC
Enhancement - not backporting to 22.11.x

Nice work everyone!