Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.5, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      Currently, you have to specify a core to get system information for a variety of request handlers - properties, logging, thread dump, system, etc.

      These should be available at a system location and not core specific location.

      1. SOLR-4943.patch
        15 kB
        Mark Miller
      2. SOLR-4943.patch
        14 kB
        Mark Miller
      3. SOLR-4943.patch
        9 kB
        Mark Miller
      4. SOLR-4943-2.patch
        14 kB
        Mark Miller
      5. SOLR-4943-3__hoss_variant.patch
        18 kB
        Hoss Man
      6. SOLR-4943-3.patch
        15 kB
        Mark Miller

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          I've related this to SOLR-4220 - and perhaps it ends up being a dupe - I'd like to keep this separate for the moment though - that issue has some ambitious ideas in it. This issue has a working hack patch now.

          Show
          Mark Miller added a comment - I've related this to SOLR-4220 - and perhaps it ends up being a dupe - I'd like to keep this separate for the moment though - that issue has some ambitious ideas in it. This issue has a working hack patch now.
          Hide
          Mark Miller added a comment -

          This patch adds a new set of non core specific urls you can hit:

          localhost:8983/solr/admin/info/properties
          localhost:8983/solr/admin/info/threads
          localhost:8983/solr/admin/info/system
          localhost:8983/solr/admin/info/logging

          It's a quick hack first patch.

          Show
          Mark Miller added a comment - This patch adds a new set of non core specific urls you can hit: localhost:8983/solr/admin/info/properties localhost:8983/solr/admin/info/threads localhost:8983/solr/admin/info/system localhost:8983/solr/admin/info/logging It's a quick hack first patch.
          Hide
          Mark Miller added a comment -

          This patch incorporates a hack for SOLR-3633.

          You can start up the admin UI with no cores and most of it works.

          I'm hoping @steffkes can help me clean it up a bit. It's really just a quick hack to make things work against the new admin info handlers.

          Show
          Mark Miller added a comment - This patch incorporates a hack for SOLR-3633 . You can start up the admin UI with no cores and most of it works. I'm hoping @steffkes can help me clean it up a bit. It's really just a quick hack to make things work against the new admin info handlers.
          Hide
          Mark Miller added a comment -

          New Patch:

          • fixes logging admin info handler - it expected a core and needed support to be init'd with a corecontainer
          Show
          Mark Miller added a comment - New Patch: fixes logging admin info handler - it expected a core and needed support to be init'd with a corecontainer
          Hide
          Alan Woodward added a comment -

          Can we remove the constructors that don't take CoreContainer objects? Particularly for the InfoHandler, which won't work at all without one.

          Show
          Alan Woodward added a comment - Can we remove the constructors that don't take CoreContainer objects? Particularly for the InfoHandler, which won't work at all without one.
          Hide
          Mark Miller added a comment -

          Can we remove the constructors that don't take CoreContainer objects?

          No, this issue doesn't move them, so they are all available under a single core as well - I think if we want to deprecate/remove those, it can be another issue - I don't have an issue with still offering them though - especially through 4X, but even in 5X is fine by me.

          Particularly for the InfoHandler,

          Yeah, that's just copy past from the CoreAdminHandler - I'd have to look at why it has both constructors, but it seems likely we can just remove it from the InfoHandler.

          Show
          Mark Miller added a comment - Can we remove the constructors that don't take CoreContainer objects? No, this issue doesn't move them, so they are all available under a single core as well - I think if we want to deprecate/remove those, it can be another issue - I don't have an issue with still offering them though - especially through 4X, but even in 5X is fine by me. Particularly for the InfoHandler, Yeah, that's just copy past from the CoreAdminHandler - I'd have to look at why it has both constructors, but it seems likely we can just remove it from the InfoHandler.
          Hide
          Alan Woodward added a comment -

          Sorry, I didn't mean remove the URLs with the cores on them, I meant the constructor functions. The URLs are all intercepted by SolrDispatchFilter and passed to the handlers on CoreContainer anyway, and that's fine.

          So for example InfoHandler has a no-argument constructor, that sets this.cc to null. If you use this constructor, you can never get useful information out of the InfoHandler - it will always throw a SolrException. So it seems a bit pointless to have this constructor at all.

          Also, some of these probably don't need to take CoreContainers at all. SystemInfoHandler just uses it to determine if it's Zk-aware or not, which it could do as easily with a boolean passed to its constructor. LoggingHandler could take a LogWatcher rather than a CoreContainer. Makes it easier to test things, for a start.

          OIC, looking at it more closely, they need the no-arg constructors because they could be defined in solrconfig.xml as request handlers, and need to go through the normal construct/init/inform lifecycle. So yes, we should probably open another issue to deprecate defining the handlers that are container-wide in individual core configs.

          Show
          Alan Woodward added a comment - Sorry, I didn't mean remove the URLs with the cores on them, I meant the constructor functions. The URLs are all intercepted by SolrDispatchFilter and passed to the handlers on CoreContainer anyway, and that's fine. So for example InfoHandler has a no-argument constructor, that sets this.cc to null. If you use this constructor, you can never get useful information out of the InfoHandler - it will always throw a SolrException. So it seems a bit pointless to have this constructor at all. Also, some of these probably don't need to take CoreContainers at all. SystemInfoHandler just uses it to determine if it's Zk-aware or not, which it could do as easily with a boolean passed to its constructor. LoggingHandler could take a LogWatcher rather than a CoreContainer. Makes it easier to test things, for a start. OIC, looking at it more closely, they need the no-arg constructors because they could be defined in solrconfig.xml as request handlers, and need to go through the normal construct/init/inform lifecycle. So yes, we should probably open another issue to deprecate defining the handlers that are container-wide in individual core configs.
          Hide
          Mark Miller added a comment -

          Sorry, I didn't mean remove the URLs with the cores on them, I meant the constructor functions.

          Yes, I know, but the two are very related

          So for example InfoHandler has a no-argument constructor, that sets this.cc to null.

          Right, like I said, that's a copy paste from the CoreAdmin class - it can likely go.

          because they could be defined in solrconfig.xml as request handlers, and need to go through the normal construct/init/inform lifecycle.

          Exactly. This issue is not about removing the per core admin info - it's just about adding that info at the system level so that the UI can be reasonable when no SolrCores are defined.

          Show
          Mark Miller added a comment - Sorry, I didn't mean remove the URLs with the cores on them, I meant the constructor functions. Yes, I know, but the two are very related So for example InfoHandler has a no-argument constructor, that sets this.cc to null. Right, like I said, that's a copy paste from the CoreAdmin class - it can likely go. because they could be defined in solrconfig.xml as request handlers, and need to go through the normal construct/init/inform lifecycle. Exactly. This issue is not about removing the per core admin info - it's just about adding that info at the system level so that the UI can be reasonable when no SolrCores are defined.
          Hide
          Hoss Man added a comment -

          I'm -0 on adding this new special "/admin/info" path ... CoreAdminHandler's STATUS call seemed like a good place to return all of this info logic form, and had the added benefit of already being a "special" handler with special semantics and a solr.xml configurable path – hence the idea i floated in SOLR-4220.

          (I know that between the collection admin paths and the restlet REST API paths we've already moved into the point of there being "special" paths that the user can't change in configuration – i'd just really like to avoid adding any more ... but I'm not going to fight hard for it.)

          Other then that, my concerns when looking at the patch...

          • Having InfoHandler explicitly wrap instances of the existing ThreadDumpHandler, PropertiesRequestHandler, LoggingHandler, and SystemInfoHandler scares me (because of the "is CoreContainer null? is SolrCore null?" situation) and seems overly complicated ... wouldn't it be just as easy to refactor the meat of these classes into static methods (or helper objects) that don't know about SolrQueryRequest and take in only hte specific item they need (CoreContainer in SystemInfoHandler's case, and LowWatcher in LoggingHandler's case, etc..) and then both InfoHandler and the original handlers could call/instantiate those helpers?
          • either way, we need some tests of this new InfoHandler (otherwise i guarantee you someone down the road is going to add some code to one of these wrapped handlers that is going to cause an NPE because it assumes it has access to a SolrCore and it won't be caught until someone notices it in the UI)
            • we also need more javadocs on all of the affected handlers noting the two differnet usage patterns.
          • why is there a change to SolrIndexSearcher in this patch? ... it smells fishy ... is this a distinct bug that was discovered as part of your work on hte patch that should be fixed & release noted in a dedicated issue?
          Show
          Hoss Man added a comment - I'm -0 on adding this new special "/admin/info" path ... CoreAdminHandler's STATUS call seemed like a good place to return all of this info logic form, and had the added benefit of already being a "special" handler with special semantics and a solr.xml configurable path – hence the idea i floated in SOLR-4220 . (I know that between the collection admin paths and the restlet REST API paths we've already moved into the point of there being "special" paths that the user can't change in configuration – i'd just really like to avoid adding any more ... but I'm not going to fight hard for it.) Other then that, my concerns when looking at the patch... Having InfoHandler explicitly wrap instances of the existing ThreadDumpHandler, PropertiesRequestHandler, LoggingHandler, and SystemInfoHandler scares me (because of the "is CoreContainer null? is SolrCore null?" situation) and seems overly complicated ... wouldn't it be just as easy to refactor the meat of these classes into static methods (or helper objects) that don't know about SolrQueryRequest and take in only hte specific item they need (CoreContainer in SystemInfoHandler's case, and LowWatcher in LoggingHandler's case, etc..) and then both InfoHandler and the original handlers could call/instantiate those helpers? either way, we need some tests of this new InfoHandler (otherwise i guarantee you someone down the road is going to add some code to one of these wrapped handlers that is going to cause an NPE because it assumes it has access to a SolrCore and it won't be caught until someone notices it in the UI) we also need more javadocs on all of the affected handlers noting the two differnet usage patterns. why is there a change to SolrIndexSearcher in this patch? ... it smells fishy ... is this a distinct bug that was discovered as part of your work on hte patch that should be fixed & release noted in a dedicated issue?
          Hide
          Mark Miller added a comment -

          there being "special" paths that the user can't change in configuration – i'd just really like to avoid adding any more

          I much prefer this - being able to count on these types of urls is very useful! I hate that the core admin handler url is configurable and in fact it breaks solrcloud if you configure it.

          CoreAdminHandler's STATUS call seemed like a good place to return

          Personally, I don't like the idea of jamming these into the status call. I think they have a good structure now - they just should not be at the core level, but at the system level.

          because of the "is CoreContainer null? is SolrCore null?" situation) and seems overly complicated

          I think it's fairly simple atm - if it's bothersome, I'd prefer we just remove them at the core level for 5.0 rather than try and refactor it to be very slightly simpler code.

          either way, we need some tests

          Yes of course we need tests.

          why is there a change to SolrIndexSearcher in this patch?

          It's just an unrelated thing that got caught up in the patch.

          Show
          Mark Miller added a comment - there being "special" paths that the user can't change in configuration – i'd just really like to avoid adding any more I much prefer this - being able to count on these types of urls is very useful! I hate that the core admin handler url is configurable and in fact it breaks solrcloud if you configure it. CoreAdminHandler's STATUS call seemed like a good place to return Personally, I don't like the idea of jamming these into the status call. I think they have a good structure now - they just should not be at the core level, but at the system level. because of the "is CoreContainer null? is SolrCore null?" situation) and seems overly complicated I think it's fairly simple atm - if it's bothersome, I'd prefer we just remove them at the core level for 5.0 rather than try and refactor it to be very slightly simpler code. either way, we need some tests Yes of course we need tests. why is there a change to SolrIndexSearcher in this patch? It's just an unrelated thing that got caught up in the patch.
          Hide
          Hoss Man added a comment -

          I much prefer this ...

          I know, agree to disagree

          I think it's fairly simple atm

          Complex probably wasn't the right word .. the code is simple, but it left the door open very side for someone to introduce bugs in the future for one code path or the other.

          Take a look at this SOLR-4943-3__hoss_variant.patch i'm attaching – to me this is just as simple and easy to understand as what you had, but makes it a lot harder for someone down the road to break it w/o getting an obvious compile failure because the static methods limit what the code has access to (and if they change the signature of hte static method, then they'll get a compile error in InfoHandler and realize why the code is written the way it is.

          if it's bothersome, I'd prefer we just remove them at the core level for 5.0 rather than try and refactor it to be very slightly simpler code.

          +1 ... starting with my SOLR-4943-3__hoss_variant.patch it would be trivial to move those static methods into InfoHandler ... they could still be called from the existing handlers, but those handlers would be marked deprecated.

          Show
          Hoss Man added a comment - I much prefer this ... I know, agree to disagree I think it's fairly simple atm Complex probably wasn't the right word .. the code is simple, but it left the door open very side for someone to introduce bugs in the future for one code path or the other. Take a look at this SOLR-4943 -3__hoss_variant.patch i'm attaching – to me this is just as simple and easy to understand as what you had, but makes it a lot harder for someone down the road to break it w/o getting an obvious compile failure because the static methods limit what the code has access to (and if they change the signature of hte static method, then they'll get a compile error in InfoHandler and realize why the code is written the way it is. if it's bothersome, I'd prefer we just remove them at the core level for 5.0 rather than try and refactor it to be very slightly simpler code. +1 ... starting with my SOLR-4943 -3__hoss_variant.patch it would be trivial to move those static methods into InfoHandler ... they could still be called from the existing handlers, but those handlers would be marked deprecated.
          Hide
          Mark Miller added a comment -

          Take a look at this

          That's fine with me. I'll incorporate it into my patch.

          Show
          Mark Miller added a comment - Take a look at this That's fine with me. I'll incorporate it into my patch.
          Hide
          Mark Miller added a comment -

          Hmm, on a quick look, it looks like your missing the reason things are as they are...you cannot get the core container from the request core in this case - there is no core. Your patch will just cause NPE's when you hit the system wide urls.

          Show
          Mark Miller added a comment - Hmm, on a quick look, it looks like your missing the reason things are as they are...you cannot get the core container from the request core in this case - there is no core. Your patch will just cause NPE's when you hit the system wide urls.
          Hide
          Mark Miller added a comment -

          New patch with some testing. Takes out the UI changes - @steffkes has said he will flesh out that part of the patch and commit it after I put in the back end changes.

          Show
          Mark Miller added a comment - New patch with some testing. Takes out the UI changes - @steffkes has said he will flesh out that part of the patch and commit it after I put in the back end changes.
          Hide
          Mark Miller added a comment -

          Adds another test to hit the SolrDispatchFilter path.

          Show
          Mark Miller added a comment - Adds another test to hit the SolrDispatchFilter path.
          Hide
          Mark Miller added a comment -

          Okay, I'm about ready to put this in so that the UI half can be finished...

          Show
          Mark Miller added a comment - Okay, I'm about ready to put this in so that the UI half can be finished...
          Hide
          ASF subversion and git services added a comment -

          Commit 1501898 from Mark Miller
          [ https://svn.apache.org/r1501898 ]

          SOLR-4943: Add a new system wide info admin handler that exposes the system info that could previously only be retrieved using a SolrCore.

          Show
          ASF subversion and git services added a comment - Commit 1501898 from Mark Miller [ https://svn.apache.org/r1501898 ] SOLR-4943 : Add a new system wide info admin handler that exposes the system info that could previously only be retrieved using a SolrCore.
          Hide
          ASF subversion and git services added a comment -

          Commit 1501899 from Mark Miller
          [ https://svn.apache.org/r1501899 ]

          SOLR-4943: Add a new system wide info admin handler that exposes the system info that could previously only be retrieved using a SolrCore.

          Show
          ASF subversion and git services added a comment - Commit 1501899 from Mark Miller [ https://svn.apache.org/r1501899 ] SOLR-4943 : Add a new system wide info admin handler that exposes the system info that could previously only be retrieved using a SolrCore.
          Hide
          Mark Miller added a comment -

          Committed - not in 4.4 so we have time to tweak it if necessary.

          Show
          Mark Miller added a comment - Committed - not in 4.4 so we have time to tweak it if necessary.
          Hide
          Jack Krupansky added a comment -

          I see that this issue is marked as "Resolved", but... I don't see either a wiki page or a new refguide page, even a simple placeholder, or any other hint of documentation to come for this new feature.

          Show
          Jack Krupansky added a comment - I see that this issue is marked as "Resolved", but... I don't see either a wiki page or a new refguide page, even a simple placeholder, or any other hint of documentation to come for this new feature.
          Hide
          Adrien Grand added a comment -

          4.5 release -> bulk close

          Show
          Adrien Grand added a comment - 4.5 release -> bulk close

            People

            • Assignee:
              Mark Miller
              Reporter:
              Mark Miller
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development