Bug 21974 - cxn_pool must be configurable
Summary: cxn_pool must be configurable
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Ere Maijala
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 22705
  Show dependency treegraph
 
Reported: 2018-12-07 13:16 UTC by Jonathan Druart
Modified: 2019-05-13 00:54 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21974: Make Elasticsearch connection settings configurable (2.08 KB, patch)
2018-12-17 14:25 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 21974: Make Elasticsearch connection settings configurable (2.13 KB, patch)
2019-03-05 11:03 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21974: Make Elasticsearch connection settings configurable (2.20 KB, patch)
2019-03-26 07:57 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-12-07 13:16:34 UTC
On Koha::SearchEngine::Elasticsearch:92 Search::Elasticsearch is instantiate with cxn_pool => 'Sniff'

The doc says:

"""
The Sniff connection pool should be used when you do have direct access to the Elasticsearch cluster, eg when your web servers and Elasticsearch servers are on the same network.
"""
https://metacpan.org/pod/Search::Elasticsearch#cxn_pool


We must make this option configurable to allow elastic cluster running on a different network.

Original issue reporter on bug 21675 comment 8, they are connecting to the Amazon Elastic cluster.
Comment 1 Ere Maijala 2018-12-17 14:25:49 UTC
Created attachment 83291 [details] [review]
Bug 21974: Make Elasticsearch connection settings configurable

Default settings are appended in get_elasticsearch_params so that there's a central place for them. Defaults can be overridden in koha-conf.xml.

Test plan:

1. Verify that searches still work with the patch applied.
2. Verify that search fails after adding the following under <elasticsearch> in koha-conf.xml:
<cxn_pool>Static</cxn_pool>
3. Verify that search fails after adding the following under <elasticsearch> in koha-conf.xml:
<cxn_pool>FOO</cxn_pool>
Comment 2 Josef Moravec 2019-03-05 11:03:27 UTC
Created attachment 86052 [details] [review]
Bug 21974: Make Elasticsearch connection settings configurable

Default settings are appended in get_elasticsearch_params so that there's a central place for them. Defaults can be overridden in koha-conf.xml.

Test plan:

1. Verify that searches still work with the patch applied.
2. Verify that search fails after adding the following under <elasticsearch> in koha-conf.xml:
<cxn_pool>Static</cxn_pool>
3. Verify that search fails after adding the following under <elasticsearch> in koha-conf.xml:
<cxn_pool>FOO</cxn_pool>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 3 Martin Renvoize 2019-03-12 12:12:11 UTC
No tests here.. and we've snuck in configuration of not just cxn_pool, but also a couple of other variables.

I'd also like to see these newly exposed variables documented somewhere..

Failing QA for now.
Comment 4 Ere Maijala 2019-03-12 12:17:30 UTC
Documentation is one thing and I can take care of it. I'm not sure how to write any meaningful tests for this, however. Note that in practice the configurability changes nothing unless you actually add the settings to koha-conf.xml.
Comment 5 Martin Renvoize 2019-03-13 11:49:33 UTC
Interesting.. I can't find tests for this module at all.. only the sub-modules.  Perhaps I'm missing something..

Either way.. lets see some minimal documentation added and we can get this moving again.

Thanks Ere.
Comment 6 Ere Maijala 2019-03-13 12:19:49 UTC
I've now added a bit of documentation: https://wiki.koha-community.org/wiki/Elasticsearch#Advanced_Connection_Settings
Comment 7 Martin Renvoize 2019-03-26 07:57:34 UTC
Created attachment 87006 [details] [review]
Bug 21974: Make Elasticsearch connection settings configurable

Default settings are appended in get_elasticsearch_params so that there's a central place for them. Defaults can be overridden in koha-conf.xml.

Test plan:

1. Verify that searches still work with the patch applied.
2. Verify that search fails after adding the following under <elasticsearch> in koha-conf.xml:
<cxn_pool>Static</cxn_pool>
3. Verify that search fails after adding the following under <elasticsearch> in koha-conf.xml:
<cxn_pool>FOO</cxn_pool>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2019-03-26 07:59:05 UTC
OK, So I'm yielding a bit on the tests requirement here as there's not a precedent I can find for testing config checks.

Thanks for the documentation and my apologies for not getting back to this sooner.

Passed QA
Comment 9 Nick Clemens 2019-03-28 13:28:42 UTC
Awesome work all!

Pushed to master for 19.05
Comment 10 Martin Renvoize 2019-04-01 10:42:28 UTC
Pushed to 18.11.x for 18.11.05
Comment 11 Lucas Gass 2019-04-01 16:01:53 UTC
not backporting ES bug fixes/features to 18.05.x
Comment 12 Jonathan Druart 2019-05-13 00:54:55 UTC
On bug 22705 there is a complain about the default value of cxn_pool.