Bug 36277 - t/db_dependent/api/v1/transfer_limits.t is failing
Summary: t/db_dependent/api/v1/transfer_limits.t is failing
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 36329
  Show dependency treegraph
 
Reported: 2024-03-07 15:56 UTC by Katrin Fischer
Modified: 2024-04-26 20:28 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.10


Attachments
Bug 36277: Do not fetch the whole library list (1.56 KB, patch)
2024-03-14 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36277: Improve algorithmic complexity of batch_add (2.91 KB, patch)
2024-03-14 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36277: Do not fetch the whole library list (1.62 KB, patch)
2024-03-15 10:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36277: Improve algorithmic complexity of batch_add (2.97 KB, patch)
2024-03-15 10:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36277: Do not fetch the whole library list (1.67 KB, patch)
2024-03-15 11:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36277: Improve algorithmic complexity of batch_add (3.03 KB, patch)
2024-03-15 11:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36277: Avoid useless warnings (1.51 KB, patch)
2024-03-15 12:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2024-03-07 15:56:47 UTC
This failure is from the last run on master:

https://jenkins.koha-community.org/job/Koha_Master/

koha_1       |     # Inactivity timeout
koha_1       | 
koha_1       |     #   Failed test 'POST //qBiciZO7stgLDtFSpp1ZivUHaHyn8cMP1d5DXRI: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.
Comment 1 Katrin Fischer 2024-03-07 15:56:59 UTC
Passes locally for me.
Comment 2 Katrin Fischer 2024-03-09 12:16:27 UTC
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'
Comment 3 Jonathan Druart 2024-03-13 12:42:58 UTC
Koha_Master_MDB_Latest/1359
Comment 4 Jonathan Druart 2024-03-13 13:35:05 UTC
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.
Comment 5 Jonathan Druart 2024-03-13 13:38:04 UTC
IMO those 2 nested loops need to be rewritten.
Comment 6 Jonathan Druart 2024-03-13 13:38:36 UTC
138         foreach my $from ( @from_branches ) {
139             foreach my $to ( @to_branches ) {
Comment 7 Jonathan Druart 2024-03-14 13:42:44 UTC
Created attachment 163134 [details] [review]
Bug 36277: Do not fetch the whole library list
Comment 8 Jonathan Druart 2024-03-14 13:42:46 UTC
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!
Comment 9 Jonathan Druart 2024-03-14 13:45:19 UTC
This certainly arises because one of the tests does not clean correctly behind it.
Comment 10 Tomás Cohen Arazi 2024-03-14 16:02:30 UTC
We overlapped here. Nice to see patches, though.
Comment 11 Katrin Fischer 2024-03-15 07:03:58 UTC
Taking the liberty to speed this one up a bit.
Comment 12 Jonathan Druart 2024-03-15 07:41:14 UTC
(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.
Comment 13 Katrin Fischer 2024-03-15 07:50:23 UTC
(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.
Comment 14 Martin Renvoize 2024-03-15 10:36:04 UTC
Created attachment 163189 [details] [review]
Bug 36277: Do not fetch the whole library list

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2024-03-15 10:36:07 UTC
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>
Comment 16 Martin Renvoize 2024-03-15 10:37:19 UTC
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
Comment 17 Tomás Cohen Arazi 2024-03-15 11:03:25 UTC
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>
Comment 18 Tomás Cohen Arazi 2024-03-15 11:03:28 UTC
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>
Comment 19 Tomás Cohen Arazi 2024-03-15 11:11:53 UTC
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.
Comment 20 Tomás Cohen Arazi 2024-03-15 11:27:12 UTC
Wait a minute :-D
Comment 21 Tomás Cohen Arazi 2024-03-15 12:10:18 UTC
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>
Comment 22 Katrin Fischer 2024-03-15 14:40:26 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 23 Fridolin Somers 2024-03-18 09:24:48 UTC
Pushed to 23.11.x for 23.11.04
Comment 24 Katrin Fischer 2024-03-18 13:58:51 UTC
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_/

:(
Comment 25 Jonathan Druart 2024-03-18 15:41:08 UTC
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.
Comment 26 Jonathan Druart 2024-03-19 08:21:54 UTC
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.
Comment 27 Lucas Gass 2024-03-19 21:50:58 UTC
Backported to 23.05.x for upcoming 23.05.10