|
Description
Thomas Klausner
2021-01-13 11:18:05 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 :) > (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.
(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... Hi Thomas, any news from this? Is it ready for testing? 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 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 :) Do you think you will have time to get back to this, Thomas? Excited to hear more about this as well. I'm keen to see MARC imports happening as background jobs. 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 (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... I am going to work on it this week. Created attachment 136127 [details] [review] Bug 27421: Use Background job for staging MARC records for import Created attachment 136128 [details] [review] Bug 27421: Commit and revert Created attachment 136129 [details] [review] Bug 27421: Enqueue only one job for indexing Might need to be backported. 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? (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. (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. (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. 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' (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... 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> Created attachment 136217 [details] [review] Bug 27421: Commit and revert Signed-off-by: David Cook <dcook@prosentient.com.au> 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. (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. 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. Oops Still applies. Might have a look rightaway.. (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? (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? (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 ? 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 Created attachment 136338 [details] [review] Bug 27421: Use Background job for staging MARC records for import Created attachment 136339 [details] [review] Bug 27421: Commit and revert (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 (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. Conflicts, if you don't have time, I can give a try before Monday. 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). Created attachment 138642 [details] [review] Bug 27421: Use Background job for staging MARC records for import Created attachment 138643 [details] [review] Bug 27421: Commit and revert 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 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) Created attachment 138656 [details] [review] Bug 27421: Use Background job for staging MARC records for import Created attachment 138657 [details] [review] Bug 27421: Commit and revert Great it applies now. New module added. What I am missing here completely, is tests ! (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? (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. | 9 | finished | 1 | 0 | 51 | marc_import_commit_batch The job size for this job does not get updated? Created attachment 138660 [details] [review] Bug 27421: Fix job's size for import Created attachment 138881 [details] [review] Bug 27421: Add tests This patch proves the evidence of bad faith, the tests were pretty trivial to write! Please test this one today! QA scheduled for tomorrow :) 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 Created attachment 139384 [details] [review] Bug 27421: Use Background job for staging MARC records for import Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 139385 [details] [review] Bug 27421: Commit and revert Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 139386 [details] [review] Bug 27421: Fix job's size for import Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 139387 [details] [review] Bug 27421: Add tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 139388 [details] [review] Bug 27421: (follow-up) Add exec flag Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (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 :) QAing 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> 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> Created attachment 139450 [details] [review] Bug 27421: (QA follow-up) Work in progress: looking at progress/size 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. (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 (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? (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 ? (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? I suppose the lack of polling would also make it a faster experience overall. (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 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. Created attachment 139797 [details] [review] Bug 27421: (QA follow-up) WIP: Adjusting progress and size Might still need a bit of polishing. Will try to get this further Monday. Hmm. Transaction stuff too in MARCImportCommitBatch.. There is no rollback included as if you would have called txn_do ! (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? 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> 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> 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> 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> Will hopefully finish up tomorrow. Nick, could you have another look later this week? 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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
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 We should probably put this one on top of 31351. Please wait (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. Pushed to master for 22.11. Nice work everyone, thanks! Created attachment 140759 [details] [review] Bug 27421: MARC staging/import are long-running tasks Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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. (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. This is cool, but no backport for the 22.05.x series Already in the manual. I'm curious if the code for failed imports was tested, as I'm not sure that I've seen it work... see bug 37147 |