Description
Nick Clemens (kidclamp)
2017-10-20 15:37:40 UTC
Created attachment 73020 [details] [review] Bug 19502: Limit pagination to first 10000 results when using ES This patch is to avoid hitting an error page. We should eventually make the max number returned configurable for ES. To test: 1 - Have Koha running ES with 10,000+ records 2 - Search for '*' 3 - Click 'Last' to view last page of results 4 - 'Cannot perform search' error 5 - Apply patch 6 - Search again 7 - View 'Last' page 8 - No error, you go to the last of 10000 9 - Note the warning above the pagination buttons Created attachment 73024 [details] [review] Bug 19502: Limit pagination to first 10000 results when using ES This patch is to avoid hitting an error page. We should eventually make the max number returned configurable for ES. To test: 1 - Have Koha running ES with 10,000+ records 2 - Search for '*' 3 - Click 'Last' to view last page of results 4 - 'Cannot perform search' error 5 - Apply patch 6 - Search again 7 - View 'Last' page 8 - No error, you go to the last of 10000 9 - Note the warning above the pagination buttons Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Works as described for both staff client and opac. Thanks Nick ! Created attachment 74587 [details] [review] Bug 19520: (follow-up) Pass parameters to avoid making templates depend on search engine I think we can do better than hardcoding the ES default value for max_result_window. We can retrieve the actual setting from ES, so that anyone can adjust this setting depending on their needs. I'm currently preparing a patch that will do that. Created attachment 75816 [details] [review] Bug 19502: Retrieve index.max_result_window from ES This avoid hardcoding '10000' in two different places and allow users to adjust this setting. Also, this patch fixes a bug when the search return less than 10000 results Test plan: 1. Do a search that returns 10000+ records. 2. Note the warning above the pagination buttons 3. Go to the last page, no error 4. Change the ES setting: curl -XPUT http://elasticsearch/koha_master_biblios/_settings -d \ '{"index": {"max_result_window": 20000}}' 5. Do another search that returns more than 10000 but less than 20000 6. Note that the warning does not show up 7. Go to the last page, still no error Nick, can you take a look at the last patch please ? Comment on attachment 74587 [details] [review] Bug 19520: (follow-up) Pass parameters to avoid making templates depend on search engine Review of attachment 74587 [details] [review]: ----------------------------------------------------------------- ::: catalogue/search.pl @@ +677,4 @@ > last_page_offset => $last_page_offset, > previous_page_offset => $previous_page_offset) unless $pages < 2; > $template->param( next_page_offset => $next_page_offset) unless $pages eq $current_page_number; > + warn "topage $hits_to_paginate"; Debug thing ? (In reply to Alex Arnaud from comment #8) > Debug thing ? Yes. It can be removed. Created attachment 76371 [details] [review] Bug 19502: Remove debug warn Created attachment 76372 [details] [review] Bug 19502: Add POD for max_result_window I don't have enough data to test this properly, but seems straight forward. Alex, maybe you could have another look? (In reply to Katrin Fischer from comment #12) > I don't have enough data to test this properly, but seems straight forward. > Alex, maybe you could have another look? I already had many looks. I'd like Nick to valid/sign Julian's patches and i could pass QA. (In reply to Alex Arnaud from comment #13) > (In reply to Katrin Fischer from comment #12) > > I don't have enough data to test this properly, but seems straight forward. > > Alex, maybe you could have another look? > > I already had many looks. I'd like Nick to valid/sign Julian's patches and i > could pass QA. Sounds like a plan. I wished Nick would review my patch to be sure that it doesn't go against his first patches. But I think this should not prevent the QA to be done. He can review the patches while pushing them ;) (In reply to Katrin Fischer from comment #12) > I don't have enough data to test this properly, but seems straight forward. > Alex, maybe you could have another look? If you want, here is a tiny script to generate thousands of new biblio: https://nopaste.xyz/?6a713982d73e1ab2#44I4RKe2ynghhda8Gp48a9f3bQSXjWbPUoeawx1Npqc= (You may want to change the title field for MARC21) Created attachment 76788 [details] [review] Bug 19502: Limit pagination to first 10000 results when using ES This patch is to avoid hitting an error page. We should eventually make the max number returned configurable for ES. To test: 1 - Have Koha running ES with 10,000+ records 2 - Search for '*' 3 - Click 'Last' to view last page of results 4 - 'Cannot perform search' error 5 - Apply patch 6 - Search again 7 - View 'Last' page 8 - No error, you go to the last of 10000 9 - Note the warning above the pagination buttons Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 76789 [details] [review] Bug 19502: (follow-up) Pass parameters to avoid making templates depend on search engine https://bugs.koha-community.org/show_bug.cgi?id=19502 Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 76790 [details] [review] Bug 19502: Retrieve index.max_result_window from ES This avoid hardcoding '10000' in two different places and allow users to adjust this setting. Also, this patch fixes a bug when the search return less than 10000 results Test plan: 1. Do a search that returns 10000+ records. 2. Note the warning above the pagination buttons 3. Go to the last page, no error 4. Change the ES setting: curl -XPUT http://elasticsearch/koha_master_biblios/_settings -d \ '{"index": {"max_result_window": 20000}}' 5. Do another search that returns more than 10000 but less than 20000 6. Note that the warning does not show up 7. Go to the last page, still no error Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 76791 [details] [review] Bug 19502: Remove debug warn Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 76792 [details] [review] Bug 19502: Add POD for max_result_window Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 76794 [details] [review] Bug 19502: Limit pagination to first 10000 results when using ES This patch is to avoid hitting an error page. We should eventually make the max number returned configurable for ES. To test: 1 - Have Koha running ES with 10,000+ records 2 - Search for '*' 3 - Click 'Last' to view last page of results 4 - 'Cannot perform search' error 5 - Apply patch 6 - Search again 7 - View 'Last' page 8 - No error, you go to the last of 10000 9 - Note the warning above the pagination buttons Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 76795 [details] [review] Bug 19502: (follow-up) Pass parameters to avoid making templates depend on search engine https://bugs.koha-community.org/show_bug.cgi?id=19502 Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 76796 [details] [review] Bug 19502: Retrieve index.max_result_window from ES This avoid hardcoding '10000' in two different places and allow users to adjust this setting. Also, this patch fixes a bug when the search return less than 10000 results Test plan: 1. Do a search that returns 10000+ records. 2. Note the warning above the pagination buttons 3. Go to the last page, no error 4. Change the ES setting: curl -XPUT http://elasticsearch/koha_master_biblios/_settings -d \ '{"index": {"max_result_window": 20000}}' 5. Do another search that returns more than 10000 but less than 20000 6. Note that the warning does not show up 7. Go to the last page, still no error Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 76798 [details] [review] Bug 19502: Remove debug warn Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 76799 [details] [review] Bug 19502: Add POD for max_result_window Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> yay! Awesome work all! Pushed to master for 18.11 Pushed to 18.05.x for 18.05.02 |