Bug 27421

Summary: Porting tools/stage-marc-import.pl to BackgroundJob
Product: Koha Reporter: Thomas Klausner <domm>
Component: MARC Bibliographic record staging/importAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: black23, caroline.cyr-la-rose, dcook, jonathan.druart, lucas, m.de.rooy, nick, tomascohen, victor
Version: unspecifiedKeywords: rel_22_11_candidate
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28128
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30739
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9354
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on: 22417, 30822    
Bug Blocks: 31580, 32780, 31711, 31891, 31992, 33576, 33755, 33865, 33972, 35156    
Attachments: Bug 27421: Use Background job for staging MARC records for import
Bug 27421: Commit and revert
Bug 27421: Enqueue only one job for indexing
Bug 27421: Use Background job for staging MARC records for import
Bug 27421: Commit and revert
Bug 27421: Use Background job for staging MARC records for import
Bug 27421: Commit and revert
Bug 27421: Use Background job for staging MARC records for import
Bug 27421: Commit and revert
Bug 27421: Use Background job for staging MARC records for import
Bug 27421: Commit and revert
Bug 27421: Fix job's size for import
Bug 27421: Add tests
Bug 27421: Use Background job for staging MARC records for import
Bug 27421: Commit and revert
Bug 27421: Fix job's size for import
Bug 27421: Add tests
Bug 27421: (follow-up) Add exec flag
Bug 27421: (QA follow-up) Include manage URL and item counts for import commit
Bug 27421: (QA follow-up) Similar changes for revert commit
Bug 27421: (QA follow-up) Work in progress: looking at progress/size
Bug 27421: (QA follow-up) WIP: Adjusting progress and size
Bug 27421: (QA follow-up) Sub finish should respect failed status too
Bug 27421: (QA follow-up) BatchCommitImportRecords needs param for skipping commits
Bug 27421: (QA follow-up) BatchCommitImportRecords needs param for skipping commits
Bug 27421: (QA follow-up) Polishing the backgroundjob modules
Bug 27421: Use Background job for staging MARC records for import
Bug 27421: Commit and revert
Bug 27421: Fix job's size for import
Bug 27421: Add tests
Bug 27421: (follow-up) Add exec flag
Bug 27421: (QA follow-up) Include manage URL and item counts for import commit
Bug 27421: (QA follow-up) Similar changes for revert commit
Bug 27421: (QA follow-up) Sub finish should respect failed status too
Bug 27421: (QA follow-up) BatchCommitImportRecords needs param for skipping commits
Bug 27421: (QA follow-up) Polishing the backgroundjob modules
Bug 27421: (QA follow-up) Template polishing
Bug 27421: (QA follow-up) Add redirection to job details
Bug 27421: MARC staging/import are long-running tasks

Description Thomas Klausner 2021-01-13 11:18:05 UTC
Hi!

I recently started to convert tools/stage-marc-import.pl to use the new BackgroundJobs, because we're using the Plack version, which does not like the old forking implementation. While I've realized now that we can still use the old CGI script by running it directly via the frontend Apache, I'm not very happy with this workaround.

So, here is my current first sort-of-working prototype:

https://github.com/Koha-Community/Koha/compare/master...domm:MARCImporter

Before I continue my work, I'd really appreciate some feedback if what I'm doing makes sense and matches your general plans.

Obviously there is still a lot missing:

* Proper feedback after starting the staging process (i.e. a link to the job)
* Probably I will also need to check if the job has finished, and present a link to the next step (copy the staged records into the library) (which AFAIK will also need to be converted to BackgroundJobs)

I also have a few questions:

* Is it neccessary to get the job-size before starting it? If yes, then I assume I'll have to first parse the MARC file to get the job size, throwing the results away (and this might not work, because we want to do the parsing in the background in the first place). Or is there an easy way to get the number of records?
* When I try to update job_size later, I get an exception (something like "job_size not tested")
* Can I actually use BatchStageMarcRecords in BackgroundJob?
* I see I can pass a batch size to BatchStageMarcRecords. But this will not result in more forking? This is "only" used to fire the progress callback?

I'm also not sure if the bug tracker or the dev mailing list is the best place to discuss my questions?


Anyway, looking forward to some feedback!

Greetings,
domm
Comment 1 Jonathan Druart 2021-01-13 16:01:10 UTC
Yes, that's the idea.


(In reply to Thomas Klausner from comment #0)
> * Is it neccessary to get the job-size before starting it? If yes, then I
> assume I'll have to first parse the MARC file to get the job size, throwing
> the results away (and this might not work, because we want to do the parsing
> in the background in the first place). Or is there an easy way to get the
> number of records?

I had the same problem with bug 27344. If it get too complicated (or cause perf issues) I'd say we can consider we are processing 1 file and so its size would be 1 :)
At least for a first step.

> * When I try to update job_size later, I get an exception (something like
> "job_size not tested")

The accessor is ->size

> * Can I actually use BatchStageMarcRecords in BackgroundJob?

Why not? Did you face a problem?

> * I see I can pass a batch size to BatchStageMarcRecords. But this will not
> result in more forking? This is "only" used to fire the progress callback?

I don't understand the question. Isn't BatchStageMarcRecords used to import the record? No fork will be done if you are using Koha::BackgroundJob.

> I'm also not sure if the bug tracker or the dev mailing list is the best
> place to discuss my questions?

Here it's perfect :)
Comment 2 David Cook 2021-01-14 00:59:32 UTC
> (In reply to Thomas Klausner from comment #0)
> > * Is it neccessary to get the job-size before starting it? If yes, then I
> > assume I'll have to first parse the MARC file to get the job size, throwing
> > the results away (and this might not work, because we want to do the parsing
> > in the background in the first place). Or is there an easy way to get the
> > number of records?
> 
> I had the same problem with bug 27344. If it get too complicated (or cause
> perf issues) I'd say we can consider we are processing 1 file and so its
> size would be 1 :)
> At least for a first step.
> 

I'd say use a job size of 1 since it's 1 file. As you say, Thomas, I think that it's unreasonable to parse the MARC file just to get the number of records. I think that "tools/stage-marc-import.pl" should just be responsible for uploading the file, enqueuing the background job, and then either showing/referring to a job management view to review the results.
Comment 3 Thomas Klausner 2021-01-14 13:50:18 UTC
(In reply to Jonathan Druart from comment #1)

> > * Can I actually use BatchStageMarcRecords in BackgroundJob?
> 
> Why not? Did you face a problem?

No, it's working. I just wasn't sure if this code might also need some work, because it's API feels a bit "old" (exported from C4::ImportBatch, long list of positional args..)

> > * I see I can pass a batch size to BatchStageMarcRecords. But this will not
> > result in more forking? This is "only" used to fire the progress callback?
> 
> I don't understand the question. Isn't BatchStageMarcRecords used to import
> the record? No fork will be done if you are using Koha::BackgroundJob.

It helps to actually read the code before asking questions. (Well, it also helps to read it *after* asking..).

I wasn't sure if maybe BatchStageMarcRecords would start a few seperate processes, passing each a number of elements. But $progress_interval is "just" used to count the progress and potentially fire the progress_callback. So all is good :-)

Thanks for the feedback, I will now continue here...
Comment 4 Jonathan Druart 2021-04-13 09:43:00 UTC
Hi Thomas, any news from this? Is it ready for testing?
Comment 5 Thomas Klausner 2021-04-13 10:18:45 UTC
Hi!

No, it's not ready for testing yet, mostly because I realized that I also need to port the second step of the import process; plus, I was distracted by other projects...

I'll try to finish this feature this month. Or do you have any sooner (release) deadlines?

Greetings,
domm
Comment 6 Jonathan Druart 2021-04-14 12:40:07 UTC
I don't think it could be ready (Passed QA) before the 21.05 feature freeze. But I would love to have it for the beginning of the next release if possible :)
Comment 7 Jonathan Druart 2022-06-03 09:39:33 UTC
Do you think you will have time to get back to this, Thomas?
Comment 8 David Cook 2022-06-06 00:01:03 UTC
Excited to hear more about this as well. I'm keen to see MARC imports happening as background jobs.
Comment 9 Thomas Klausner 2022-06-09 21:12:08 UTC
We ended up using the commandline tool (bulkmarkimport.pl) to migrate the data. So I stopped working on porting the web tool to background jobs.

I could give it another go, but I'll most likely will have to do that on my own time (which is fine by me regarding money etc, but it's the most limited time slot I have...)

So yes, I'd like to finish this feature, but I'm not sure I can prepare a patch that's nice enough for you and/or easy to test (mostly because I still did not get around to set up a proper Koha dev environment).

I also have some architectural concerns / ideas about how the background jobs are implemented (eg having to list all the background job classes in the source code seems a bit redundant), but I think it's better to discuss those in another issue (or just via mail) instead of bloating this issue.

Greetings,
domm
Comment 10 David Cook 2022-06-09 23:44:08 UTC
(In reply to Thomas Klausner from comment #9)
> I could give it another go, but I'll most likely will have to do that on my
> own time (which is fine by me regarding money etc, but it's the most limited
> time slot I have...)

I can relate to that on several projects :(
 
> So yes, I'd like to finish this feature, but I'm not sure I can prepare a
> patch that's nice enough for you and/or easy to test (mostly because I still
> did not get around to set up a proper Koha dev environment).

Also relatable.

Regarding a proper Koha dev environment, take a look at https://gitlab.com/koha-community/koha-testing-docker. I think a lot of people run it on Mac/Linux but I run it on Windows. So happy to answer any questions you might have about it. It makes Koha dev much easier. 

> I also have some architectural concerns / ideas about how the background
> jobs are implemented (eg having to list all the background job classes in
> the source code seems a bit redundant), but I think it's better to discuss
> those in another issue (or just via mail) instead of bloating this issue.
 
I have voiced the same concern but I think we might be the minority there. 

I think Bug 29149 re-organized the hard-coded list into a different hard-coded list in Koha::BackgroundJob.

However, Bug 27783 removed the hard-coded list from misc/background_jobs_worker.pl at least. 

So halfway there. I don't understand the resistance to not having a hard-coded allow list. I think its security value is very limited and just makes the system much less usable...
Comment 11 Jonathan Druart 2022-06-13 13:15:30 UTC
I am going to work on it this week.
Comment 12 Jonathan Druart 2022-06-16 08:29:37 UTC
Created attachment 136127 [details] [review]
Bug 27421: Use Background job for staging MARC records for import
Comment 13 Jonathan Druart 2022-06-16 08:29:44 UTC
Created attachment 136128 [details] [review]
Bug 27421: Commit and revert
Comment 14 Jonathan Druart 2022-06-16 08:29:48 UTC
Created attachment 136129 [details] [review]
Bug 27421: Enqueue only one job for indexing

Might need to be backported.
Comment 15 Jonathan Druart 2022-06-16 08:33:27 UTC
With those 3 patches I think most of the work is done.

To continue I need people to commit to an involvement on these patches.
To start I would like someone familiar with the import process to test the workflow and see if they are happy.

Then we will need to add test coverage and certainly provide some bug fixes.

Who's onboard?
Comment 16 Jonathan Druart 2022-06-16 10:47:37 UTC
(In reply to Jonathan Druart from comment #14)
> Created attachment 136129 [details] [review] [review]
> Bug 27421: Enqueue only one job for indexing
> 
> Might need to be backported.

This is actually bug 30822.
Comment 17 David Cook 2022-06-17 01:17:35 UTC
(In reply to Jonathan Druart from comment #15)
> With those 3 patches I think most of the work is done.
> 
> To continue I need people to commit to an involvement on these patches.
> To start I would like someone familiar with the import process to test the
> workflow and see if they are happy.
> 
> Then we will need to add test coverage and certainly provide some bug fixes.
> 
> Who's onboard?

I'm keen.
Comment 18 David Cook 2022-06-17 05:22:18 UTC
(In reply to David Cook from comment #17)
> (In reply to Jonathan Druart from comment #15)
> > Who's onboard?
> 
> I'm keen.

It looks like bug 30822 doesn't apply to master anymore. 

But 27421 cleanly applies without it.
Comment 19 David Cook 2022-06-17 05:23:26 UTC
My test plan:
0. Apply patches
1. Comment out the following lines in /etc/koha/apache-shared-intranet-plack.conf:
ProxyPass "/cgi-bin/koha/tools/manage-marc-import.pl" "!"
ProxyPass "/cgi-bin/koha/tools/stage-marc-import.pl" "!"
2. service apache2 reload
3. koha-plack --restart kohadev
4. Go to http://localhost:8081/cgi-bin/koha/catalogue/export.pl?format=marcxml&op=export&bib=29
5. Go to http://localhost:8081/cgi-bin/koha/tools/stage-marc-import.pl
6. Upload bib-29.marcxml file
7. Change "How to process items" to "Ignore items"
8. Click "Stage for import"
9. Click "View detail of the enqueued job"
10. Note that a job has been created in Koha at http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl?op=view&id=1

However, the job isn't getting processed. Note the following in /var/log/koha/kohadev/worker-output.log:

Exception 'Koha::Exception' thrown 'stage_marc_for_import is not a valid job_type'
Comment 20 David Cook 2022-06-17 05:28:48 UTC
(In reply to David Cook from comment #19)
> However, the job isn't getting processed. Note the following in
> /var/log/koha/kohadev/worker-output.log:
> 
> Exception 'Koha::Exception' thrown 'stage_marc_for_import is not a valid
> job_type'

I imagine that's because I need to restart the workers actually... I'll change my test plan...
Comment 21 David Cook 2022-06-17 05:36:21 UTC
Created attachment 136216 [details] [review]
Bug 27421: Use Background job for staging MARC records for import

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 22 David Cook 2022-06-17 05:36:25 UTC
Created attachment 136217 [details] [review]
Bug 27421: Commit and revert

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 23 David Cook 2022-06-17 05:36:41 UTC
My test plan:
0. Apply patches
1. Comment out the following lines in /etc/koha/apache-shared-intranet-plack.conf:
ProxyPass "/cgi-bin/koha/tools/manage-marc-import.pl" "!"
ProxyPass "/cgi-bin/koha/tools/stage-marc-import.pl" "!"
2. service apache2 reload
3. service koha-common restart
#NOTE: We need to restart starman/Plack and the koha workers

4. Go to http://localhost:8081/cgi-bin/koha/catalogue/export.pl?format=marcxml&op=export&bib=29
5. Go to http://localhost:8081/cgi-bin/koha/tools/stage-marc-import.pl
6. Upload bib-29.marcxml file
7. Change "How to process items" to "Ignore items"
8. Click "Stage for import"
9. Click "View detail of the enqueued job"
10. Click "View batch" (or go to "Staged MARC management" and find the batch you were interested in)

11. Change "Matching rule applied" to "KohaBiblio (999$c)"
12. Change "Action if matching record found" to "Replace existing record with incoming record"
13. Change "Item processing" to "Ignore items"[1]
14. Click "Apply different matching rules"
15. Click "Import this batch into the catalog"
16. Click "View detail of the enqueued job"[2]
17. Note "Number of records updated" is "1"

[1] not sure if this is a bug in these patches or if this is pre-existing. It sounds pre-existing iirc...
[2] The text in this window isn't very user friendly. I think we could use more job-specific text.
Comment 24 David Cook 2022-06-17 05:38:43 UTC
(In reply to Jonathan Druart from comment #15)
> To continue I need people to commit to an involvement on these patches.
> To start I would like someone familiar with the import process to test the
> workflow and see if they are happy.

I'm happy enough from a user perspective I think.

It could probably use a bit more polishing but it looks like the guts are there.
Comment 25 David Cook 2022-06-17 05:42:28 UTC
From a dev perspective, I've been wanting to do clean asynchronous MARC importing as part of OAI-PMH harvesting, but I don't think I'd be able to use these background jobs. That's OK though. I think those will need to be customized anyway, and any reusable code will need to be refactored later...

I wondered if ./misc/migration_tools/bulkmarcimport.pl would be able to use this, but unfortunately that script seems to have its own approach entirely to importing MARC records. 

So technically... I think this is good enough too. 

I reckon let's get this into the codebase, and optimizations can happen later. It gets the job done! 

--

Note that I only tested with 1 MARC record. QA will probably want to test using a larger number of MARC records.
Comment 26 Marcel de Rooy 2022-06-17 09:28:42 UTC
Oops
Comment 27 Marcel de Rooy 2022-06-17 09:31:13 UTC
Still applies. Might have a look rightaway..
Comment 28 Marcel de Rooy 2022-06-17 09:34:20 UTC
(In reply to David Cook from comment #25)

> I reckon let's get this into the codebase, and optimizations can happen
> later. It gets the job done! 

Without referring to this code specifically, but we did that before and what happened?
Comment 29 Marcel de Rooy 2022-06-17 09:37:48 UTC
(In reply to David Cook from comment #25)

> Note that I only tested with 1 MARC record. QA will probably want to test
> using a larger number of MARC records.

Hmm. Lets defer testing to the QA phase? :) I would say: back to NSO, and add another signoff?
Comment 30 Marcel de Rooy 2022-06-17 10:02:43 UTC
(In reply to Jonathan Druart from comment #15)
> With those 3 patches I think most of the work is done.
> 
> To continue I need people to commit to an involvement on these patches.
> To start I would like someone familiar with the import process to test the
> workflow and see if they are happy.
> 
> Then we will need to add test coverage and certainly provide some bug fixes.
> 
> Who's onboard?

Well, I was. No problem. Seeing no test plan from the author and no unit tests. But a signed off status :)
Did you find some experienced user to get user feedback btw ?
Comment 31 Nick Clemens 2022-06-17 12:42:41 UTC
I tried testing, if I stage with matching it seems to die every time - it is also a bad experience when staging fails - the job remains at 'Started' and the report is present but empty

When I import:
 - The job says 'Started' but also shows 'Completed import of records'
 - When it finishes, there is no link back to the batch
 - If it fails it says 'Finished' "Progress 0/0" 
 - From a completed import I can't get back to the background job (maybe not needed?)

Lack of error feedback was problematic before, and is worse here because the user is moved to a different section of Koha and the progress is less visible - before you had a stuck bar, now just a not updating screen

I like this alot but do think we need to fix these other problems while we do this work - it can be a second bug while we address functionality here, but I would like to see them pushed together in that case
Comment 32 Jonathan Druart 2022-06-20 07:43:19 UTC
Created attachment 136338 [details] [review]
Bug 27421: Use Background job for staging MARC records for import
Comment 33 Jonathan Druart 2022-06-20 07:43:25 UTC
Created attachment 136339 [details] [review]
Bug 27421: Commit and revert
Comment 34 Jonathan Druart 2022-06-20 07:45:43 UTC
(In reply to Marcel de Rooy from comment #30)
> (In reply to Jonathan Druart from comment #15)
> > With those 3 patches I think most of the work is done.
> > 
> > To continue I need people to commit to an involvement on these patches.
> > To start I would like someone familiar with the import process to test the
> > workflow and see if they are happy.
> > 
> > Then we will need to add test coverage and certainly provide some bug fixes.
> > 
> > Who's onboard?
> 
> Well, I was. No problem. Seeing no test plan from the author and no unit
> tests. But a signed off status :)
> Did you find some experienced user to get user feedback btw ?

The status should stay 'in discussion', the patches are not ready for master. I will need help for that.

(In reply to Nick Clemens from comment #31)

Thanks, Nick!

> I tried testing, if I stage with matching it seems to die every time - it is
> also a bad experience when staging fails - the job remains at 'Started' and
> the report is present but empty

Fixed.

> When I import:
>  - The job says 'Started' but also shows 'Completed import of records'

Should be fixed now.

>  - When it finishes, there is no link back to the batch

Yes, that's one problem I identified. However we don't have the batch id yet. One solution would be to have an ajax script that would get the job's detail and display the link when the job has started. But I would suggest to do it on a separate bug report, if we don't have a better solution. Note that bug 30982 is adding the REST API route for GET /background_jobs and GET /background_jobs/$id

>  - If it fails it says 'Finished' "Progress 0/0"

Progress bugs should be fixed now.

>  - From a completed import I can't get back to the background job (maybe not
> needed?)

Hum, I don't know. If we need it it will be "tricky" (we will need a separate DB column to store that I think).

> Lack of error feedback was problematic before, and is worse here because the
> user is moved to a different section of Koha and the progress is less
> visible - before you had a stuck bar, now just a not updating screen

Yes, that's why I think we need feedback from regular users. The ajax suggestion above could work but I will be happy if we can come up with a better solution.

> I like this alot but do think we need to fix these other problems while we
> do this work - it can be a second bug while we address functionality here,
> but I would like to see them pushed together in that case
Comment 35 David Cook 2022-06-27 00:52:29 UTC
(In reply to Jonathan Druart from comment #34)
> Yes, that's why I think we need feedback from regular users. 

Let's ask the folk at bug 28128 and bug 31043 since they're having issues with the current MARC import. They can say whether or not this improves things surely.
Comment 36 Victor Grousset/tuxayo 2022-08-05 02:36:04 UTC
Conflicts, if you don't have time, I can give a try before Monday.
Comment 37 Marcel de Rooy 2022-08-05 07:06:34 UTC
I fixed the first conflict
Looking at this one:
Applying: Bug 27421: Use Background job for staging MARC records for import
Applying: Bug 27421: Commit and revert
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/stage_marc_for_import.inc).
Comment 38 Marcel de Rooy 2022-08-05 07:15:57 UTC
Created attachment 138642 [details] [review]
Bug 27421: Use Background job for staging MARC records for import
Comment 39 Marcel de Rooy 2022-08-05 07:16:01 UTC
Created attachment 138643 [details] [review]
Bug 27421: Commit and revert
Comment 40 Marcel de Rooy 2022-08-05 07:17:57 UTC
Manual patching the diffs:

patching file Koha/BackgroundJob.pm
patching file Koha/BackgroundJob/MARCImportCommitBatch.pm
patching file Koha/BackgroundJob/MARCImportRevertBatch.pm
patching file debian/templates/apache-shared-intranet-plack.conf
patching file koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/marc_import_commit_batch.inc
patching file koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/marc_import_revert_batch.inc
patching file koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/stage_marc_for_import.inc
patching file koha-tmpl/intranet-tmpl/prog/en/modules/admin/background_jobs.tt
patching file koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt
patching file tools/manage-marc-import.pl
Hunk #6 NOT MERGED at 275-283.

I do not trust the last script. Somehow we ended up touching a considerable lower number of lines.. Will have a look
Comment 41 Marcel de Rooy 2022-08-05 07:23:53 UTC
The diff in the original 35K patch gives me on tools/manage-marc-import.pl:

 1 file changed, 47 insertions(+), 22 deletions(-)

But there are definitely 210 changes in that patch file. (found with sed)
Comment 42 Jonathan Druart 2022-08-05 07:40:22 UTC
Created attachment 138656 [details] [review]
Bug 27421: Use Background job for staging MARC records for import
Comment 43 Jonathan Druart 2022-08-05 07:40:27 UTC
Created attachment 138657 [details] [review]
Bug 27421: Commit and revert
Comment 44 Marcel de Rooy 2022-08-05 09:16:35 UTC
Great it applies now.
New module added.
What I am missing here completely, is tests !
Comment 45 Jonathan Druart 2022-08-05 09:20:22 UTC
(In reply to Marcel de Rooy from comment #44)
> Great it applies now.
> New module added.
> What I am missing here completely, is tests !

How could this be tested easily?
Comment 46 Jonathan Druart 2022-08-05 09:25:16 UTC
(In reply to Jonathan Druart from comment #45)
> (In reply to Marcel de Rooy from comment #44)
> > Great it applies now.
> > New module added.
> > What I am missing here completely, is tests !
> 
> How could this be tested easily?

I mean, I am not going to write UI tests, neither a full coverage of the import process. And enqueuing jobs is already tested by t/db_dependent/BackgroundJob.t

I could make you happy by making `git grep MARCImportCommitBatch t` returns a couple of occurrences, but I don't know how to have a test that would be useful and that won't require too much time to write.
Comment 47 Marcel de Rooy 2022-08-05 09:40:59 UTC
|  9 | finished |        1 |    0 |             51 | marc_import_commit_batch

The job size for this job does not get updated?
Comment 48 Jonathan Druart 2022-08-05 09:50:16 UTC
Created attachment 138660 [details] [review]
Bug 27421: Fix job's size for import
Comment 49 Jonathan Druart 2022-08-09 07:43:37 UTC
Created attachment 138881 [details] [review]
Bug 27421: Add tests
Comment 50 Jonathan Druart 2022-08-09 07:44:59 UTC
This patch proves the evidence of bad faith, the tests were pretty trivial to write!
Comment 51 Marcel de Rooy 2022-08-18 09:32:20 UTC
Please test this one today! QA scheduled for tomorrow :)
Comment 52 Nick Clemens 2022-08-18 13:55:41 UTC
Stage:
 - No report number of items found in batch when complete
 - Staging shows "results" info before completion
 - in my testing I never saw progress update from "Started" 0/0 until the job finished
 - On finish: Progress 600532/7655?

Import:
 - Job details always displays: Completed import of records
 - An import that dies never resolves: Started/Progess 50/437
 - Table of results always displays with no info/empty columns
 - Table is populated when job finished, but is empty on refresh or browse to page

Both:
 - Detailed messages never contains anything
 - No error feedback
 - I feel we should directly send the user to the job details:
   the intermediary screen provides no info and adds a click:
   The job has been enqueued! It will be processed as soon as possible.
   View detail of the enqueued job 


This is so much faster though, and when it works, it works as it did previously

I am willing to add provisional sign off to allow QA, but this will need polishing if included in release
Comment 53 Nick Clemens 2022-08-18 14:04:01 UTC
Created attachment 139384 [details] [review]
Bug 27421: Use Background job for staging MARC records for import

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Nick Clemens 2022-08-18 14:04:09 UTC
Created attachment 139385 [details] [review]
Bug 27421: Commit and revert

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Nick Clemens 2022-08-18 14:04:14 UTC
Created attachment 139386 [details] [review]
Bug 27421: Fix job's size for import

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens 2022-08-18 14:04:19 UTC
Created attachment 139387 [details] [review]
Bug 27421: Add tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens 2022-08-18 14:04:23 UTC
Created attachment 139388 [details] [review]
Bug 27421: (follow-up) Add exec flag

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Marcel de Rooy 2022-08-19 06:16:43 UTC
(In reply to Jonathan Druart from comment #50)
> This patch proves the evidence of bad faith, the tests were pretty trivial
> to write!

And we can use this comment now to insist that you should write tests whatever claims you make :)
Comment 59 Marcel de Rooy 2022-08-19 06:17:19 UTC
QAing
Comment 60 Marcel de Rooy 2022-08-19 08:00:10 UTC
Created attachment 139448 [details] [review]
Bug 27421: (QA follow-up) Include manage URL and item counts for import commit

Test plan:
Run an stage and import.
Check import commit job.
Click to managed batch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 61 Marcel de Rooy 2022-08-19 08:00:15 UTC
Created attachment 139449 [details] [review]
Bug 27421: (QA follow-up) Similar changes for revert commit

Test plan:
Look at job results when reverting imported batch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Marcel de Rooy 2022-08-19 08:00:20 UTC
Created attachment 139450 [details] [review]
Bug 27421: (QA follow-up) Work in progress: looking at progress/size
Comment 63 Marcel de Rooy 2022-08-19 08:01:31 UTC
QA Status:

Thx Nick for your comments. I copied them and added my own.
I agree that the status of this patch set is not production ready yet. Since Jonathan cannot finish it, I am willing to coordinate getting this further. But please communicate and help !
I think we should get this in 21.11 and proceed to the other scripts that still need this refactoring to get rid of the ProxyPass stuff.

Stage:
 - No report number of items found in batch when complete
=> ADDRESSED for Stage and Revert
 - Staging shows "results" info before completion
=> STILL LOOKING
 - in my testing I never saw progress update from "Started" 0/0 until the job finished
=> STILL LOOKING
 - On finish: Progress 600532/7655?
=> STILL LOOKING

Import:
 - Job details always displays: Completed import of records
 - An import that dies never resolves: Started/Progess 50/437
 - Table of results always displays with no info/empty columns
 - Table is populated when job finished, but is empty on refresh or browse to page

Revert:
- Finished revert job shows 0 / 0
=> PARTIALLY ADDRESSED

All:
 - Detailed messages never contains anything
=> Room for future extension. No problem for me.
 - No error feedback
=> Wasnt this poor already ?
 - I feel we should directly send the user to the job details:
=> AGREED. No blocker for me now, but we should either redirect within a few seconds or do it rightaway with a enqueued alert or so.
Comment 64 Marcel de Rooy 2022-08-19 08:02:20 UTC
(In reply to Marcel de Rooy from comment #63)
> I think we should get this in 21.11 and proceed to the other scripts that
> still need this refactoring to get rid of the ProxyPass stuff.

Haha 22.11
Comment 65 Victor Grousset/tuxayo 2022-08-20 20:40:34 UTC
(In reply to Nick Clemens from comment #52)
> [...]
> 
> This is so much faster though, and when it works, it works as it did
> previously

Out of curiosity about how BackgroundJob works, how does it makes the process faster?
Comment 66 Marcel de Rooy 2022-08-22 06:39:45 UTC
(In reply to Victor Grousset/tuxayo from comment #65)
> (In reply to Nick Clemens from comment #52)
> > [...]
> > 
> > This is so much faster though, and when it works, it works as it did
> > previously
> 
> Out of curiosity about how BackgroundJob works, how does it makes the
> process faster?

Just an educated guess: using Plack now ?
Comment 67 Marcel de Rooy 2022-08-22 06:40:22 UTC
(In reply to Marcel de Rooy from comment #66)
> (In reply to Victor Grousset/tuxayo from comment #65)
> > (In reply to Nick Clemens from comment #52)
> > > [...]
> > > 
> > > This is so much faster though, and when it works, it works as it did
> > > previously
> > 
> > Out of curiosity about how BackgroundJob works, how does it makes the
> > process faster?
> 
> Just an educated guess: using Plack now ?

And worker still in memory?
Comment 68 David Cook 2022-08-22 06:45:46 UTC
I suppose the lack of polling would also make it a faster experience overall.
Comment 69 Nick Clemens 2022-08-22 10:23:52 UTC
(In reply to Marcel de Rooy from comment #66)
> (In reply to Victor Grousset/tuxayo from comment #65)
> > (In reply to Nick Clemens from comment #52)
> > > [...]
> > > 
> > > This is so much faster though, and when it works, it works as it did
> > > previously
> > 
> > Out of curiosity about how BackgroundJob works, how does it makes the
> > process faster?
> 
> Just an educated guess: using Plack now ?

+1, that's what i guessed made it better
Comment 70 Victor Grousset/tuxayo 2022-08-22 21:13:45 UTC
Thanks all ^^
Oh right, that was one of the few (the only?) things not using Plack. Surprising that this has a big effect on tasks that are long where the time loading Perl and the modules should be a small fraction.
Comment 71 Marcel de Rooy 2022-08-25 14:38:12 UTC
Created attachment 139797 [details] [review]
Bug 27421: (QA follow-up) WIP: Adjusting progress and size

Might still need a bit of polishing.
Comment 72 Marcel de Rooy 2022-09-02 09:54:45 UTC
Will try to get this further Monday.
Comment 73 Marcel de Rooy 2022-09-05 10:31:58 UTC
Hmm. Transaction stuff too in MARCImportCommitBatch..
There is no rollback included as if you would have called txn_do !
Comment 74 Marcel de Rooy 2022-09-05 11:49:25 UTC
(In reply to Marcel de Rooy from comment #73)
> Hmm. Transaction stuff too in MARCImportCommitBatch..
> There is no rollback included as if you would have called txn_do !

I am wondering if we should still commit after 50 records or so. If the job fails, I do not expect it to commit any records at all now. Especially since it runs in a background worker now.
My idea now: parameter that controls intermediate commits !

Any thoughts?
Comment 75 Marcel de Rooy 2022-09-05 12:13:53 UTC
Created attachment 140205 [details] [review]
Bug 27421: (QA follow-up) Sub finish should respect failed status too

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 76 Marcel de Rooy 2022-09-05 12:13:58 UTC
Created attachment 140206 [details] [review]
Bug 27421: (QA follow-up) BatchCommitImportRecords needs param for skipping commits

When you submit a background jobs, and it fails, you do not expect
partial results in the database.
Note that when the Background feature would support a partially
completed status, things might change again.

Test plan:
Run t/db_dependent/Koha/BackgroundJobs/StageMARCForImport.t
Note: This serves to verify that it still runs as expected.
We will still test the new parameter further on.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 77 Marcel de Rooy 2022-09-05 12:20:18 UTC
Created attachment 140207 [details] [review]
Bug 27421: (QA follow-up) BatchCommitImportRecords needs param for skipping commits

When you submit a background jobs, and it fails, you do not expect
partial results in the database.
Note that when the Background feature would support a partially
completed status, things might change again.

Note that the >0 test was superfluous if you check for ^\d+$.

Test plan:
Run t/db_dependent/Koha/BackgroundJobs/StageMARCForImport.t
Note: This serves to verify that it still runs as expected.
We will still test the new parameter further on.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 78 Marcel de Rooy 2022-09-05 13:13:31 UTC
Created attachment 140208 [details] [review]
Bug 27421: (QA follow-up) Polishing the backgroundjob modules

StageMARCForImport:
- Rollback in catch
- Setting progress, size or status after BatchStageMarcRecords
  in both try and catch block

ImportCommitBatch:
- Move setting size back to enqueue moment
- Rollback in catch
- Setting progress, size or status after BatchStageMarcRecords
  in both try and catch block

ImportRevertBatch:
- Move setting size back to enqueue moment
- Adding transaction/rollback to module since import routine
  does not support it. Could be moved later.
- Setting progress, size or status after BatchStageMarcRecords
  in both try and catch block

Test plan:
Run t/db_dependent/Koha/BackgroundJobs/StageMARCForImport.t
Test staging file
Bonus: Put a die statement in BatchStageMarcRecords.
Test importing batch
Bonus: Include some records with an invalid library code; this will
trigger an FK exception.
Test reverting batch.
Bonus: Put a die in BatchRevertRecords.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 79 Marcel de Rooy 2022-09-05 13:19:58 UTC
Will hopefully finish up tomorrow.
Nick, could you have another look later this week?
Comment 80 Marcel de Rooy 2022-09-06 12:48:20 UTC
Created attachment 140223 [details] [review]
Bug 27421: Use Background job for staging MARC records for import

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 81 Marcel de Rooy 2022-09-06 12:48:25 UTC
Created attachment 140224 [details] [review]
Bug 27421: Commit and revert

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 82 Marcel de Rooy 2022-09-06 12:48:30 UTC
Created attachment 140225 [details] [review]
Bug 27421: Fix job's size for import

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 83 Marcel de Rooy 2022-09-06 12:48:34 UTC
Created attachment 140226 [details] [review]
Bug 27421: Add tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 84 Marcel de Rooy 2022-09-06 12:48:38 UTC
Created attachment 140227 [details] [review]
Bug 27421: (follow-up) Add exec flag

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 85 Marcel de Rooy 2022-09-06 12:48:43 UTC
Created attachment 140228 [details] [review]
Bug 27421: (QA follow-up) Include manage URL and item counts for import commit

Test plan:
Run an stage and import.
Check import commit job.
Click to managed batch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 86 Marcel de Rooy 2022-09-06 12:48:48 UTC
Created attachment 140229 [details] [review]
Bug 27421: (QA follow-up) Similar changes for revert commit

Test plan:
Look at job results when reverting imported batch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 87 Marcel de Rooy 2022-09-06 12:48:53 UTC
Created attachment 140230 [details] [review]
Bug 27421: (QA follow-up) Sub finish should respect failed status too

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 88 Marcel de Rooy 2022-09-06 12:48:57 UTC
Created attachment 140231 [details] [review]
Bug 27421: (QA follow-up) BatchCommitImportRecords needs param for skipping commits

When you submit a background jobs, and it fails, you do not expect
partial results in the database.
Note that when the Background feature would support a partially
completed status, things might change again.

Note that the >0 test was superfluous if you check for ^\d+$.

Test plan:
Run t/db_dependent/Koha/BackgroundJobs/StageMARCForImport.t

Note: This serves to verify that it still runs as expected.
The test plan of the following patch covers the new param.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 89 Marcel de Rooy 2022-09-06 12:49:02 UTC
Created attachment 140232 [details] [review]
Bug 27421: (QA follow-up) Polishing the backgroundjob modules

StageMARCForImport:
- Rollback in catch
- Setting progress, size or status after BatchStageMarcRecords
  in both try and catch block

ImportCommitBatch:
- Move setting size back to enqueue moment
- Rollback in catch
- Setting progress, size or status after BatchStageMarcRecords
  in both try and catch block

ImportRevertBatch:
- Move setting size back to enqueue moment
- Adding transaction/rollback to module since import routine
  does not support it. Could be moved later.
- Setting progress, size or status after BatchStageMarcRecords
  in both try and catch block

Test plan:
Run t/db_dependent/Koha/BackgroundJobs/StageMARCForImport.t
Test staging file
Bonus: Put a die statement in BatchStageMarcRecords.

Test importing batch
Bonus: Include some records with an invalid library code; this will
trigger an FK exception. (Reduce the progress from 50 to 1. If your
first record would be fine, check if it is NOT imported when the job
fails.)

Test reverting batch.
Bonus: Put a die in BatchRevertRecords.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 90 Marcel de Rooy 2022-09-06 12:49:07 UTC
Created attachment 140233 [details] [review]
Bug 27421: (QA follow-up) Template polishing

The completed alert needs a condition on status.
The record_type variable needs a bit of 'context'.

Test plan:
Verify if a failed or new job does not have a Completed alert.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 91 Marcel de Rooy 2022-09-06 12:49:12 UTC
Created attachment 140234 [details] [review]
Bug 27421: (QA follow-up) Add redirection to job details

Low budget implementation. After 5 seconds we jump to jobs.
Small jobs should already be finished.

Test plan:
Try staging a file. Wait and see if you got redirected.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 92 Marcel de Rooy 2022-09-06 12:51:03 UTC
My latest responses to Nick's findings:

Stage:
 - No report number of items found in batch when complete
=> ADDRESSED for Stage and Revert
 - Staging shows "results" info before completion
=> ADDRESSED (at least partially)
 - in my testing I never saw progress update from "Started" 0/0 until the job finished
=> OK: TESTED: +          = BatchCommitRecords( $import_batch_id, $frameworkcode, 1, +            sub { my $job_progress = shift; $self->progress( $job_progress )->store; sleep 10; } );
 - On finish: Progress 600532/7655?
=> ADJUSTED

Import:
 - Job details always displays: Completed import of records
=> ADDRESSED
 - An import that dies never resolves: Started/Progess 50/437
=> ADDRESSED
 - Table of results always displays with no info/empty columns
=> ADDRESSED
 - Table is populated when job finished, but is empty on refresh or browse to page
=> PLEASE TEST AGAIN

Revert:
- Finished revert job shows 0 / 0
=> ADDRESSED

All:
 - Detailed messages never contains anything
=> OK Room for future extension? No problem for me.
 - No error feedback
=> OK Wasnt this poor already ?
 - I feel we should directly send the user to the job details:
=> ADDRESSED
Comment 93 Marcel de Rooy 2022-09-06 12:53:33 UTC
If a patch set needs 8 follow-ups, if it safe to say that it seemed not ready yet as Nick mentioned earlier too ;) Joubu had a good excuse this time.

If Nick is available, he is very welcome to have another look. Otherwise Tomas will certainly do.

Passing QA
Comment 94 Marcel de Rooy 2022-09-07 12:49:15 UTC
We should probably put this one on top of 31351. Please wait
Comment 95 Marcel de Rooy 2022-09-07 14:47:56 UTC
(In reply to Marcel de Rooy from comment #94)
> We should probably put this one on top of 31351. Please wait

Actually we should be fine already. Applies on top.
Comment 96 Tomás Cohen Arazi 2022-09-19 18:17:37 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 97 Tomás Cohen Arazi 2022-09-19 18:40:43 UTC
Created attachment 140759 [details] [review]
Bug 27421: MARC staging/import are long-running tasks

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 98 Tomás Cohen Arazi 2022-09-19 19:01:21 UTC
Ok, I pushed it to give this a chance (maybe people worry about this change and start giving opinions and patches).

Please focus on bug 30982 (which I'd push right away too) so we can come up with a component for displaying progress (once and for all).

I agree item information being missing on the run report is bad.

Also, I agree we should add a job_id column to the import_batches table so we can link back when the task is complete.
Comment 99 Marcel de Rooy 2022-09-20 11:10:06 UTC
(In reply to Tomás Cohen Arazi from comment #98)
> Ok, I pushed it to give this a chance (maybe people worry about this change
> and start giving opinions and patches).
> 
> Please focus on bug 30982 (which I'd push right away too) so we can come up
> with a component for displaying progress (once and for all).
> 
> I agree item information being missing on the run report is bad.
> 
> Also, I agree we should add a job_id column to the import_batches table so
> we can link back when the task is complete.

Yes, we live in brave RM times.
Comment 100 Lucas Gass 2022-10-31 22:21:35 UTC
This is cool, but no backport for the 22.05.x series
Comment 101 Caroline Cyr La Rose 2023-05-05 19:36:38 UTC
Already in the manual.