Description
Ere Maijala
2018-01-23 13:17:01 UTC
Created attachment 70827 [details] [review] Bug 20073: Move Elasticsearch configs to yaml files and improve the default settings. The patch contains the following changes: - Index settings moved from code to etc/searchengine/elasticsearch/index_config.yaml. An alternative can be specified in koha-conf.xml. - Field settings moved from code to etc/searchengine/elasticsearch/field_config.yaml. An alternative can be specified in koha-conf.xml. - Default settings have been improved to remove punctuation from phrases used for sorting etc. - State variables are used for storing configuration to avoid parsing it multiple times. - A possibility to reset the fields too has been added to the reset operation of mappings administration. - mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. - An stdno field type has been added for standard identifiers. Comment on attachment 70827 [details] [review] Bug 20073: Move Elasticsearch configs to yaml files and improve the default settings. Oops, I omitted the required DB and UI changes. Created attachment 70836 [details] [review] Bug 20073: Move Elasticsearch configs to yaml files and improve the default settings. Created attachment 70837 [details] [review] Bug 20073: Add database and mappings UI support for isbn and stdno field types. Ok, so I'm fumbling a bit with git-bz here, but the current patches should do it. Note that the isbn field type has been added in anticipation of a future patch that adds a bit of special handling for ISBNs so that they're indexed in all forms. How could I test it? There are some stray sections related to payment systems left in the /etc/koha-conf.xml file and some whitespace errors. This seems to work otherwise, need a test plan, it might be easier to separate the move to files and the updates to mappings to two separate bugs for simpler tests plans, or can be comprehensive here. Should this depend on the improvement to mappings reset provided else where? Thanks for the comments. I'll work on adding a test plan and rebasing the patches. This doesn't necessarily require Bug 20248, though testing is more difficult without it. Created attachment 73054 [details] [review] Bug 20073 - Move Elasticsearch configs to yaml files and improve the default settings. Improvements: 1) Index settings moved from code to etc/searchengine/elasticsearch/index_config.yaml. An alternative can be specified in koha-conf.xml. 2) Field settings moved from code to etc/searchengine/elasticsearch/field_config.yaml. An alternative can be specified in koha-conf.xml. 3) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. An alternative can be specified in koha-conf.xml. 4) Default settings have been improved to remove punctuation from phrases used for sorting etc. 5) State variables are used for storing configuration to avoid parsing it multiple times. 6) A possibility to reset the fields too has been added to the reset operation of mappings administration. 7) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. 8) An stdno field type has been added for standard identifiers. To test: 1) Run tests in t/Koha/SearchEngine/Elasticsearch.t 2) Clear tables search_fields and search_marc_map 3) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 4) Verify that admin/searchengine/elasticsearch/mappings.pl displays the mappings properly, including ISBN and other standard number fields. 5) Index some records 6) Verify that you can find the records 7) Put <elasticsearch_index_mappings>non_existent</elasticsearch_index_mappings> to koha-conf.xml 8) Verify that admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 fails because it can't find non_existent. 9) Copy etc/searchengine/elasticsearch/mappings.yaml to a new location and make elasticsearch_index_mappings setting in koha-conf.xml point to it. 10) Make a change in the new mappings.yaml. 11) Clear table search_fields (mappings reset doesn't do it yet, see bug 20248) 12) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 13) Verify that the changes you made are now visible in the mappings UI Hey Ere, my install (Unimarc, ES 5.2.8) failed at step 6. I can index and find books on master. With 20073 applied The index seems to work, I don't have deprecated messages as on master. But I can't find anything, my index is empty: $ curl http://localhost:9200/_cat/indices?v health status index uuid pri rep docs.count docs.deleted store.size pri.store.size yellow open koha_robin_biblios oDy7ppLWTkS4uOeta1KijA 5 1 0 0 810b 810b yellow open koha_robin_authorities TFRTgnFYTCusa9pTuTI35Q 5 1 3865 0 6.9mb 6.9mb On master, it is filled as it should: $ curl http://localhost:9200/_cat/indices?v health status index uuid pri rep docs.count docs.deleted store.size pri.store.size yellow open koha_robin_authorities 0vd2mF0aTWCyvsKtQlrkhw 5 1 3865 0 4.9mb 4.9mb yellow open koha_robin_biblios hto7dbqNSSSN3Y4IG0FK0Q 5 1 856 0 10mb 10mb Or maybe I am missing something? Hi Nicolas, The test step 5 was probably quite bad. You should make sure to run misc/search_tools/rebuild_elastic_search.pl long enough for it to commit at least one set of records before it creates the index with the correct settings. Otherwise indexing will quietly fail (this would be the subject of another bug). So, something like misc/search_tools/rebuild_elastic_search.pl -d -b -v -v should do the trick if you make sure that you don't try to save any records in the cataloguing UI before the first commit. Otherwise you'll probably find only errors in /var/log/elasticsearch/elasticsearch.log. Bug 20248 contains some improvements to the indexing script. I have a lot of: [2018-03-19T15:55:21,033][DEBUG][o.e.a.b.TransportShardBulkAction] [0aw4dLe] [koha_robin_biblios][2] failed to execute bulk item (index) BulkShardRequest [[ko ha_robin_biblios][2]] containing [21] requests I'm also on a dev install and not on a kohadevbox. I try to make and make install again, to take in account the etc changes, but it didn't do any good... Is is working for you? We did a bit of testing but couldn't quite figure the issue out yet. It's working for me with MARC 21 and Unimarc. If anyone else is up to it, please feel free to test and provide feedback. Nicolas kindly provided me with test set of Unimarc records. I was able to import and index them without issues. Perhaps the issue something specific to ES version or something. I'm running 5.6.8. Created attachment 73138 [details] [review] Bug 20073 - Move Elasticsearch configs to yaml files and improve the default settings. Improvements: 1) Index settings moved from code to etc/searchengine/elasticsearch/index_config.yaml. An alternative can be specified in koha-conf.xml. 2) Field settings moved from code to etc/searchengine/elasticsearch/field_config.yaml. An alternative can be specified in koha-conf.xml. 3) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. An alternative can be specified in koha-conf.xml. 4) Default settings have been improved to remove punctuation from phrases used for sorting etc. 5) State variables are used for storing configuration to avoid parsing it multiple times. 6) A possibility to reset the fields too has been added to the reset operation of mappings administration. 7) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. 8) An stdno field type has been added for standard identifiers. To test: 1) Run tests in t/Koha/SearchEngine/Elasticsearch.t 2) Clear tables search_fields and search_marc_map 3) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 4) Verify that admin/searchengine/elasticsearch/mappings.pl displays the mappings properly, including ISBN and other standard number fields. 5) Index some records using the -d parameter with misc/search_tools/rebuild_elastic_search.pl to recreate the index 6) Verify that you can find the records 7) Put <elasticsearch_index_mappings>non_existent</elasticsearch_index_mappings> to koha-conf.xml 8) Verify that admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 fails because it can't find non_existent. 9) Copy etc/searchengine/elasticsearch/mappings.yaml to a new location and make elasticsearch_index_mappings setting in koha-conf.xml point to it. 10) Make a change in the new mappings.yaml. 11) Clear table search_fields (mappings reset doesn't do it yet, see bug 20248) 12) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 13) Verify that the changes you made are now visible in the mappings UI I managed to reproduce the problem and found a stupid typo in field_config.yaml. The latest patch should work. Created attachment 73144 [details] [review] Bug 20073 - Move Elasticsearch configs to yaml files and improve the default settings. Improvements: 1) Index settings moved from code to etc/searchengine/elasticsearch/index_config.yaml. An alternative can be specified in koha-conf.xml. 2) Field settings moved from code to etc/searchengine/elasticsearch/field_config.yaml. An alternative can be specified in koha-conf.xml. 3) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. An alternative can be specified in koha-conf.xml. 4) Default settings have been improved to remove punctuation from phrases used for sorting etc. 5) State variables are used for storing configuration to avoid parsing it multiple times. 6) A possibility to reset the fields too has been added to the reset operation of mappings administration. 7) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. 8) An stdno field type has been added for standard identifiers. To test: 1) Run tests in t/Koha/SearchEngine/Elasticsearch.t 2) Clear tables search_fields and search_marc_map 3) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 4) Verify that admin/searchengine/elasticsearch/mappings.pl displays the mappings properly, including ISBN and other standard number fields. 5) Index some records using the -d parameter with misc/search_tools/rebuild_elastic_search.pl to recreate the index 6) Verify that you can find the records 7) Put <elasticsearch_index_mappings>non_existent</elasticsearch_index_mappings> to koha-conf.xml 8) Verify that admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 fails because it can't find non_existent. 9) Copy etc/searchengine/elasticsearch/mappings.yaml to a new location and make elasticsearch_index_mappings setting in koha-conf.xml point to it. 10) Make a change in the new mappings.yaml. 11) Clear table search_fields (mappings reset doesn't do it yet, see bug 20248) 12) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 13) Verify that the changes you made are now visible in the mappings UI Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Created attachment 73383 [details] [review] Bug 20073 - Move Elasticsearch configs to yaml files and improve the default settings. Improvements: 1) Index settings moved from code to etc/searchengine/elasticsearch/index_config.yaml. An alternative can be specified in koha-conf.xml. 2) Field settings moved from code to etc/searchengine/elasticsearch/field_config.yaml. An alternative can be specified in koha-conf.xml. 3) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. An alternative can be specified in koha-conf.xml. 4) Default settings have been improved to remove punctuation from phrases used for sorting etc. 5) State variables are used for storing configuration to avoid parsing it multiple times. 6) A possibility to reset the fields too has been added to the reset operation of mappings administration. 7) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. 8) An stdno field type has been added for standard identifiers. To test: 1) Run tests in t/Koha/SearchEngine/Elasticsearch.t 2) Clear tables search_fields and search_marc_map 3) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 4) Verify that admin/searchengine/elasticsearch/mappings.pl displays the mappings properly, including ISBN and other standard number fields. 5) Index some records using the -d parameter with misc/search_tools/rebuild_elastic_search.pl to recreate the index 6) Verify that you can find the records 7) Put <elasticsearch_index_mappings>non_existent</elasticsearch_index_mappings> to koha-conf.xml 8) Verify that admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 fails because it can't find non_existent. 9) Copy etc/searchengine/elasticsearch/mappings.yaml to a new location and make elasticsearch_index_mappings setting in koha-conf.xml point to it. 10) Make a change in the new mappings.yaml. 11) Clear table search_fields (mappings reset doesn't do it yet, see bug 20248) 12) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 13) Verify that the changes you made are now visible in the mappings UI Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> + by visiting /cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1&reset_fields=1. This comment will conflict with bug 20248. True. I can address that in bug 20248 if this gets committed first. Would not it make more sense to have these new config entries inside the existing elasticsearch one? The existing elasticsearch entry is passed to Catmandu::Store::Elasticsearch as is, so it would require either cleaning it up to exclude stuff that's irrelevant to the store or restructuring the tag in a back-incompatible way. I'm open to both, but since Koha is not in control of what may be introduced as options in Catmandu, I thought it would be better to keep it separate from everything else. (In reply to Ere Maijala from comment #23) > The existing elasticsearch entry is passed to Catmandu::Store::Elasticsearch > as is, so it would require either cleaning it up to exclude stuff that's > irrelevant to the store or restructuring the tag in a back-incompatible way. > I'm open to both, but since Koha is not in control of what may be introduced > as options in Catmandu, I thought it would be better to keep it separate > from everything else. Is it a bug or a feature? Are we suppose to have something else than server or index_name? If not, it sounds easy to recreate it. Tomas, Nick, opinions? (In reply to Jonathan Druart from comment #24) > Is it a bug or a feature? Are we suppose to have something else than server > or index_name? > If not, it sounds easy to recreate it. Catmandu::Store::Elasticsearch uses Search::Elasticsearch underneath, and I believe we are supposed to able to pass on whatever it accepts (see https://metacpan.org/pod/Search::Elasticsearch). There are settings like trace_to and cxn_pool that can be useful and we shouldn't limit the settings to any currently-known set of options. (In reply to Ere Maijala from comment #25) > (In reply to Jonathan Druart from comment #24) > > Is it a bug or a feature? Are we suppose to have something else than server > > or index_name? > > If not, it sounds easy to recreate it. > > Catmandu::Store::Elasticsearch uses Search::Elasticsearch underneath, and I > believe we are supposed to able to pass on whatever it accepts (see > https://metacpan.org/pod/Search::Elasticsearch). There are settings like > trace_to and cxn_pool that can be useful and we shouldn't limit the settings > to any currently-known set of options. I think this is right, we can consider these two separate sets of options - server config and koha settings, could ultimately be organized as: <ES_settings> <ES_server_settings> <server> etc... </ES_server_settings> <ES_koha_settings> <ES_index_config> etc... </ES_koha_settings> </ES_settings> but this would really just be organizational and we don't do for all zebra stuff now so I would say could be new bug report and not a blocker here Created attachment 74420 [details] [review] Bug 20073 - Move Elasticsearch configs to yaml files and improve the default settings. Improvements: 1) Index settings moved from code to etc/searchengine/elasticsearch/index_config.yaml. An alternative can be specified in koha-conf.xml. 2) Field settings moved from code to etc/searchengine/elasticsearch/field_config.yaml. An alternative can be specified in koha-conf.xml. 3) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. An alternative can be specified in koha-conf.xml. 4) Default settings have been improved to remove punctuation from phrases used for sorting etc. 5) State variables are used for storing configuration to avoid parsing it multiple times. 6) A possibility to reset the fields too has been added to the reset operation of mappings administration. 7) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. 8) An stdno field type has been added for standard identifiers. To test: 1) Run tests in t/Koha/SearchEngine/Elasticsearch.t 2) Clear tables search_fields and search_marc_map 3) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 4) Verify that admin/searchengine/elasticsearch/mappings.pl displays the mappings properly, including ISBN and other standard number fields. 5) Index some records using the -d parameter with misc/search_tools/rebuild_elastic_search.pl to recreate the index 6) Verify that you can find the records 7) Put <elasticsearch_index_mappings>non_existent</elasticsearch_index_mappings> to koha-conf.xml 8) Verify that admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 fails because it can't find non_existent. 9) Copy etc/searchengine/elasticsearch/mappings.yaml to a new location and make elasticsearch_index_mappings setting in koha-conf.xml point to it. 10) Make a change in the new mappings.yaml. 11) Clear table search_fields (mappings reset doesn't do it yet, see bug 20248) 12) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 13) Verify that the changes you made are now visible in the mappings UI The new patch is same as before, just rebased on top of master. Created attachment 74421 [details] [review] Bug 20073 - Move Elasticsearch configs to yaml files and improve the default settings. Improvements: 1) Index settings moved from code to etc/searchengine/elasticsearch/index_config.yaml. An alternative can be specified in koha-conf.xml. 2) Field settings moved from code to etc/searchengine/elasticsearch/field_config.yaml. An alternative can be specified in koha-conf.xml. 3) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. An alternative can be specified in koha-conf.xml. 4) Default settings have been improved to remove punctuation from phrases used for sorting etc. 5) State variables are used for storing configuration to avoid parsing it multiple times. 6) A possibility to reset the fields too has been added to the reset operation of mappings administration. 7) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. 8) An stdno field type has been added for standard identifiers. To test: 1) Run tests in t/Koha/SearchEngine/Elasticsearch.t 2) Clear tables search_fields and search_marc_map 3) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 4) Verify that admin/searchengine/elasticsearch/mappings.pl displays the mappings properly, including ISBN and other standard number fields. 5) Index some records using the -d parameter with misc/search_tools/rebuild_elastic_search.pl to recreate the index 6) Verify that you can find the records 7) Put <elasticsearch_index_mappings>non_existent</elasticsearch_index_mappings> to koha-conf.xml 8) Verify that admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 fails because it can't find non_existent. 9) Copy etc/searchengine/elasticsearch/mappings.yaml to a new location and make elasticsearch_index_mappings setting in koha-conf.xml point to it. 10) Make a change in the new mappings.yaml. 11) Clear table search_fields (mappings reset doesn't do it yet, see bug 20248) 12) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 13) Verify that the changes you made are now visible in the mappings UI Okay, this one should really be correct. The previous patch contained one unrelated change. signed-off-by lines have been removed, do we need to retest? Should not be necessary, it was just a rebase (which I botched once, but should be fine now). Pushed to master for 18.05, thanks to everybody involved! Created attachment 74759 [details] [review] Bug 20073: Remove perlcritic error "return" statement with explicit "undef" at line 245, column 5. See page 199 of PBP. (Severity: 5) Thanks for the perlcritic fix, Jonathan! There is a blocker issue introduced by these patches. The installer process (using packages) explodes with: '/usr/share/koha/intranet/cgi-bin/etc/searchengine/elasticsearch/mappings.yaml' is empty or non-existent at /usr/lib/x86_64-linux-gnu/perl5/5.20/YAML/Syck.pm line 129. The concatenation is obviously wrong. the koha-conf entry is filled with: <!-- <elasticsearch_index_mappings>/etc/koha/sites/libraryname/searchengine/elasticsearch/mappings.yaml</elasticsearch_index_mappings> --> Which is wrong too. Please fix ASAP or it will be reverted before 18.05 is released. Created attachment 75438 [details] [review] Bug 20073: (follow-up) Correct paths The original patch moved elasticsearch.yaml from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. I didn't realize this affects packaging. I think it's best to revert the move and put the other yaml files into admin/searchengine/elasticsearch too. I'll attach a patch ASAP. Created attachment 75453 [details] [review] Bug 20073: Move Elasticsearch yaml files back to admin directory Attached a patch that moves the default yaml files back to admin directory. Tests pass but I don't know enough about packaging to test it. The koha-conf entry is a commented-out example, and I can't see anything wrong with it. I am going to revert this patch from master, 18.05 is released tomorrow and I will not have time to retest this one. It seems safer to consolidate and push it later. Reverted. Sending back to QA queue. Created attachment 75829 [details] [review] Bug 20073 - Move Elasticsearch configs to yaml files and improve the default settings. Improvements: 1) Index settings moved from code to etc/searchengine/elasticsearch/index_config.yaml. An alternative can be specified in koha-conf.xml. 2) Field settings moved from code to etc/searchengine/elasticsearch/field_config.yaml. An alternative can be specified in koha-conf.xml. 3) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. An alternative can be specified in koha-conf.xml. 4) Default settings have been improved to remove punctuation from phrases used for sorting etc. 5) State variables are used for storing configuration to avoid parsing it multiple times. 6) A possibility to reset the fields too has been added to the reset operation of mappings administration. 7) mappings.yaml has been moved from admin/searchengine/elasticsearch to etc/searchengine/elasticsearch. 8) An stdno field type has been added for standard identifiers. To test: 1) Run tests in t/Koha/SearchEngine/Elasticsearch.t 2) Clear tables search_fields and search_marc_map 3) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 4) Verify that admin/searchengine/elasticsearch/mappings.pl displays the mappings properly, including ISBN and other standard number fields. 5) Index some records using the -d parameter with misc/search_tools/rebuild_elastic_search.pl to recreate the index 6) Verify that you can find the records 7) Put <elasticsearch_index_mappings>non_existent</elasticsearch_index_mappings> to koha-conf.xml 8) Verify that admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 fails because it can't find non_existent. 9) Copy etc/searchengine/elasticsearch/mappings.yaml to a new location and make elasticsearch_index_mappings setting in koha-conf.xml point to it. 10) Make a change in the new mappings.yaml. 11) Clear table search_fields (mappings reset doesn't do it yet, see bug 20248) 12) Go to admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 13) Verify that the changes you made are now visible in the mappings UI Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 75830 [details] [review] Bug 20073: Remove perlcritic error "return" statement with explicit "undef" at line 245, column 5. See page 199 of PBP. (Severity: 5) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 75831 [details] [review] Bug 20073: Move Elasticsearch yaml files back to admin directory Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Could we get this back into master soon? There were only bug fixes pushed for a bit of time after the new release to ease backporting, but I think pushing enh will happen soon now. Awesome work all! Pushed to aster for 18.11.x Squashed 1st and 3rd patches on push Created attachment 76315 [details] [review] Bug 20073: (RM follow-up) Remove atomicupdate D'oh Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Enhancement, not back porting to 18.05.x series. Great! Nick, DBIC schema has not been updated. Created attachment 76520 [details] [review] Bug 20073: (RM follow-up) Update schema files |