Bug 30360 introduced helper methods for dealing with starting, marking as finished and also handling progress. Our tasks should use them.
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>
I forgot about this one, should've been on 22.11...
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>
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..?
(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?
(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.
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
(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.
Also looked at 32305. Please indicate the dependency (reading the comments)? Moving to ID for now.
(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.
Please read the history on both bugs.
(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.
Back to SO
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.
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.
Created attachment 145957 [details] [review] Bug 30943: Adjust CreateEHoldingsFromBiblios
Created attachment 145958 [details] [review] Bug 30943: Fix test
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.
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>
Created attachment 147766 [details] [review] Bug 30943: Adjust CreateEHoldingsFromBiblios
Created attachment 147767 [details] [review] Bug 30943: Fix test
Rebased, tests still pass.
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>
Created attachment 148079 [details] [review] Bug 30943: Adjust CreateEHoldingsFromBiblios Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 148080 [details] [review] Bug 30943: Fix test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 23.05. Nice work everyone, thanks!
Enhancement - not backporting to 22.11.x Nice work everyone!