[
https://issues.apache.org/jira/browse/SOLR-5653?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Timothy Potter updated SOLR-5653:
---------------------------------
Attachment: SOLR-5653.patch
Thanks for the thorough review Steve, very helpful! The updated patch cleans up
a number of the issues you raised, specifically:
{quote}1. In SolrDispatchFilter.doFilter(): The section starting if(
path.startsWith("/rest") ) appears to be a vestige?{quote}
Fixed (was vestige). Currently, the RestManager supports /schema and /config
only.
{quote}2. There's no way to create a new resource (i.e., one not mentioned in
schema.xml/solrconfig.xml){quote}
The RestManager's ManagedEndpoint class is now attached as the default resource
class (no longer DefaultSchemaResource). If a request to create a new resource
comes in, the PUT/POST request will be routed to the RestManager's
RestManagerManagedResource, which knows how to create new managed resources and
make them active in the server.
{quote}3. I don't see any REST API tests?{quote}
Most of the API testing is in the concrete endpoints (stop words / synonym) as
most of what needs to be tested in the RestManager layer can be tested without
making REST API calls. However, I've also added some additional REST API tests
to TestRestManager.
{quote}4. I think we should limit the permissible top-level registerable paths,
currently to /schema/ and /config/.{quote}
Done, but required the introduction of SolrConfigRestApi class and changes to
Solr's web.xml. At this point, ManagedResource implementers have to choose
which path makes the most sense for their resource, see
ManagedStopFilterFactory as an example.
We may be able to get away with 1 class that extends Restlet's Application by
implementing a custom Finder. However, I wanted to get something in place that
solidifies the paths the API will support as soon as possible. If we determine
a cleaner way to support /schema and /config, then we should be able to do that
without breaking client code.
{quote}5. In looking at the patch on SOLR-5641, which will need to be changed
to use the RestManager solution developed here ... {quote}
TBD - That's a large patch that will need more time to go over. There is some
overlap in that this patch also includes a SolrConfigRestApi.
{quote}6. It's important that configuration in schema.xml/solrconfig.xml
remains valid regardless of the operation of the RestManager interacting with
managed resources{quote}
I've added a check for having additional args to the
BaseManagedTokenFilterFactory however there's nothing in the RestManager
framework that enforces this.
The main idea behind my implementation is that the "managed" attribute is the
only one declared and then all initArgs are stored/managed in the managed data.
That said, I could see the validity of supporting invariant initArgs so this
might need some more work.
{quote}7. About your known issue c): "Deletes - the ManagedResource framework
supports deletes but I wasn't sure how to enable them in Restlet" - not sure
about Restlet, but there are two Solr code locations I know of that need to be
changed to enable new HTTP verbs (I ran into this when I added PUT support):
SolrRequestParsers.parseParamsAndFillStreams() and
SolrDispatchFilter.remoteQuery().{quote}
Thanks. Deletes are working now. However, a ManagedResource cannot be deleted
if there are Solr components still using it.
{quote}8. In ManagedWordSetResource, onManagedDataLoadedFromStorage() and
updateInitArgs(), the String.toLowerCase() calls should be given a Locale
argument (likely Locale.ROOT) to escape the wrath of forbidden-apis. Running
ant precommit at the top level will catch this kind of problem (and other
things).{quote}
Fixed.
{quote}9. You didn't mention it here, but if we want to enable using PATCH REST
calls via Restlet, we'll need to upgrade Restlet from the current v2.1.1 to at
least v2.2M1 - note that v2.2RC1 was just released. (See the issue adding PATCH
support and the v2.2RC1 release announcement.){quote}
We should tackle upgrading Restlet in a different JIRA issue and then PATCH
support can be added later if desired.
{quote}10. In ManagedWordSetResource.updateInitArgs(), if ignoreCase is changed
from true to false, the previous downcasing operation can't be undone. Not sure
the best way to handle this: maybe serialization should always capture the
unprocessed original versions?{quote}
This is a tough one. My thinking is that we shouldn't worry about this for now
since it seems like more work / trouble supporting than it may be worth and
seems like a client app issue vs. something that needs to be built-in to the
RestManager framework. That said, I'm also open to hearing about cases where we
should support this.
{quote}11. ManagedWordSetResource.doGet() ignores ignoreCase{quote}
Fixed. Improved the unit tests to verify this as well.
{quote}12. In the current model, all managed resource GET calls will return
dirty managed resource data, rather than live data. I think it's important to
make the dirty data accessible, but we should consider whether live data should
be accessible in addition, maybe as a param to the GET call?{quote}
The use case I designed for was that updates would be applied using a core /
collection reload very soon after submitting changes to the API. In other
words, the time where live data and dirty data are different should very small
and not all that important. Mainly, I don't want to start going down the path
of implementing as version control system for what is supposed to be a simple
API for changing config settings and then reloading the core to apply them.
{quote}13. In your tests, you escape double-quotes in JSON strings, but there's
a nice method in SolrTestCaseJ4 named json() that will accept single-quoted
string values and auto-convert to double-quotes. Makes the JSON much easier to
look at without all the backslashes.{quote}
Fixed. Thanks for tip, much nicer to look at indeed.
{quote}14. ManagedResource assumes JSON storage format, but the idea is to
override methods to handle other formats, right? I think there should be a
complete example of doing that in a test.{quote}
Added. Please see - TestManagedResource#testCustomStorageFormat
{quote}15. It seems weird to me that PUT requests in your patch respond with
the same information as GET would against the same resource. Maybe just return
a string indicating success? (I might be off-base here, I haven't looked at
many examples of this elsewhere.){quote}
Agreed on the weird part and fixed. Now just returns 200 status code
{quote}16. The patch ignores the (wt param) on GET methods - this should be
fairly simple to fix, by storing data structures rather than JSON strings on
the response; see e.g. CopyFieldCollectionResource.get().{quote}
Hmmm... Not sure what's up with this one as using wt=xml works for me without
changes. To be sure, I added test to verify XML as well as JSON.
{quote}17. In RestManager.doInit() on line 128, you have a TODO:{quote}
Good catch ... Thanks!
> Create a RESTManager to provide REST API endpoints for reconfigurable plugins
> -----------------------------------------------------------------------------
>
> Key: SOLR-5653
> URL: https://issues.apache.org/jira/browse/SOLR-5653
> Project: Solr
> Issue Type: Sub-task
> Reporter: Steve Rowe
> Attachments: SOLR-5653.patch, SOLR-5653.patch
>
>
> It should be possible to reconfigure Solr plugins' resources and init params
> without directly editing the serialized schema or {{solrconfig.xml}} (see
> Hoss's arguments about this in the context of the schema, which also apply to
> {{solrconfig.xml}}, in the description of SOLR-4658)
> The RESTManager should allow plugins declared in either the schema or in
> {{solrconfig.xml}} to register one or more REST endpoints, one endpoint per
> reconfigurable resource, including init params. To allow for multiple plugin
> instances, registering plugins will need to provide a handle of some form to
> distinguish the instances.
> This RESTManager should also be able to create new instances of plugins that
> it has been configured to allow. The RESTManager will need its own
> serialized configuration to remember these plugin declarations.
> Example endpoints:
> * SynonymFilterFactory
> ** init params: {{/solr/collection1/config/syns/myinstance/options}}
> ** synonyms resource:
> {{/solr/collection1/config/syns/myinstance/synonyms-list}}
> * "/select" request handler
> ** init params: {{/solr/collection1/config/requestHandlers/select/options}}
> We should aim for full CRUD over init params and structured resources. The
> plugins will bear responsibility for handling resource modification requests,
> though we should provide utility methods to make this easy.
> However, since we won't be directly modifying the serialized schema and
> {{solrconfig.xml}}, anything configured in those two places can't be
> invalidated by configuration serialized elsewhere. As a result, it won't be
> possible to remove plugins declared in the serialized schema or
> {{solrconfig.xml}}. Similarly, any init params declared in either place
> won't be modifiable. Instead, there should be some form of init param that
> declares that the plugin is reconfigurable, maybe using something like
> "managed" - note that request handlers already provide a "handle" - the
> request handler name - and so don't need that to be separately specified:
> {code:xml}
> <requestHandler name="/select" class="solr.SearchHandler">
> <managed/>
> </requestHandler>
> {code}
> and in the serialized schema - a handle needs to be specified here:
> {code:xml}
> <fieldType name="text_general" class="solr.TextField"
> positionIncrementGap="100">
> ...
> <analyzer type="query">
> <tokenizer class="solr.StandardTokenizerFactory"/>
> <filter class="solr.SynonymFilterFactory" managed="english-synonyms"/>
> ...
> {code}
> All of the above examples use the existing plugin factory class names, but
> we'll have to create new RESTManager-aware classes to handle registration
> with RESTManager.
> Core/collection reloading should not be performed automatically when a REST
> API call is made to one of these RESTManager-mediated REST endpoints, since
> for batched config modifications, that could take way too long. But maybe
> reloading could be a query parameter to these REST API calls.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
