|
Description
Katrin Fischer
2024-03-07 15:56:47 UTC
Passes locally for me. We have the same in the latest run #2808: koha_1 | # Inactivity timeout koha_1 | koha_1 | # Failed test 'POST //RukUTMflvmiFmWiGbiMJbpcClr5uXXjA4MqgN_LRiINumkvxboZcrYkP:thePassword123@/api/v1/transfer_limits/batch' koha_1 | # at t/db_dependent/api/v1/transfer_limits.t line 210. koha_1 | koha_1 | # Failed test 'SWAGGER3.2.1' koha_1 | # at t/db_dependent/api/v1/transfer_limits.t line 210. koha_1 | # got: undef koha_1 | # expected: '201' koha_1 | # Looks like you failed 2 tests of 26. koha_1 | koha_1 | # Failed test 'batch_add() and batch_delete() tests' Koha_Master_MDB_Latest/1359 apt install stress
stress --cpu 16 (or the number of CPUs you have)
Run the following script to inject a lot of libraries
use t::lib::TestBuilder;
my $builder = t::lib::TestBuilder->new;
for my $i ( 1..100000 ) {
say $i;
$builder->build_object({class => 'Koha::Libraries'});
}
% time prove t/db_dependent/api/v1/transfer_limits.t
t/db_dependent/api/v1/transfer_limits.t .. 3/4 # Inactivity timeout
# Failed test 'POST //hN26548B2h7s3DR9jWZ55OcQnBYvz_olhgm6YAKWU8rRGvCuHOkjM:thePassword123@/api/v1/transfer_limits/batch' # at t/db_dependent/api/v1/transfer_limits.t line 210.
# Failed test 'SWAGGER3.2.1'
# at t/db_dependent/api/v1/transfer_limits.t line 210.
# got: undef
# expected: '201'
t/db_dependent/api/v1/transfer_limits.t .. 4/4 # Looks like you failed 2 tests of 26.
# Failed test 'batch_add() and batch_delete() tests'
# at t/db_dependent/api/v1/transfer_limits.t line 269.
# Looks like you failed 1 test of 4.
t/db_dependent/api/v1/transfer_limits.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests
Test Summary Report
-------------------
t/db_dependent/api/v1/transfer_limits.t (Wstat: 256 Tests: 4 Failed: 1)
Failed test: 4
Non-zero exit status: 1
Files=1, Tests=4, 156 wallclock secs ( 0.03 usr 0.00 sys + 105.04 cusr 6.17 csys = 111.24 CPU)
Result: FAIL
real 2m36.183s
user 1m45.121s
sys 0m6.208s
No idea what to do then...
There might be a perf regression somewhere.
IMO those 2 nested loops need to be rewritten. 138 foreach my $from ( @from_branches ) {
139 foreach my $to ( @to_branches ) {
Created attachment 163134 [details] [review] Bug 36277: Do not fetch the whole library list Created attachment 163135 [details] [review] Bug 36277: Improve algorithmic complexity of batch_add The 2 nested loops are terrible in term of algorithmic complexity. Especially if we are fetching from there. The goal of this patch is to fetch all the limits outside of the loop. If you have 100 libraries, it will remove 100^2 - 1 fetches! This certainly arises because one of the tests does not clean correctly behind it. We overlapped here. Nice to see patches, though. Taking the liberty to speed this one up a bit. (In reply to Katrin Fischer from comment #11) > Taking the liberty to speed this one up a bit. The patches require proper SO+QA. It's not a trivial change and could introduce regressions. (08:41:37) Joubu: cait: I would not push 36277 without a proper review I did some tests, but I'd like to have a proper QA review on these please. Created attachment 163189 [details] [review] Bug 36277: Do not fetch the whole library list Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 163190 [details] [review] Bug 36277: Improve algorithmic complexity of batch_add The 2 nested loops are terrible in term of algorithmic complexity. Especially if we are fetching from there. The goal of this patch is to fetch all the limits outside of the loop. If you have 100 libraries, it will remove 100^2 - 1 fetches! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Wow, this is really quite a considerable improvement in my opinion. Code is clear to me and tests continue to pass before and after.. and pass consistently after too when I put the system under load now. Thanks Jonathan. Passing QA Created attachment 163199 [details] [review] Bug 36277: Do not fetch the whole library list Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 163200 [details] [review] Bug 36277: Improve algorithmic complexity of batch_add The 2 nested loops are terrible in term of algorithmic complexity. Especially if we are fetching from there. The goal of this patch is to fetch all the limits outside of the loop. If you have 100 libraries, it will remove 100^2 - 1 fetches! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I tested the UI too, and nothing seems to be broken. I noticed the endpoint allows setting things the UI doesn't, though. But I'll file a separate bug for that. Nice job as usual, Jonathan. Wait a minute :-D Created attachment 163225 [details] [review] Bug 36277: Avoid useless warnings As mentioned on bug 36329, this endpoint is only used with one of this parameters: * item_type * collection_code The other will be NULL both on the DB and the (deserialized) request body. For the data from the DB, the author added `|| q{}` but missed to do so on the incoming parameters when generates the hash key. This generates the following warnings when using from the UI: [2024/03/15 11:42:51] [WARN] Use of uninitialized value in sprintf at /kohadevbox/koha/Koha/REST/V1/TransferLimits.pm line 146. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.04 We still have this one failing seemingly randomly (first it was D11, now on U20) https://jenkins.koha-community.org/job/Koha_Master_U20/lastCompletedBuild/testReport/(root)/t_db_dependent_api_v1_transfer_limits_t/batch_add___and_batch_delete___tests__1683_/ :( The error now starts with "Premature connection close" instead of "Inactivity timeout" koha_1 | # Premature connection close koha_1 | koha_1 | # Failed test 'POST //jJk8CE_fqJgQlLwZpiokcDRYn3YvprTEr5hQLvRFIQjcQDdyCMiW__Dx5yc:thePassword123@/api/v1/transfer_limits/batch' koha_1 | # at t/db_dependent/api/v1/transfer_limits.t line 210. koha_1 | koha_1 | # Failed test 'SWAGGER3.2.1' koha_1 | # at t/db_dependent/api/v1/transfer_limits.t line 210. koha_1 | # got: undef koha_1 | # expected: '201' koha_1 | # Looks like you failed 2 tests of 26. koha_1 | koha_1 | # Failed test 'batch_add() and batch_delete() tests' koha_1 | # at t/db_dependent/api/v1/transfer_limits.t line 269. koha_1 | # Looks like you failed 1 test of 4. koha_1 | [11:07:21] t/db_dependent/api/v1/transfer_limits.t . A successful run on Jenkins (Koha_Master#2813 Docker_15) is slow koha_1 | [10:56:23] t/db_dependent/api/v1/transfer_limits.t ..................................... ok 41681 ms ( 0.01 usr 0.00 sys + 30.81 cusr 1.84 csys = 32.66 CPU) It takes ~10sec on my laptop and I compared with other tests, they are faster on Docker_15. Continue on bug 36356 to fix the number of libraries in the DB. But it seems that ~200 libraries is enough to reach the timeout. Backported to 23.05.x for upcoming 23.05.10 |