Doing some testing, Pseudonymization has an impact on the rate of checkouts. In large systems, with many attributes, we have seen significant impact. To mitigate this, we should enqueue pseudonymization as a background task
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.