[
https://issues.apache.org/jira/browse/SOLR-5653?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13902598#comment-13902598
]
Steve Rowe commented on SOLR-5653:
----------------------------------
Hi Tim,
I've looked over your code, and it looks very well thought out - nice!
A few comments, in no particular order - I intentionally haven't looked at your
synonyms/stopwords patches yet, so apologies in advance if that results in
misunderstanding on my part:
# In {{SolrDispatchFilter.doFilter()}}: The section starting {{if(
path.startsWith("/rest") )}} appears to be a vestige? It's not used anywhere.
(May be related to your known issue *a)* about RestManager endpoints?)
# There's no way to create a new resource (i.e., one not mentioned in
{{schema.xml}}/{{solrconfig.xml}}) - maybe that's what the above vestige was
intended for?
# I don't see any REST API tests?
# I think we should limit the permissible top-level registerable paths,
currently to {{/schema/}} and {{/config/}}. (This relates to your known issue
*a)* about RestManager endpoints.)
# In looking at the patch on SOLR-5641, which will need to be changed to use
the RestManager solution developed here, I couldn't figure out a way to have a
single Restlet {{Application}} subclass that covers more than one top-level
path ({{/schema/}} and {{/config/}} in that issue). If you look at the patch on
SOLR-5641, you'll see that it adds a new Restlet {{Application}} subclass. My
sense is that even if it might be possible to trick Restlet into doing this,
it's an anti-pattern. (This relates to your known issue *b)* about refactoring
BaseSchemaResource.)
# It's important that configuration in {{schema.xml}}/{{solrconfig.xml}}
remains valid regardless of the operation of the RestManager interacting with
managed resources; as written, it's possible to have initArgs in these files
that are contradicted by initArgs stored in a manage resource. This is bad. I
think we need to have an immutable initArgs concept, and it will either have to
be per-arg, or we'll have to disallow mixing explicit initArgs with modifiable
ones.
# 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()}}.
# 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).
# 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|https://github.com/restlet/restlet-framework-java/issues/502] and
[the v2.2RC1 release
announcement|http://blog.restlet.com/2014/02/13/restlet-framework-2-1-7-and-2-2-rc1-released/].)
# 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?
# {{ManagedWordSetResource.doGet()}} ignores {{ignoreCase}}
# 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?
# 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.
# {{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.
# 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.)
# 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()}}.
# In {{RestManager.doInit()}} on line 128, you have a TODO:
{code:java}
// TODO: Feels like there should be a method in restlet to do this
String rootPath = getRequest().getRootRef().toString();
String resourceRef = getRequest().getResourceRef().toString();
String resourceId = resourceRef.substring(rootPath.length());
int qsAt = resourceId.indexOf("?");
if (qsAt != -1) resourceId = resourceId.substring(0,qsAt);
{code}
Restlet's {{Reference}} class, an instance of which is returned by
{{getRequest.getResourceRef()}}, has a method
[{{getRelativeRef()}}|http://restlet.org/learn/javadocs/2.1/jse/api/org/restlet/data/Reference.html#getRelativeRef()]
that can extract a URI reference relative to an absolute base reference, so
you could do something like (untested):
{code:java}
Reference rootRef = getRequest().getRootRef();
String resourceId =
getRequest().getResourceRef().getRelativeRef(rootRef).getPath();
{code}
> 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
>
>
> 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: [email protected]
For additional commands, e-mail: [email protected]
