Ett 1788 solr fields - #248
Conversation
aelkiss
left a comment
There was a problem hiding this comment.
Queries are working locally for me with solr 9; I haven't tested locally with solr 6 yet.
I also still want to look some at the question of tests here.
I have some questions about some of the duplicated files, but the bigger question is when is the right time to try to disable interleave (A vs B) for solr 9 -- do we try to do that now, or do we wait until later? On the one hand, it may require fewer changes to keep it with interleave enabled than if we try to disable it. On the other, there are already some some workarounds here to make it work despite not having two different cores; this PR adds some additional workarounds, and I don't think we want to continue those workarounds indefinitely.
|
|
||
| side_by_side =0 | ||
| use_B_query= 0 | ||
| use_interleave=1 |
There was a problem hiding this comment.
Does it make sense to try to turn off interleave entirely for solr 9, since we don't have a separate core-y set up there?
There was a problem hiding this comment.
If use_interleave=1 works to disable interleave for solr9, that is something we should check
There was a problem hiding this comment.
Do we need additional copies of the files under solr9 when they are the same as in the base Config directory (i.e. will they be found?). I think this is the case at least for langformat.yaml and bindings.pl.
If we do need them under solr9, does it make sense to make a symlink into the parent Config directory for them, rather than copying?
There was a problem hiding this comment.
We might consider setting up a symlink; however, this method creates dependencies between Solr 6 and Solr 9 config files, so it’s important to keep track of these details in our documentation. Duplicating the files can also make the transition smoother when we’re ready to retire Solr 6, since it allows us to safely delete the folder when the time comes.
There was a problem hiding this comment.
Note that Solr9/facetconfig.pl is still reading the original copy (edit: and Eugene has already pointed that out in a comment):
my $lang_format_file = $ENV{SDRROOT} . '/ls/lib/Config/langformat.yaml';
If we don't intend to use the Solr9 copy then that's argument for not having it. I would expect that data to be invariant no matter which Solr we are using.
There was a problem hiding this comment.
As ls_config_dir is a MdpConfig key (ls/lib/Config/global.com), this variable is defined in the config object $C, and $C is not in scope inside facetconfig.pl because it only reaches variables from $ENV.
To keep things simple and aligned with my plan to remove all Solr 6 configurations once we're ready. I've removed the Solr 6 hardcoded path as follows.
- Resolve
langformat.yamlfrom this file's own directory (selected by thels_config_dirconfig key)my $lang_format_file = dirname(__FILE__) . '/langformat.yaml'; - Claude created a test to ensure that the identical file between Solr 6 and Solr 9 (AB_test_config, bindings.pl, langformat.yaml and jc_css_filelist.txt) do not change.
|
Config is duplicated across two trees (ls/lib/Config and .../solr9) instead of one source with overrides. It may be causing a drift, such as sdrnum missing from the solr9 copy. It's worth thinking about consolidating them, it's not a priority/blocker, just a thought. |
| - [author2, 50] | ||
| - [issn, 1] | ||
| - [isbn, 1] | ||
| - [oclc, 1] |
There was a problem hiding this comment.
Looks like [sdrnum, 1] is missing here, should it be added back?
| #my $rel_weights_file_A = $ENV{SDRROOT} . '/ls/lib/Config/dismax_A.yaml'; | ||
| #my $rel_weights_file_B = $ENV{SDRROOT} . '/ls/lib/Config/dismax_B.yaml'; | ||
|
|
||
| my $lang_format_file = $ENV{SDRROOT} . '/ls/lib/Config/langformat.yaml'; |
There was a problem hiding this comment.
Should this be using the new ls_config_dir variable?
There was a problem hiding this comment.
@eumalin I answered this question in the comment above.
| $C->set_object('MdpConfig', $config); | ||
|
|
||
| # additional configuration for click_logging and A/B testing | ||
| my $AB_test_config_filename = $ENV{SDRROOT} . '/ls/lib/Config/AB_test_config'; |
There was a problem hiding this comment.
Should this be using the new ls_config_dir variable?
There was a problem hiding this comment.
This is the A/B test file, which is schema-independent. For now, I'll keep it hardcoded. The logic is:
solr9/ holds every file (for the clean mv solr9/* up at cutover), but only the schema-bearing ones load from it: facetconfig.pl, dismax_A/B.yaml, plus langformat.yaml (via dirname(__FILE__), since it's facetconfig.pl's own data file). AB_test_config, bindings.pl, js_css_filelist.txt stay hardcoded to ls/lib/Config/ and the test enforces they match their solr9/ copies.
I'll add this information to the documentation
Let's try disabling the interleave test for Solr 9 later; it should be an optimization, probably in the user stories related to measuring relevance results. Right now, the focus is ensuring we can run queries on Solr 9 and Solr 6 in the production environment. I'll create a task to disable interleave. In this PR, I kept the interleave but pointed to only |
I have duplicated the Config folders because some fields used in Solr 6 have been removed from Solr 9. |
| }; | ||
| }; | ||
|
|
||
| subtest 'solr9 facetconfig has incompatible fields removed' => sub { |
There was a problem hiding this comment.
Good to have these tests. They don't really belong here though. Really this is testing a component of FacetConfig.pm (which I will rant about a little bit later) so I would create a file ls/t/LS/FacetConfig.t and populate it with
use strict;
use warnings;
use Test::More;
<this subtest>
done_testing();
Then restore this file to its original state.
| - [lccn, 1] | ||
| - [oclc, 1] | ||
| - [sdrnum, 1] | ||
| - [ht_id, 1] |
There was a problem hiding this comment.
I think this should be id, not ht_id. I got an error trying with beta-1 when doing an "all fields" advanced search complaining about ht_id -- I don't think it's there in solr 6 either, but I guess it doesn't give an error?
4b714d4 to
b0e2d02
Compare
… parameterization) parameterize LSS_SOLR core name for Solr 9 core-x-only routing ls: resolve uber.conf mbooks_solr_engines TODO (slip/index/sync-i depends on it) ls: remove dead Operation::Login entry from bindings.pl %g_operation_params document LSS_CONFIG_DIR for running against Solr 9 split Solr 6/Solr 9 facet config into ls/lib/Config/solr9/ and add ls_config_dir -replicationFactor is deprecated, so replace by --replicationFactor Temporarly solution to fix the error Solr 9 could not create a new core restore /solr context path in solr-lss-dev-solr9 healthcheck) Adding tests for solr 6 and solr 9 directory and Resolve langformat.yaml from this file's own directory Creating the file ls/t/LS/FacetConfig.t to handle test related to solr9 config files Replace ht_id by id in solr9 config directory
b0e2d02 to
5745386
Compare
This PR is about the Jira ticket
The main goal was to split Solr 6/Solr 9 config files because Solr 9's schema removed the fields
ctrlnum, fulgeographic, fullgenre, and hlb3, documentation hereThe solution was to add
ls/lib/Config/solr9/as a copy ofls/lib/Config/and remove the fields from there. The folderls/lib/Config/is untouched and is the default configuration to start Babel accessing to Solr 6.A new
ls_config_dirconfig key has been defined. By default,LSS_CONFIG_DIR=ls/lib/Configand letsls/cgi/lsandLS::FacetConfig::get_rel_weights_from_yamlpick the right directory instead of hardcodingls/lib/Config. To connect to Solr9, the Compose profile LSS_CONFIG_DIR was defined inapache.yml/perl.yml.I've updated the
README.mdfile with the commands to run against Solr 9, addingLSS_CONFIG_DIR.I've tested these changes on both Solr 6 and Solr 9, and I get results. I'll listed the commands below, so you can reproduce them locally.
Testing Babel + Solr 6
Testing Babel + Solr 9
In
Solr 9, you should index the sample data on the server. The process for indexing data is explained here. I've tested it, and it works, but you will need to clone thelss_solr_configsrepository to your machine and follow the steps to run the scriptindexing_data.sh.If in the service
solr-lss-dev-solr9(docker logs solr-lss-dev-solr9) you see the errorCould not create a new core in /var/solr/data/core-x_shard1_replica_n1as another core is already defined there. This happens because the persist volumesbabel_solr1_dataandbabel_zoo1_dataexist from a previous run, and the create_collection call collided with the leftover directory. There is a Jira ticket to fix this problemTemporary solution: --> I'll create a task to fix this issue.
Following up on this ticket, it's a good time to start thinking about testing. Let's consider how we can verify that the existing LS tests cover both Solr 6 and Solr 9, and whether we might need to add any extra unit tests. Of course, this will be part of a different task and epic. Please feel free to add any additional tickets that came up during your review of this PR and that you think we should work on.
I've created some tests with Claude support on
ls/t/LS/Query/Facets.t