[Bug 30943] New: Make background job classes use helpers
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Bug ID: 30943 Summary: Make background job classes use helpers Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org Bug 30360 introduced helper methods for dealing with starting, marking as finished and also handling progress. Our tasks should use them. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Depends on| |30360 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30360 [Bug 30360] Add helper methods to Koha::BackgroundJobs -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |31351 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31351 [Bug 31351] Worker dies on reindex job when operator last name/first name/branch name contains non-ASCII chars -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 140329 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=140329&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |32370 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32370 [Bug 32370] Provide a generic set of tools for JSON fields -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com, | |kyle@bywatersolutions.com, | |m.de.rooy@rijksmuseum.nl, | |martin.renvoize@ptfs-europe | |.com --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I forgot about this one, should've been on 22.11... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #140329|0 |1 is obsolete| | --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 144340 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144340&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #4 from 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..? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Also looked at 32305. Please indicate the dependency (reading the comments)? Moving to ID for now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Please read the history on both bugs. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Back to SO -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |32781 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32781 [Bug 32781] CreateEHoldingsFromBiblios not dealing with non-existent package correcly -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 145957 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145957&action=edit Bug 30943: Adjust CreateEHoldingsFromBiblios -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #17 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 145958 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145958&action=edit Bug 30943: Fix test -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |32779 --- Comment #18 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32779 [Bug 32779] Import from list is broken -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|32781 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32781 [Bug 32781] CreateEHoldingsFromBiblios not dealing with non-existent package correcly -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|32779 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32779 [Bug 32779] Import from list is broken -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |32781 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32781 [Bug 32781] CreateEHoldingsFromBiblios not dealing with non-existent package correcly -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |32779 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32779 [Bug 32779] Import from list is broken -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Bug 30943 depends on bug 32779, which changed state. Bug 32779 Summary: Import from list is broken https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32779 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144340|0 |1 is obsolete| | --- Comment #19 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 147765 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147765&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145957|0 |1 is obsolete| | --- Comment #20 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 147766 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147766&action=edit Bug 30943: Adjust CreateEHoldingsFromBiblios -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145958|0 |1 is obsolete| | --- Comment #21 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Created attachment 147767 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147767&action=edit Bug 30943: Fix test -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Patch doesn't apply |Signed Off --- Comment #22 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Rebased, tests still pass. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #147765|0 |1 is obsolete| | Attachment #147766|0 |1 is obsolete| | Attachment #147767|0 |1 is obsolete| | --- Comment #23 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 148078 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148078&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #24 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 148079 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148079&action=edit Bug 30943: Adjust CreateEHoldingsFromBiblios Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 148080 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=148080&action=edit Bug 30943: Fix test Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 --- Comment #26 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.05. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30943 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED CC| |matt.blenkinsop@ptfs-europe | |.com --- Comment #27 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Enhancement - not backporting to 22.11.x Nice work everyone! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org