Description
Nick Clemens (kidclamp)
2023-11-07 13:33:03 UTC
Created attachment 158596 [details]
Benchmarking script
I used this to test the performance with and without pseudonymization enabled
Created attachment 158597 [details] [review] Bug 35277: Pseudonymize in a background job This patch adds a new background job for pseduonymization To test: 1 - Download the benchmarking script 2 - Ensure your koha has bcrypt_settings in koha-conf.xml See bug 28911 3 - perl perf_check_pseudo.pl 4 - Note the slowdown after pseudonymization enabled 5 - Apply patches, restart all 6 - perl perf_check_pseudo.pl 7 - Note improvement 8 - Enable pseudonymization in sytem preferences 9 - Perform some checkouts and returns 10 - Verify the background jobs complete successfully 11 - Verify the pseudonymized_transactions table is updated correctly Created attachment 158606 [details] [review] Bug 35277: Pseudonymize in a background job This patch adds a new background job for pseduonymization To test: 1 - Download the benchmarking script 2 - Ensure your koha has bcrypt_settings in koha-conf.xml See bug 28911 3 - perl perf_check_pseudo.pl 4 - Note the slowdown after pseudonymization enabled 5 - Apply patches, restart all 6 - perl perf_check_pseudo.pl 7 - Note improvement 8 - Enable pseudonymization in sytem preferences 9 - Perform some checkouts and returns 10 - Verify the background jobs complete successfully 11 - Verify the pseudonymized_transactions table is updated correctly Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This makes a significant improvement for me, the stats are interesting. That tables all look correct to me after the patch and job has run too. Without the patch I go from 30/s to 8/s for the checkout_in benchmark between the syspref on and off.. with the patch I go from 30/s to 25/s.. that's a marked improvement and I feel is well worthwhile. Signing off Created attachment 158607 [details] [review] Bug 35277: Pseudonymize in a background job This patch adds a new background job for pseduonymization To test: 1 - Download the benchmarking script 2 - Ensure your koha has bcrypt_settings in koha-conf.xml See bug 28911 3 - perl perf_check_pseudo.pl 4 - Note the slowdown after pseudonymization enabled 5 - Apply patches, restart all 6 - perl perf_check_pseudo.pl 7 - Note improvement 8 - Enable pseudonymization in sytem preferences 9 - Perform some checkouts and returns 10 - Verify the background jobs complete successfully 11 - Verify the pseudonymized_transactions table is updated correctly Created attachment 158608 [details] [review] Bug 35277: Unit tests prove -v t/db_dependent/Koha/BackgroundJob/PseudonymizeStatistic.t Created attachment 158610 [details] [review] Bug 35277: Pseudonymize in a background job This patch adds a new background job for pseduonymization To test: 1 - Download the benchmarking script 2 - Ensure your koha has bcrypt_settings in koha-conf.xml See bug 28911 3 - perl perf_check_pseudo.pl 4 - Note the slowdown after pseudonymization enabled 5 - Apply patches, restart all 6 - perl perf_check_pseudo.pl 7 - Note improvement 8 - Enable pseudonymization in sytem preferences 9 - Perform some checkouts and returns 10 - Verify the background jobs complete successfully 11 - Verify the pseudonymized_transactions table is updated correctly Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 158611 [details] [review] Bug 35277: Unit tests prove -v t/db_dependent/Koha/BackgroundJob/PseudonymizeStatistic.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> No Assignee. FAIL t/db_dependent/Koha/BackgroundJob/PseudonymizeStatistic.t FAIL valid Can't locate Carp/Always.pm in @INC (you may need to install the Carp::Always module) (@INC contains: /usr/share/koha /usr/share/koha/lib /app/qatools /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.32.1 /usr/local/share/perl/5.32.1 /usr/lib/x86_64-linux-gnu/perl5/5.32 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.32 /usr/share/perl/5.32 /usr/local/lib/site_perl /var/lib/koha/myclone/plugins) Created attachment 158802 [details] [review] Bug 35277: (follow-up) Remove debugging module Partial QA: Code looks good to me. Though I don't know if there are pitfalls when moving a processing to a background job. That looks like a very simple and minimal case of background jobs usage. Sorry, string freeze. +use Modern::Perl; + +use Koha::PseudonymizedTransactions; + +use base 'Koha::BackgroundJob'; + +=head1 NAME + +Koha::BackgroundJob::BatchCancelHold - Batch cancel holds Copy and paste stuff? Please check the rest too. Created attachment 159440 [details] [review] Bug 35277: (QA follow-up) Fix POD Revisiting now Created attachment 160908 [details] [review] Bug 35277: Pseudonymize in a background job This patch adds a new background job for pseduonymization To test: 1 - Download the benchmarking script 2 - Ensure your koha has bcrypt_settings in koha-conf.xml See bug 28911 3 - perl perf_check_pseudo.pl 4 - Note the slowdown after pseudonymization enabled 5 - Apply patches, restart all 6 - perl perf_check_pseudo.pl 7 - Note improvement 8 - Enable pseudonymization in sytem preferences 9 - Perform some checkouts and returns 10 - Verify the background jobs complete successfully 11 - Verify the pseudonymized_transactions table is updated correctly Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 160909 [details] [review] Bug 35277: Unit tests prove -v t/db_dependent/Koha/BackgroundJob/PseudonymizeStatistic.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 160910 [details] [review] Bug 35277: (follow-up) Remove debugging module Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 160911 [details] [review] Bug 35277: (QA follow-up) Fix POD Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 160912 [details] [review] Bug 35277: (QA follow-up) POD corrections Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed for 24.05! Well done everyone, thank you! It broke Jenkins: koha_1 | # Failed test 'No tests run for subtest "Config does not exist"' koha_1 | # at t/db_dependent/Koha/Pseudonymization.t line 73. koha_1 | # Looks like you planned 12 tests but ran 1. koha_1 | koha_1 | # Failed test 'Koha::Anonymized::Transactions tests' koha_1 | # at t/db_dependent/Koha/Pseudonymization.t line 118. koha_1 | Can't call method "hashed_borrowernumber" on an undefined value at t/db_dependent/Koha/Pseudonymization.t line 104. Created attachment 161065 [details] [review] Bug 35277: (follow-up) Fix tests This patch adjusts the tests to mock the Background to ensure it is called and directly generate the pseudonymized transactions for testing Great work. But more enhancement no ? Since there is additional_work_needed I do not backport to 23.11.x for now. (In reply to Fridolin Somers from comment #25) > Great work. But more enhancement no ? > Since there is additional_work_needed I do not backport to 23.11.x for now. Good catch. The additional_work_needed was for the last patch - failing tests. I am removing it now. This was posted as a performance issue, not sure how severe. Nick? (In reply to Katrin Fischer from comment #26) > (In reply to Fridolin Somers from comment #25) > > Great work. But more enhancement no ? > > Since there is additional_work_needed I do not backport to 23.11.x for now. > > Good catch. > The additional_work_needed was for the last patch - failing tests. I am > removing it now. > > This was posted as a performance issue, not sure how severe. Nick? Depending on the setup it could be a severe issue, even in minimal setups the delay was noticeable. I would suggest for backport to 23.11 at least OK i give it a try Works fine in 23.11, its great :D Pushed to 23.11.x for 23.11.03 Ah one reflexion. Circulation operations are in a DB transaction. But when they generate a backgroundjob this action will be outside transaction right ? So if transaction fails the backgroundjob will still exist. I bet this is very rare we will do with it ;) (In reply to Fridolin Somers from comment #31) > Ah one reflexion. > Circulation operations are in a DB transaction. > But when they generate a backgroundjob this action will be outside > transaction right ? > So if transaction fails the backgroundjob will still exist. > I bet this is very rare we will do with it ;) See bug 35819. |