[ 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.