Bugzilla – Attachment 104503 Details for
Bug 24823
Drop Catmandu dependency
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 24823: Remove unused parameters
Bug-24823-Remove-unused-parameters.patch (text/plain), 14.20 KB, created by
Nick Clemens (kidclamp)
on 2020-05-07 12:13:12 UTC
(
hide
)
Description:
Bug 24823: Remove unused parameters
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2020-05-07 12:13:12 UTC
Size:
14.20 KB
patch
obsolete
>From 30e18f063a3942dd0da2a732993e36d22e17dabc Mon Sep 17 00:00:00 2001 >From: David Gustafsson <david.gustafsson@ub.gu.se> >Date: Thu, 12 Mar 2020 11:52:18 +0100 >Subject: [PATCH] Bug 24823: Remove unused parameters > >Remove unused Elasticsearch parameters 'key_prefix' and 'request_timeout'. >Refactor so that get_elasticsearch_params only returns parameters used >by Search::Elasticsearch, add class accessor for index_name. > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >Signed-off-by: Nick Clemens <nick@bywatersolutions.com> >--- > Koha/SearchEngine/Elasticsearch.pm | 75 +++++++++++----------- > Koha/SearchEngine/Elasticsearch/Browse.pm | 3 +- > Koha/SearchEngine/Elasticsearch/Indexer.pm | 24 +++---- > Koha/SearchEngine/Elasticsearch/Search.pm | 13 ++-- > t/Koha/SearchEngine/Elasticsearch.t | 6 +- > .../Koha/SearchEngine/Elasticsearch/Indexer.t | 10 +-- > .../Koha/SearchEngine/Elasticsearch/Search.t | 2 +- > 7 files changed, 62 insertions(+), 71 deletions(-) > >diff --git a/Koha/SearchEngine/Elasticsearch.pm b/Koha/SearchEngine/Elasticsearch.pm >index 6b64f97c59..3eaabca5b6 100644 >--- a/Koha/SearchEngine/Elasticsearch.pm >+++ b/Koha/SearchEngine/Elasticsearch.pm >@@ -45,7 +45,7 @@ use Encode qw(encode); > use Business::ISBN; > use Scalar::Util qw(looks_like_number); > >-__PACKAGE__->mk_ro_accessors(qw( index )); >+__PACKAGE__->mk_ro_accessors(qw( index index_name )); > __PACKAGE__->mk_accessors(qw( sort_fields )); > > # Constants to refer to the standard index names >@@ -66,15 +66,27 @@ The name of the index to use, generally 'biblios' or 'authorities'. > > =back > >+=item index_name >+ >+The Elasticsearch index name with Koha instance prefix. >+ >+=back >+ >+ > =head1 FUNCTIONS > > =cut > > sub new { > my $class = shift @_; >- my $self = $class->SUPER::new(@_); >+ my ($params) = @_; >+ > # Check for a valid index >- Koha::Exceptions::MissingParameter->throw('No index name provided') unless $self->index; >+ Koha::Exceptions::MissingParameter->throw('No index name provided') unless $params->{index}; >+ my $config = _read_configuration(); >+ $params->{index_name} = $config->{index_name} . '_' . $params->{index}; >+ >+ my $self = $class->SUPER::new(@_); > return $self; > } > >@@ -122,36 +134,21 @@ This is configured by the following in the C<config> block in koha-conf.xml: > sub get_elasticsearch_params { > my ($self) = @_; > >- # Copy the hash so that we're not modifying the original >- my $conf = C4::Context->config('elasticsearch'); >- die "No 'elasticsearch' block is defined in koha-conf.xml.\n" if ( !$conf ); >- my $es = { %{ $conf } }; >- >- # Helpfully, the multiple server lines end up in an array for us anyway >- # if there are multiple ones, but not if there's only one. >- my $server = $es->{server}; >- delete $es->{server}; >- if ( ref($server) eq 'ARRAY' ) { >- >- # store it called 'nodes' (which is used by newer Search::Elasticsearch) >- $es->{nodes} = $server; >- } >- elsif ($server) { >- $es->{nodes} = [$server]; >- } >- else { >- die "No elasticsearch servers were specified in koha-conf.xml.\n"; >- } >- die "No elasticsearch index_name was specified in koha-conf.xml.\n" >- if ( !$es->{index_name} ); >- # Append the name of this particular index to our namespace >- $es->{index_name} .= '_' . $self->index; >- >- $es->{key_prefix} = 'es_'; >- $es->{cxn_pool} //= 'Static'; >- $es->{request_timeout} //= 60; >+ my $conf; >+ try { >+ $conf = _read_configuration(); >+ } catch { >+ if ( ref($_) eq 'Koha::Exceptions::Config::MissingEntry' ) { >+ croak($_->message); >+ } >+ }; >+ # Extract relevant parts of configuration >+ my $params = { >+ nodes => $conf->{nodes} >+ }; >+ $params->{cxn_pool} //= 'Static'; > >- return $es; >+ return $params; > } > > =head2 get_elasticsearch_settings >@@ -1231,9 +1228,11 @@ sub _read_configuration { > my $configuration; > > my $conf = C4::Context->config('elasticsearch'); >- Koha::Exceptions::Config::MissingEntry->throw( >- "Missing 'elasticsearch' block in config file") >- unless defined $conf; >+ unless ( defined $conf ) { >+ Koha::Exceptions::Config::MissingEntry->throw( >+ "Missing <elasticsearch> entry in koha-conf.xml" >+ ); >+ } > > if ( $conf && $conf->{server} ) { > my $nodes = $conf->{server}; >@@ -1246,7 +1245,8 @@ sub _read_configuration { > } > else { > Koha::Exceptions::Config::MissingEntry->throw( >- "Missing 'server' entry in config file for elasticsearch"); >+ "Missing <elasticsearch>/<server> entry in koha-conf.xml" >+ ); > } > > if ( defined $conf->{index_name} ) { >@@ -1254,7 +1254,8 @@ sub _read_configuration { > } > else { > Koha::Exceptions::Config::MissingEntry->throw( >- "Missing 'index_name' entry in config file for elasticsearch"); >+ "Missing <elasticsearch>/<index_name> entry in koha-conf.xml", >+ ); > } > > return $configuration; >diff --git a/Koha/SearchEngine/Elasticsearch/Browse.pm b/Koha/SearchEngine/Elasticsearch/Browse.pm >index 1153e68ac5..85070730a7 100644 >--- a/Koha/SearchEngine/Elasticsearch/Browse.pm >+++ b/Koha/SearchEngine/Elasticsearch/Browse.pm >@@ -106,9 +106,8 @@ sub browse { > > my $query = $self->_build_query($prefix, $field, $options); > my $elasticsearch = $self->get_elasticsearch(); >- my $conf = $self->get_elasticsearch_params(); > my $results = $elasticsearch->search( >- index => $conf->{index_name}, >+ index => $self->index_name, > body => $query > ); > >diff --git a/Koha/SearchEngine/Elasticsearch/Indexer.pm b/Koha/SearchEngine/Elasticsearch/Indexer.pm >index aa50bc38ca..8f70817a6c 100644 >--- a/Koha/SearchEngine/Elasticsearch/Indexer.pm >+++ b/Koha/SearchEngine/Elasticsearch/Indexer.pm >@@ -96,8 +96,6 @@ Arrayref of C<MARC::Record>s. > sub update_index { > my ($self, $biblionums, $records) = @_; > >- my $conf = $self->get_elasticsearch_params(); >- my $elasticsearch = $self->get_elasticsearch(); > my $documents = $self->marc_records_to_documents($records); > my @body; > >@@ -113,8 +111,9 @@ sub update_index { > } > my $response; > if (@body) { >+ my $elasticsearch = $self->get_elasticsearch(); > $response = $elasticsearch->bulk( >- index => $conf->{index_name}, >+ index => $self->index_name, > type => 'data', # is just hard coded in Indexer.pm? > body => \@body > ); >@@ -236,14 +235,13 @@ failes. > > sub update_mappings { > my ($self) = @_; >- my $conf = $self->get_elasticsearch_params(); > my $elasticsearch = $self->get_elasticsearch(); > my $mappings = $self->get_elasticsearch_mappings(); > > foreach my $type (keys %{$mappings}) { > try { > my $response = $elasticsearch->indices->put_mapping( >- index => $conf->{index_name}, >+ index => $self->index_name, > type => $type, > body => { > $type => $mappings->{$type} >@@ -252,8 +250,9 @@ sub update_mappings { > } catch { > $self->set_index_status_recreate_required(); > my $reason = $_[0]->{vars}->{body}->{error}->{reason}; >+ my $index_name = $self->index_name; > Koha::Exceptions::Exception->throw( >- error => "Unable to update mappings for index \"$conf->{index_name}\". Reason was: \"$reason\". Index needs to be recreated and reindexed", >+ error => "Unable to update mappings for index \"$index_name\". Reason was: \"$reason\". Index needs to be recreated and reindexed", > ); > }; > } >@@ -288,11 +287,9 @@ sub delete_index { > my ($self, $biblionums) = @_; > > my $elasticsearch = $self->get_elasticsearch(); >- my $conf = $self->get_elasticsearch_params(); >- > my @body = map { { delete => { _id => $_ } } } @{$biblionums}; > my $result = $elasticsearch->bulk( >- index => $conf->{index_name}, >+ index => $self->index_name, > type => 'data', > body => \@body, > ); >@@ -322,9 +319,8 @@ Drops the index from the Elasticsearch server. > sub drop_index { > my ($self) = @_; > if ($self->index_exists) { >- my $conf = $self->get_elasticsearch_params(); > my $elasticsearch = $self->get_elasticsearch(); >- $elasticsearch->indices->delete(index => $conf->{index_name}); >+ $elasticsearch->indices->delete(index => $self->index_name); > $self->set_index_status_recreate_required(); > } > } >@@ -337,11 +333,10 @@ Creates the index (including mappings) on the Elasticsearch server. > > sub create_index { > my ($self) = @_; >- my $conf = $self->get_elasticsearch_params(); > my $settings = $self->get_elasticsearch_settings(); > my $elasticsearch = $self->get_elasticsearch(); > $elasticsearch->indices->create( >- index => $conf->{index_name}, >+ index => $self->index_name, > body => { > settings => $settings > } >@@ -358,10 +353,9 @@ empty string to indicate whether index exists or not. > > sub index_exists { > my ($self) = @_; >- my $conf = $self->get_elasticsearch_params(); > my $elasticsearch = $self->get_elasticsearch(); > return $elasticsearch->indices->exists( >- index => $conf->{index_name}, >+ index => $self->index_name, > ); > } > >diff --git a/Koha/SearchEngine/Elasticsearch/Search.pm b/Koha/SearchEngine/Elasticsearch/Search.pm >index 60af359227..ff56ad1174 100644 >--- a/Koha/SearchEngine/Elasticsearch/Search.pm >+++ b/Koha/SearchEngine/Elasticsearch/Search.pm >@@ -82,7 +82,6 @@ Returns > sub search { > my ($self, $query, $page, $count, %options) = @_; > >- my $params = $self->get_elasticsearch_params(); > # 20 is the default number of results per page > $query->{size} = $count // 20; > # ES doesn't want pages, it wants a record to start from. >@@ -95,7 +94,7 @@ sub search { > my $elasticsearch = $self->get_elasticsearch(); > my $results = eval { > $elasticsearch->search( >- index => $params->{index_name}, >+ index => $self->index_name, > body => $query > ); > }; >@@ -117,12 +116,11 @@ faster than pulling all the data in, usually. > sub count { > my ( $self, $query ) = @_; > my $elasticsearch = $self->get_elasticsearch(); >- my $conf = $self->get_elasticsearch_params(); > > # TODO: Probably possible to exclude results > # and just return number of hits > my $result = $elasticsearch->search( >- index => $conf->{index_name}, >+ index => $self->index_name, > body => $query > ); > >@@ -407,16 +405,15 @@ sub max_result_window { > my ($self) = @_; > > my $elasticsearch = $self->get_elasticsearch(); >- my $conf = $self->get_elasticsearch_params(); > > my $response = $elasticsearch->indices->get_settings( >- index => $conf->{index_name}, >+ index => $self->index_name, > flat_settings => 'true', > include_defaults => 'true' > ); > >- my $max_result_window = $response->{$conf->{index_name}}->{settings}->{'index.max_result_window'}; >- $max_result_window //= $response->{$conf->{index_name}}->{defaults}->{'index.max_result_window'}; >+ my $max_result_window = $response->{$self->index_name}->{settings}->{'index.max_result_window'}; >+ $max_result_window //= $response->{$self->index_name}->{defaults}->{'index.max_result_window'}; > > return $max_result_window; > } >diff --git a/t/Koha/SearchEngine/Elasticsearch.t b/t/Koha/SearchEngine/Elasticsearch.t >index a4608f66f2..fede84ca34 100644 >--- a/t/Koha/SearchEngine/Elasticsearch.t >+++ b/t/Koha/SearchEngine/Elasticsearch.t >@@ -46,7 +46,7 @@ subtest '_read_configuration() tests' => sub { > 'Configuration problem, exception thrown'; > is( > $@->message, >- "Missing 'elasticsearch' block in config file", >+ "Missing <elasticsearch> entry in koha-conf.xml", > 'Exception message is correct' > ); > >@@ -59,7 +59,7 @@ subtest '_read_configuration() tests' => sub { > 'Configuration problem, exception thrown'; > is( > $@->message, >- "Missing 'server' entry in config file for elasticsearch", >+ "Missing <elasticsearch>/<server> entry in koha-conf.xml", > 'Exception message is correct' > ); > >@@ -72,7 +72,7 @@ subtest '_read_configuration() tests' => sub { > 'Configuration problem, exception thrown'; > is( > $@->message, >- "Missing 'index_name' entry in config file for elasticsearch", >+ "Missing <elasticsearch>/<index_name> entry in koha-conf.xml", > 'Exception message is correct' > ); > >diff --git a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t >index 4ce73d8875..24f377df40 100644 >--- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t >+++ b/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t >@@ -32,12 +32,12 @@ use_ok('Koha::SearchEngine::Elasticsearch::Indexer'); > subtest 'create_index() tests' => sub { > plan tests => 5; > my $se = Test::MockModule->new( 'Koha::SearchEngine::Elasticsearch' ); >- $se->mock( 'get_elasticsearch_params', sub { >+ $se->mock( '_read_configuration', sub { > my ($self, $sub ) = @_; >- my $method = $se->original( 'get_elasticsearch_params' ); >- my $params = $method->( $self ); >- $params->{index_name} .= '__test'; >- return $params; >+ my $method = $se->original( '_read_configuration' ); >+ my $conf = $method->( $self ); >+ $conf->{index_name} .= '__test'; >+ return $conf; > }); > > my $indexer; >diff --git a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t >index 82ab287aba..e7d3b2ae62 100644 >--- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t >+++ b/t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t >@@ -117,7 +117,7 @@ SKIP: { > is ($searcher->max_result_window, 10000, 'By default, max_result_window is 10000'); > > $searcher->get_elasticsearch()->indices->put_settings( >- index => $searcher->get_elasticsearch_params()->{index_name}, >+ index => $searcher->index_name, > body => { > 'index' => { > 'max_result_window' => 12000, >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 24823
:
100268
|
100308
|
100337
|
100617
|
104303
|
104304
|
104305
|
104306
|
104307
|
104371
|
104372
|
104373
|
104501
|
104502
| 104503 |
104504