SOLR-16847 Give v2 APIs access to solrconfig.xml config #1778
Conversation
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
There was a problem hiding this comment.
This looks awesome, thanks @stillalex !
I left a few comments inline; if you can respond to those I'll switch this to 'Approve' and start testing/merging.
A bit of a larger macro question: should this PR modify an existing v2 API/request-handler to implement APIConfigProvider as a way to validate (and exhibit) the approach? Or would that bloat the PR too much (because it'd involve converting a new API to JAX-RS?)
solr/core/src/java/org/apache/solr/handler/admin/api/APIConfigProvider.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| protected void configure() { | ||
| bindFactory(cfgProvider).to(cfgProvider.getConfigClass()); |
There was a problem hiding this comment.
[Q] In HK2 you can set a "scope" for bindings, that impacts how often factories are asked to produce a new instance. e.g. We have a few bindings in JerseyApplications that are RequestScoped - meaning that Jersey prompts the factory on each API call.
In the case of these config objects - I think right now we expect them to be driven by solrconfig.xml config, which should be static through the life of the Jersey application. Tbh I forget what "scope" is used as a default when no explicit value is provided, but we should make sure it fits this expectations. It'd prob be wasteful if each API call triggered provide(), returning a functionally equivalent object each time.
There was a problem hiding this comment.
I agree with the perf aspect, this will get called on each call. my thought was that the config object gets created once and simply returned as many times as needed. I would suggest we leave as is and revisit this pattern if needed, but let me double check again on the scope parts.
stillalex
force-pushed
the
SOLR-16847-api-config-access
branch
from
d34c8ce
to
4017b23
Compare
this was my original thought, but to avoid this growing too large I was thinking we can push this in and pickup some API that needs it next, so we can iterate if needed. I also made sure this works by adding a test to showcase the functionality. |
SGTM, as long as you or I remember to follow up on that 😛 This LGTM, gonna run tests this afternoon and look to merge! |
|
thank you @gerlowskija! |
Prior to this commit, v2 APIs had no clean way to access any solrconfig.xml configuration for their associated "requestHandler". This commit introduces an interface, APIConfigProvider, which allows us to inject this configuration to v2 API classes at call time in a "strongly-typed" way.
https://issues.apache.org/jira/browse/SOLR-16847
Description
Please provide a short description of the changes you're making with this pull request.
Solution
Please provide a short description of the approach taken to implement your solution.
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.