[
https://issues.apache.org/jira/browse/SOLR-104?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12467305
]
Hoss Man commented on SOLR-104:
-------------------------------
Woot! ... i think we're really close to comiting this.
I made a hodgepodge list of comments as i read through everything, and then
tried to organize them. I agree with yonik that we should feel free to commit
new functionality without being afraid of needing to change the api of that
functionality befor the next release, but i'm not 100% comfortable with how
backwards compatible this patch is for the existing /select and /update URLs
... this may just be an issue of me being paranoid (and tired) but there's at
least one code path difference.
Anyway, here are my notes...
Comments regarding backwards compatibility of the patch...
- SolrCore.update(Reader,Writer) was a public method that's been
removed ... this is probably fine, just pointing it out for the
record.
- SolrUpdateServlet used HttpServletRequest.getReader, the new
UpdateRequestHandler uses an InputStreamReader arround
HttpServletRequest.getInputStream() ... this seems bad for legacy
update support from a char encoding standpoint.
- While i think it's important to refactor the XML Update
parsing out of SolrCore - I'm still not clear what is gained by
eliminating SolrServlet and SolrUpdate. The big advantage of
the new dispatcher being a Filter is that it can pass requests on
that it doesn't want to deal with, so why not leave the existing
servlets arround with only the minimum neccessary changes...
- move SolrCore's init to Dispatcher
- use 3 arg core.execute in SolrServlet
- have SolrUpdateServlet call UpdateRequestHandler.update(Reader)
and generate the legacy response XML
...in order to reduce the possibility of an introducing bugs
(particularly since the existing Servlets are the one area where we
don't have *any* unit tests)
Comments regarding functionality that i think we *may* want to address
before commiting (but i won't fight over if i'm the only one that cares)...
- UpdateRequestHandler should probably renamed XmlUpdateRequestHandler
(particularly since i expect Yonik to commit a
CsvUpdateRequestHandler real soon now)
- StandardRequestParser can't assume that a POST which isn't
multipart/* should be handled by a RawRequestParser ... if the
content type is "application/x-www-form-urlencoded" then
SimpleRequestParser should be used (so all params from query string
and body are included)
- What should the expectations of
ContentStream.getInputStream().close() be? Should the Dispatcher
iterate over any Iterable streams when writing the output and try
to close them, ignoring any Exceptions?
- I'm really not fond of having SolrParams.STREAM_TYPE. Can we please,
please leave it out for now and rely on on content-type detection?
We can add it back in if/when we make RequestParser a public
interface and let people register them in solrconfig.
- I really don't think we want to open the pandoras box of putting
the HttpServletRequest in the SolrQueryRequest ... i'd hate to put
that in and then have to support it forever.
Things in the current patch that aren't strictly neccessary
for the current issue and can (should?) be commited seperately...
- are we definitely deprecating SolrQueryResponse.getException ?
- StandardRequestHandler and DisMaxRequestHandler have only been
changed to subclass the new base class.
- only whitespace changes in SolrRequestHandler.java
- SolrServletRequest has only imports rearranged
Things which definitely shouldn't block up the patch, but should go on
a short term todo list...
- see backwards compatibility comment about (Xml)UpdateRequestHandler
using InputStreamReader without specifying a charset ... in general
the handler should look at the ContentStream's content type to determine
the encoding of the InputStream (and probably default to UTF-8)
- need to work out what kind of NamedList should be returned by
(Xml)UpdateRequestHandler.update(Reader)
- some of the new files are missing the Apache boilerplate.
- a use case we talked about that still isn't covered is opening local
files as a stream ... this should be easy to add later right along
side STREAM_URL.
- we should fill in the getURL methods for DisMax/Standard to point at wiki
- CommitRequestHandler should use UpdateParams.OPTIMIZE
- the init semantics for (Xml)UpdateRequestHandler are odd: as a
RequestHandler it's garunteed that init(NamedList) will be called, but
instead it uses it's own private init() that's called lazily.
- DumpRequestHandler should dump ContentStream.getSize().
- doFilter should call parsers.parse( path, req ) as soon as it has
the path, and then delegate to a helper method that doesn't have
access to the HttpServletRequest ... this reduces both the
complexity of the method, and the likelyhood of
someone inadvertently introducing an error (mangling the POST body)
when making future changes in the long deep method.
The one thing to watch out for is forcing POST in legacy
update. (assuming legacy update stays in the Filter)
- STREAM_URL based ContentStreams can have a meaningful getSize()
and getContentType() if we use openConnection instead of openStream.
- STREAM_BODY based ContentStreams can get their size from the char[]
- it's not clear to me why the interface for SolrRequestParser is...
public SolrParams parseParamsAndFillStreams(
final HttpServletRequest, ArrayList<ContentStream>)
...instead of just...
public SolrQueryRequest(final HttpServletRequest)
...with the param parsing loops in
SolrRequestParsers.buildRequestFrom
in static utility methods (in case a RequestParser doesn't want to
support STREAM_URL or STREAM_BODY)
> Update Plugins
> --------------
>
> Key: SOLR-104
> URL: https://issues.apache.org/jira/browse/SOLR-104
> Project: Solr
> Issue Type: Improvement
> Components: update
> Affects Versions: 1.2
> Reporter: Ryan McKinley
> Fix For: 1.2
>
> Attachments: commons-fileupload-20070107.jar, commons-io-1.2.jar,
> DispatchFilter.patch, DispatchFilter.patch, DispatchFilter.patch,
> DispatchFilter.patch, DispatchFilter.patch, DispatchFilter.patch,
> HandlerRefactoring-DRAFT-SRC.zip, HandlerRefactoring-DRAFT-SRC.zip,
> HandlerRefactoring.DRAFT.patch, HandlerRefactoring.DRAFT.patch,
> HandlerRefactoring.DRAFT.zip
>
>
> The plugin framework should work for 'update' actions in addition to 'search'
> actions.
> For more discussion on this, see:
> http://www.nabble.com/Re%3A-Handling-disparate-data-sources-in-Solr-tf2918621.html#a8305828
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
