Hi, Clearly a refactor is needed here, thanks Gus for diving in deep.
Like Uwe I don't want us to be more dependent upon web.xml or Jetty's startup logic. That's why I filed SIP-6 (https://cwiki.apache.org/confluence/display/SOLR/SIP-6+Solr+should+own+the+bootstrap+process) to try to describe the vision that Uwe just described. If such a bootstrap redesign makes the dispatch cleanup easier or harder I don't know. Perhaps one should be done before the other? Or shuold be viewed as part of the same rewrite? Jan > 18. aug. 2021 kl. 12:00 skrev Uwe Schindler <[email protected]>: > > Hi, > > Thanks Mark, I fully agree with you on this: Booting up jetty with start.jar > and using a web application is dramatically slower. > > Nevertheless, I think that we should go away with the web application > alltogether. We can still stay with the servlet API, but just – as in Gus > patch – to split responsibilities and go away with a singleton ServletFilter, > although from my own experience, I figured out that Servletapis’s URI > handling is far away from ideal. In most cases at end I always landed in a > separate “master” servlet (hooked to “/*”) that only parses the PathInfo with > regular expressions (one thing that does not work with servlet api) and then > delegates using RequestDispatcher to several more specific servlets or > returns 404 not found. This setup would also help to split the huge > DispatchFilter mess, but still the URI parsing logic would be centralized. > But this servlet would only take care of the dispatching, so DispatchServlet > would be fine. But it should only dispatch, nothing else. > > My ideal Solr would be: Add some org.apache.solr.Bootstrap class to the main > distribution and remove start.jar completely. Also remove jetty-webapp.jar > and related dependencies. In Boostrap just start Jetty embedded, add listen > ip, port numbers, etc. We can still use the config API of Jetty for this, so > maybe put this into an XML file (although XML is a bad format for this stuff, > goes back to web applications). Then Bootsrap starts the Corecontainer and > the embedded Zookeeper (this is another horrible thing in Solr: the embedded > zookeper starts somehow as part of the web application) and hooks the > servlets into a (at first) single root ServletContextHandler. This is upside > down. So Solr starts with a Bootstrap class, starts zookeeper, jetty and > hands over to CoreContainer. > > Some other ideas (not sure if they are already implemented in the patch): > Maybe we should install a separate ServletContextHandler/Classloader for each > Core. This would allow us to go away with our own classloader magic. > Bootstrap could also do this: It starts the CoreContainer, Zookeeper, Jetty > Installs a servlet for the admin UI and the Core/Collection management and > then hands over to the CoreContainer to initialize all Cores. Each > CoreContainer gets his own ServletContextHandler and can therefore easily be > loaded unloaded from Jetty. > > I wrote many other apps in my daily live like this, the startup time > (especially for microservices) is great. A Jetty starting up in milliseconds > and going to service is unbeatable. > > Uwe > > ----- > Uwe Schindler > Achterdiek 19, D-28357 Bremen > https://www.thetaphi.de <https://www.thetaphi.de/> > eMail: [email protected] <mailto:[email protected]> > > From: Mark Miller <[email protected]> > Sent: Wednesday, August 18, 2021 9:08 AM > To: Gus Heck <[email protected]> > Cc: [email protected] > Subject: Re: Solr and Jetty and Servlets > > Agreement or not, I wouldn’t step in front of whatever you did. > > I doubt anyone is going to take on actually pulling out of a web app because > it’s easier to lean on it for configuration and I doubt start up speed is a > very high priority for most. > > It had never really occurred to me in the past that JettySolrRunner was so > much faster. I just know it because of driving the test down to ridiculously > times, starting up and stopping mini clusters of whatever size in literally > no time. With no QuickStart setup for embedded Jetty. > > So than starting up standalone Solr, it became apparent how much slower it > was. So I started poking around. Cloud guys like those at google had already > been feeling pain - preferring to be able to spin up and down relatively > quickly - and so this QuickStart “precompile” web.xml / JSP’s thing how they > were dealing with it. And it’s simple, but it’s still easily non comparable > at the end of the day. Solr ships with like 10,000 classes across a ton of > jars, even with QuickStart, you have to follow the spec of what to do and > what might be done by any given webapp - though as JettySolrRunner shows, we > don’t actually need any of it. QuickStart is trying to find every trick to > speed up following the container and webapp spec. Injecting the dispatch > filter just tosses it all out :) > > Any, may point was not that I disagree and you should make Solr inject a > dispatch filter - my point is just that it doesn’t really matter to dispatch. > You can do just as much via Jetty either way. > > And anyone that doesn’t agree about “something should be done” is either > lying or not looking. So I wouldn’t worry about agreement. The problem is > that it’s quicksand that people are avoiding, not that anyone that looks > can’t see it a huge rats nest. Look at the V2 API. Someone actually wanted to > work towards getting out of that hole. But at the end of the day, it’s now > part of the hole. Made it deeper. Bermuda Triangle. But look at it - there > was hope there. Hope that one day, dispatching would be better. Rather than > twice as complicated. I don’t know that hope lives on. But no one gonna stop > you on reasonable technical sense with any proper license. > > Mark > > > > On Tue, Aug 17, 2021 at 10:12 AM Gus Heck <[email protected] > <mailto:[email protected]>> wrote: >> *ServletContextHandler not ServletContext >> >> On Tue, Aug 17, 2021 at 11:03 AM Gus Heck <[email protected] >> <mailto:[email protected]>> wrote: >>> Ok nope, since that only happens with WebAppContext, not ServletContext >>> (the parent class). whew :) >>> >>> On Tue, Aug 17, 2021 at 10:33 AM Gus Heck <[email protected] >>> <mailto:[email protected]>> wrote: >>>> WRT the JettySolrRunner tangent, I wonder if our lack of a web.xml and >>>> thus lack of metadata-complete=true in web.xml means that we wind up >>>> scanning for annotations? >>>> >>>> On Tue, Aug 17, 2021 at 10:06 AM Gus Heck <[email protected] >>>> <mailto:[email protected]>> wrote: >>>>> https://issues.apache.org/jira/browse/SOLR-15590 >>>>> <https://issues.apache.org/jira/browse/SOLR-15590> >>>>> >>>>> On Tue, Aug 17, 2021 at 9:44 AM Gus Heck <[email protected] >>>>> <mailto:[email protected]>> wrote: >>>>>> Ok well it would be interesting to compare quickstarting vs >>>>>> JettySolrRunner, and reading up on quickstart >>>>>> <https://webtide.com/jetty-9-quick-start/> gives ideas about >>>>>> pre-building the quickstart xml, but web.xml for JettySolrRunner is a >>>>>> tangent. Either way we are still in a container and I think I hear some >>>>>> agreement that something should be done about the dispatch here, and >>>>>> both of you seem to agree that an actual servlet would make more sense >>>>>> than a filter, so I'll make a ticket/pr to make it easier to track & >>>>>> easier to read the code. >>>>>> >>>>>> On Tue, Aug 17, 2021 at 1:41 AM Mark Miller <[email protected] >>>>>> <mailto:[email protected]>> wrote: >>>>>>> The downside to respecting web.xml and making JettySolrServer serve a >>>>>>> webapp is that loading a webapp is very expensive and slow in >>>>>>> comparison. JettySolrServer actually starts up extremely quickly. It’s >>>>>>> almost more appealing to change the Server to use the JettySolrServer >>>>>>> strategy. It’s so slow to load a webapp because of all the things it >>>>>>> needs to support and scan jars for - in a kind of JavaEE situation, >>>>>>> though not as bad. Jetty QuickStart does improve the situation if used >>>>>>> at least. But there is really no need to eat the whole webapp standard >>>>>>> for a non webapp app to have Jetty manage more of the dispatching. I >>>>>>> always wondering about the motivation / upside of hacking in a straight >>>>>>> servlet myself. But for a non webapp stuck in a servlet container, it’s >>>>>>> actually a beautiful move that side steps a bunch of slow crap even >>>>>>> better than the QuickStart pushes ever will, and still allows for >>>>>>> matching any support we would want. >>>>>>> >>>>>>> The HttpSolrCallV2 extending HttpSolrCall is horrendous. Personally, I >>>>>>> made a slim SolrCall base class that each extends. All that logic is >>>>>>> hairy enough to follow without them interleaving and sharing in a kind >>>>>>> of wild collaboration. It’s pretty unfortunate they both still exist. >>>>>>> >>>>>>> You are right, leaving that path in that state is a poor idea. If there >>>>>>> was anything that could be considered the “hot” path, it’s right there >>>>>>> - feverishly checking and ripping up streams for anyone and anything. >>>>>>> Is a core? A collection? A handler? Maybe a V2 handler? What I’d we cut >>>>>>> the path down? Do a dance? Treat the string like a date and see if that >>>>>>> works. >>>>>>> >>>>>>> So yeah, agreed, nobody does or would dispatch this way, you have to >>>>>>> frog boil into it. It’s slow, almost incomprehensible, and incredibly >>>>>>> good at holding onto life, even building on it. But being a webapp and >>>>>>> parsing web.xml has little to do with dispatching and having sensible >>>>>>> http api that doesn’t work like it’s a perl compiler. >>>>>>> >>>>>>> Anyway, I can’t imagine trying to trace through that code with any >>>>>>> solid feeling about having a handle on it via inspection. But just like >>>>>>> Perl, if you debug / trace log the hell out of it, it is all actually >>>>>>> pretty simple to adjust and reign in. >>>>>>> >>>>>>> >>>>>>> MRM >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Mon, Aug 16, 2021 at 10:39 PM Gus Heck <[email protected] >>>>>>> <mailto:[email protected]>> wrote: >>>>>>>> So I'm not interested in *adhering* to anything here, just using stuff >>>>>>>> where it helps... I see tools sitting on the shelf, already bought and >>>>>>>> paid for and carried with us to every job but then they sit in the >>>>>>>> back of the truck mostly unused... and (I think) they look useful. >>>>>>>> >>>>>>>> This should not be seen as in any way advocating a return to war file >>>>>>>> deployment or "choose your container". All of what I would suggest >>>>>>>> should likely be compatible with a jetty startup controlled by our >>>>>>>> code (I assume we can convince it to read web.xml properly when doing >>>>>>>> so). Certainly point out anything that would interfere with that. >>>>>>>> >>>>>>>> Here we sit in a container that is a tool box containing: >>>>>>>> A nice mechanism for initializing stuff at the application level, >>>>>>>> (ServletContextListener) and a well defined guarantee that it runs >>>>>>>> before any filter or servlet, >>>>>>>> An automatic shutdown mechanism that it will call for us on graceful >>>>>>>> shutdown (ServletContextListner again). >>>>>>>> A nice layered filter chain mechanism which could have been used to >>>>>>>> layer in things like tracing and authentication, and close shields etc >>>>>>>> as small succinct filters rather than weaving them into an ever more >>>>>>>> complex filter & call class. >>>>>>>> In more recent versions of the spec, for listeners defined in web.xml >>>>>>>> the order is also guaranteed. >>>>>>>> Servlet classes that are *already* set up to distinguish http verbs >>>>>>>> automatically when desired >>>>>>>> So why is this better? Because monster classes that do a hundred >>>>>>>> things are really hard to understand and maintain. Small methods, >>>>>>>> small classes whenever possible. I also suspect that there may be some >>>>>>>> gains in performance to be had if we rely more on the container (which >>>>>>>> will already be dispatching based on path) to choose our code paths >>>>>>>> (at least at a coarse level) and then have less custom dispatch logic >>>>>>>> executed on *every* request >>>>>>>> >>>>>>>> Obviously I'm wrong and if the net result is less performant to any >>>>>>>> significant degree forget it that wouldn't be worth it. (wanted: >>>>>>>> standardized solr benchmarks) >>>>>>>> >>>>>>>> There WILL be complications with v2 because it is a subclass of >>>>>>>> HttpSolrCall, which will take a bit of teasing apart for sure. Ideally >>>>>>>> it should be a separate servlet from v1, but we don't want to >>>>>>>> duplicate code either... so work to do there... >>>>>>>> >>>>>>>> I think an incremental approach is necessary since very few of us have >>>>>>>> the bandwidth to more than that and certainly it becomes difficult to >>>>>>>> find anyone with the time to review large changes. What I have thus >>>>>>>> far is stable with respect to the tests, and simplifies some stuff >>>>>>>> already which is why I chose this point to start the discussion >>>>>>>> >>>>>>>> But yeah, look at what I did and say what you like and what you don't. >>>>>>>> :). >>>>>>>> >>>>>>>> -Gus >>>>>>>> >>>>>>>> On Mon, Aug 16, 2021 at 9:42 PM David Smiley <[email protected] >>>>>>>> <mailto:[email protected]>> wrote: >>>>>>>>> Sounds like an interesting adventure last weekend. >>>>>>>>> >>>>>>>>> I'm unclear what the point of going this direction is; my instinct is >>>>>>>>> to go the opposite direction. You seem to suggest there are some >>>>>>>>> simplification/organization benefits, which I love, so I'll need to >>>>>>>>> look at what you've done to judge that for myself. Yes Jetty >>>>>>>>> supports the Servlet spec but we need not embrace it. Adhering to >>>>>>>>> that is useful if you have a generic web app that can be deployed to >>>>>>>>> a container of the user's convenience/choosing. No doubt this is why >>>>>>>>> Solr started this way, and why the apps I built in my early days >>>>>>>>> adhered to that spec. But since 6.0, we view Solr as self-contained >>>>>>>>> and more supportable if the project makes these decisions and thus >>>>>>>>> not needlessly constrain itself as well. >>>>>>>>> >>>>>>>>> It is super weird to me that SolrDispatchFilter is a Servlet *Filter* >>>>>>>>> and not a *Servlet* itself. >>>>>>>>> >>>>>>>>> Also, I suspect there may be complications in changes here relating >>>>>>>>> to Solr's v1 vs v2 API. And most definitely also what you discovered >>>>>>>>> -- JettySolrRunner. >>>>>>>>> >>>>>>>>> ~ David Smiley >>>>>>>>> Apache Lucene/Solr Search Developer >>>>>>>>> http://www.linkedin.com/in/davidwsmiley >>>>>>>>> <http://www.linkedin.com/in/davidwsmiley> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Mon, Aug 16, 2021 at 11:37 AM Gus Heck <[email protected] >>>>>>>>> <mailto:[email protected]>> wrote: >>>>>>>>>> TLDR: I've got a working branch >>>>>>>>>> <https://github.com/gus-asf/solr/tree/servlet-solr> where >>>>>>>>>> CoreContainer & our startup process is extracted from SolrDispatch >>>>>>>>>> Filter. Do other folks think that is interesting enough that I >>>>>>>>>> should make a JIRA and/or PR with intention to make this change? >>>>>>>>>> >>>>>>>>>> Details: >>>>>>>>>> >>>>>>>>>> Jetty is a servlet container, yet we more or less ignore it by >>>>>>>>>> stuffing everything into a single filter (almost, admin UI is served >>>>>>>>>> separately). I'm sure there are lots of historical reasons for this, >>>>>>>>>> probably including servlet containers and their specs were much less >>>>>>>>>> mature when solr was first started. Maybe also the early authors >>>>>>>>>> were more focused on making search work than leveraging what the >>>>>>>>>> container could do for them (but I wasn't there for that so that's >>>>>>>>>> just a guess). >>>>>>>>>> >>>>>>>>>> The result is that we have a couple of very large classes that are >>>>>>>>>> touched by almost every request, and they have a lot of conditional >>>>>>>>>> logic trying to decide what the user is asking. Normally this sort >>>>>>>>>> of dispatch is done by the container based on the request URL to >>>>>>>>>> direct it to the appropriate servlet. Solr does a LOT of different >>>>>>>>>> things so this code is extremely tricky and complex to understand. >>>>>>>>>> Specifically, I'm speaking of SolrDispatchFilter and HttpSolrCall, >>>>>>>>>> which are so inseparable that being two classes probably makes them >>>>>>>>>> harder to understand. >>>>>>>>>> >>>>>>>>>> It seems to me (and this mail is asking if you agree) that these >>>>>>>>>> classes are long overdue for some subdivision. The most obvious >>>>>>>>>> thing to pull out is all the admin calls. Admin code paths really >>>>>>>>>> have little or nothing to do with query or update code paths since >>>>>>>>>> there are no documents to route or sub-requests to some subset of >>>>>>>>>> nodes. >>>>>>>>>> >>>>>>>>>> The primary obstacle to any such separation and simplification is >>>>>>>>>> that most requests have some interaction with CoreContainer, and the >>>>>>>>>> things it holds, and this is initialized and held by a field in >>>>>>>>>> SolrDispatchFilter. After spending a significant chunk of time >>>>>>>>>> reading this code in the prior weeks and a timely and motivating >>>>>>>>>> conversation with Eric Pugh, I dumped a chunk of my weekend into an >>>>>>>>>> experiment to see if I could pull CoreContainer out of the dispatch >>>>>>>>>> filter, and leverage the facilities of our servlet container. >>>>>>>>>> >>>>>>>>>> That wasn't too terribly hard, but keeping JettySolrRunner happy was >>>>>>>>>> very confusing, and worrisome since I've realized it's not >>>>>>>>>> respecting our web.xml at all, and any configuration in web.xml >>>>>>>>>> needs to be duplicated for our tests in JettySolrRunner (tangent >>>>>>>>>> alert) >>>>>>>>>> >>>>>>>>>> The result is that CoreContainer is now held by a class called >>>>>>>>>> CoreService (please help me name things if you don't like my names >>>>>>>>>> :) ). CoreService is a ServletContextListener, appropriately >>>>>>>>>> configured in web.xml, and has a static method that can be used to >>>>>>>>>> get a reference to the CoreContainer corresponding to the >>>>>>>>>> ServletContext in which code wanting a core container is running >>>>>>>>>> (this supports having multiple JettySolrRunners in tests, though >>>>>>>>>> probably never has more than one CoreContainer in the running >>>>>>>>>> application) >>>>>>>>>> >>>>>>>>>> I achieved this in 4 stages shown here: >>>>>>>>>> https://github.com/gus-asf/solr/tree/servlet-solr >>>>>>>>>> <https://github.com/gus-asf/solr/tree/servlet-solr> >>>>>>>>>> >>>>>>>>>> Ignore the AdminServlet class, it's a placeholder, and can be >>>>>>>>>> subtracted without harm. >>>>>>>>>> >>>>>>>>>> Since the current state of the code in that branch is apparently >>>>>>>>>> test-stable (4 runs of check in a row passing, none slower than any >>>>>>>>>> run of 3 runs of main, both as low as 10.5 min if I don't continue >>>>>>>>>> working on the machine)... >>>>>>>>>> >>>>>>>>>> Do we want to push this refactor in now to avoid making a huge ball >>>>>>>>>> of changes that gets harder and harder to merge? The next push point >>>>>>>>>> would probably be when AdminServlet was functional (if/when that >>>>>>>>>> happens) (and we could not push that class for now). >>>>>>>>>> >>>>>>>>>> If you read this far, thanks :) I wasn't sure how feasible this >>>>>>>>>> would be so I felt the need to prove it to my self in code before >>>>>>>>>> wasting your time, but please don't hesitate to point to costs I >>>>>>>>>> might be missing or anything that looks like a mistake, or say why >>>>>>>>>> this was a total waste of time :) >>>>>>>>>> >>>>>>>>>> Thoughts? >>>>>>>>>> >>>>>>>>>> -Gus >>>>>>>>>> -- >>>>>>>>>> http://www.needhamsoftware.com <http://www.needhamsoftware.com/> >>>>>>>>>> (work) >>>>>>>>>> http://www.the111shift.com <http://www.the111shift.com/> (play) >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> http://www.needhamsoftware.com <http://www.needhamsoftware.com/> (work) >>>>>>>> http://www.the111shift.com <http://www.the111shift.com/> (play) >>>>>>> >>>>>>> -- >>>>>>> - Mark >>>>>>> >>>>>>> http://about.me/markrmiller <http://about.me/markrmiller> >>>>>> >>>>>> >>>>>> -- >>>>>> http://www.needhamsoftware.com <http://www.needhamsoftware.com/> (work) >>>>>> http://www.the111shift.com <http://www.the111shift.com/> (play) >>>>> >>>>> >>>>> >>>>> -- >>>>> http://www.needhamsoftware.com <http://www.needhamsoftware.com/> (work) >>>>> http://www.the111shift.com <http://www.the111shift.com/> (play) >>>> >>>> >>>> >>>> -- >>>> http://www.needhamsoftware.com <http://www.needhamsoftware.com/> (work) >>>> http://www.the111shift.com <http://www.the111shift.com/> (play) >>> >>> >>> >>> -- >>> http://www.needhamsoftware.com <http://www.needhamsoftware.com/> (work) >>> http://www.the111shift.com <http://www.the111shift.com/> (play) >> >> >> >> -- >> http://www.needhamsoftware.com <http://www.needhamsoftware.com/> (work) >> http://www.the111shift.com <http://www.the111shift.com/> (play) > > -- > - Mark > > http://about.me/markrmiller <http://about.me/markrmiller>
