Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3
    • Component/s: search, update
    • Labels:
      None

      Description

      This patch adds JMX capability to get statistics from all the SolrInfoMBean.

      The implementation is done such a way to minimize code changes.
      In SolrInfoRegistry, I have overloaded Map's put and remove methods to register and unregister SolrInfoMBean in MBeanServer.

      Later on, I am planning to use register and unregister methods in SolrInfoRegistry and removing getRegistry() method (Hiding the map instance to other classes)

      1. SOLR-256.patch
        14 kB
        Shalin Shekhar Mangar
      2. SOLR-256.patch
        14 kB
        Shalin Shekhar Mangar
      3. SOLR-256.patch
        15 kB
        Shalin Shekhar Mangar
      4. SOLR-256.patch
        14 kB
        Shalin Shekhar Mangar
      5. SOLR-256.patch
        14 kB
        Shalin Shekhar Mangar
      6. SOLR-256.patch
        26 kB
        Shalin Shekhar Mangar
      7. SOLR-256.patch
        26 kB
        Shalin Shekhar Mangar
      8. SOLR-256.patch
        27 kB
        Shalin Shekhar Mangar
      9. SOLR-256.patch
        27 kB
        Shalin Shekhar Mangar
      10. SOLR-256.patch
        27 kB
        Shalin Shekhar Mangar
      11. jmx.patch
        7 kB
        Sharad Agarwal
      12. jmx.patch
        7 kB
        Sharad Agarwal
      13. jmx.patch
        7 kB
        Hoss Man
      14. jmx.patch
        7 kB
        Sharad Agarwal
      15. jmx.patch
        7 kB
        Hoss Man

        Activity

        Hide
        Sharad Agarwal added a comment -

        It uses Dynamic MBean to expose all SolrInfoMBean getStatistics() NameList keys as String getters.

        Show
        Sharad Agarwal added a comment - It uses Dynamic MBean to expose all SolrInfoMBean getStatistics() NameList keys as String getters.
        Hide
        Sharad Agarwal added a comment -

        -Corrected line wrap to 80 columns
        -Used the original LinkedHashMap (when jmx monitoring in not enabled)

        Show
        Sharad Agarwal added a comment - -Corrected line wrap to 80 columns -Used the original LinkedHashMap (when jmx monitoring in not enabled)
        Hide
        Hoss Man added a comment -

        update of previous patch that works against trunk (r550170)

        Show
        Hoss Man added a comment - update of previous patch that works against trunk (r550170)
        Hide
        Hoss Man added a comment -

        Sharad: JMX is one of those things i don't really understand very well, but i tried out the patch and played around with jconsole and it's really cool.

        one thing i'm not clear on (forgive my jmx ignorance) when trying to figure out how jconsole worked, i found some info saying that to enable JMX in a java app, you use "-Dcom.sun.management.jmxremote" option on the java command line ... doing that even without this patch allowed jconsole to "autodetect" that the "start.jar" process on my local machine supported JMX and gave me some interesting stats on things like memory and thread usage info from the jetty instance (including MBeans for the java.util.Logger's SOlr creates).

        With the patch, it doesn't seem to matter wether i use that commandline option or not i can only get access to the new solr specific stats if i tell jconsole to explicitly connect to the service:jmx:... url that shows up in the log. but in this case it can't get access to the other interesting stats from before (memory, thread, etc...) ... it seems like i can make two seperates "sessions" (forgive me if my terminology is wrong) to get this info, but not in the same session

        Is there a way to "register" the SOlr MBeans with the JVMs main JMX instance so all of hte info is available together?

        also: if we're going to have options for specifying the port for remove JMX connections, shoudl we also have options for the user/pass to connect with?

        Show
        Hoss Man added a comment - Sharad: JMX is one of those things i don't really understand very well, but i tried out the patch and played around with jconsole and it's really cool. one thing i'm not clear on (forgive my jmx ignorance) when trying to figure out how jconsole worked, i found some info saying that to enable JMX in a java app, you use "-Dcom.sun.management.jmxremote" option on the java command line ... doing that even without this patch allowed jconsole to "autodetect" that the "start.jar" process on my local machine supported JMX and gave me some interesting stats on things like memory and thread usage info from the jetty instance (including MBeans for the java.util.Logger's SOlr creates). With the patch, it doesn't seem to matter wether i use that commandline option or not i can only get access to the new solr specific stats if i tell jconsole to explicitly connect to the service:jmx:... url that shows up in the log. but in this case it can't get access to the other interesting stats from before (memory, thread, etc...) ... it seems like i can make two seperates "sessions" (forgive me if my terminology is wrong) to get this info, but not in the same session Is there a way to "register" the SOlr MBeans with the JVMs main JMX instance so all of hte info is available together? also: if we're going to have options for specifying the port for remove JMX connections, shoudl we also have options for the user/pass to connect with?
        Hide
        Sharad Agarwal added a comment -

        Hoss, thanks for having given a look at the patch.

        Via -Dcom.sun.management.jmxremote jvm "local" platform monitoring gets enabled; and you get to see all jvm statistics.

        This patch actually starts its own mbean registry (called MBeanServer).
        mbeanServer = MBeanServerFactory.createMBeanServer();
        java.lang.String serviceUrl = "service:jmx:rmi:///jndi/rmi://"
        + InetAddress.getLocalHost().getHostAddress().toString() + ":"
        + jmxPort + "/solr";
        JMXConnectorServer cs = JMXConnectorServerFactory
        .newJMXConnectorServer(new JMXServiceURL(serviceUrl), null,
        mbeanServer);
        cs.start();

        Instead of starting its own, other alternative (as you have pointed out) could be to use platform registry itself. In that case:
        MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();
        Register all solr mbean into platform mbeanserver itself. In this case all info would be available together.

        Remote monitoring and access control could be set up using appropriate com.sun.management.jmxremote.port, com.sun.management.jmxremote.password.file, com.sun.management.jmxremote.ssl options. I found http://java.sun.com/j2se/1.5.0/docs/guide/management/agent.html#properties pretty handy.

        I agree with you that having solr stats along with jvm stats makes more sense as it will require just one registry to manage.(setting up ports and access control). I can work on the patch to have that in place.

        Show
        Sharad Agarwal added a comment - Hoss, thanks for having given a look at the patch. Via -Dcom.sun.management.jmxremote jvm "local" platform monitoring gets enabled; and you get to see all jvm statistics. This patch actually starts its own mbean registry (called MBeanServer). mbeanServer = MBeanServerFactory.createMBeanServer(); java.lang.String serviceUrl = "service:jmx:rmi:///jndi/rmi://" + InetAddress.getLocalHost().getHostAddress().toString() + ":" + jmxPort + "/solr"; JMXConnectorServer cs = JMXConnectorServerFactory .newJMXConnectorServer(new JMXServiceURL(serviceUrl), null, mbeanServer); cs.start(); Instead of starting its own, other alternative (as you have pointed out) could be to use platform registry itself. In that case: MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); Register all solr mbean into platform mbeanserver itself. In this case all info would be available together. Remote monitoring and access control could be set up using appropriate com.sun.management.jmxremote.port, com.sun.management.jmxremote.password.file, com.sun.management.jmxremote.ssl options. I found http://java.sun.com/j2se/1.5.0/docs/guide/management/agent.html#properties pretty handy. I agree with you that having solr stats along with jvm stats makes more sense as it will require just one registry to manage.(setting up ports and access control). I can work on the patch to have that in place.
        Hide
        Hoss Man added a comment -

        > Instead of starting its own, other alternative (as you have pointed out) could be to use platform registry itself. In that > case:
        > MBeanServer mbs = ManagementFactory.getPlatformMBeanServer();

        that seems like the way to go.

        the one thing i'm not clear on is if there is simple programmaticly way to tell when JMX monitoring is already enabled so we can still do the check you have in the static block and only use the JMXified impls if we need them (i'm assuming you put that in because there is some overhead we want to avoid if JMX is not needed)

        the javadocs for ManagementFactory.getPlatformMBeanServer() indicate that it creates on first use ... so testing that won't work ... we could test for the system properties ourselves, but i think servlet containers like jetty and tomcat have their own config file syntax for enabling JMX and then call the neccessary underlying methods , so just becuase those properties aren't set doesn't neccessarily mean anything right?

        do you know if my guess about servlet containers programmaticly turning JMX on even without the standard system properties being set is true? do you have any suggestions on how to deal with this?

        Show
        Hoss Man added a comment - > Instead of starting its own, other alternative (as you have pointed out) could be to use platform registry itself. In that > case: > MBeanServer mbs = ManagementFactory.getPlatformMBeanServer(); that seems like the way to go. the one thing i'm not clear on is if there is simple programmaticly way to tell when JMX monitoring is already enabled so we can still do the check you have in the static block and only use the JMXified impls if we need them (i'm assuming you put that in because there is some overhead we want to avoid if JMX is not needed) the javadocs for ManagementFactory.getPlatformMBeanServer() indicate that it creates on first use ... so testing that won't work ... we could test for the system properties ourselves, but i think servlet containers like jetty and tomcat have their own config file syntax for enabling JMX and then call the neccessary underlying methods , so just becuase those properties aren't set doesn't neccessarily mean anything right? do you know if my guess about servlet containers programmaticly turning JMX on even without the standard system properties being set is true? do you have any suggestions on how to deal with this?
        Hide
        Sharad Agarwal added a comment -

        ArrayList list = MBeanServerFactory.findMBeanServer(null);
        gives all registered mbeanserver. It could be used to figure out programitically whether jmx is enabled or not.

        Show
        Sharad Agarwal added a comment - ArrayList list = MBeanServerFactory.findMBeanServer(null); gives all registered mbeanserver. It could be used to figure out programitically whether jmx is enabled or not.
        Hide
        Sharad Agarwal added a comment -

        Update of patch to work on standard JAVA_OPTS to enable JMX.

        Now standard JMX properties (refer http://java.sun.com/j2se/1.5.0/docs/guide/management/agent.html#properties) are used to configure JMX.

        To test the patch
        set -Dcom.sun.management.jmxremote in JAVA_OPTS environment variable
        then try using jconsole.

        Show
        Sharad Agarwal added a comment - Update of patch to work on standard JAVA_OPTS to enable JMX. Now standard JMX properties (refer http://java.sun.com/j2se/1.5.0/docs/guide/management/agent.html#properties ) are used to configure JMX. To test the patch set -Dcom.sun.management.jmxremote in JAVA_OPTS environment variable then try using jconsole.
        Hide
        Hoss Man added a comment -

        Sharad: my concern with your most recent patch is that if a servlet container uses it's own config to drive programmatic JMX Server creation (jetty plus appears to do this past on the example jetty-jmx.xml config file, but i haven't actually confirmed this), then Solr won't detect it because it's looking explicitly for the system properties.

        based on the javadocs your findMBeanServer(null) idea seems right on the money ... i'm attaching a tweak to your patch that uses this appraoch, and it seems to work great, what do you think?

        (was there a reason you decided to look for the properties explicitly instead of try this appraoch?)

        Any JMX experts want to chime in whether we should be doing something differently?

        Show
        Hoss Man added a comment - Sharad: my concern with your most recent patch is that if a servlet container uses it's own config to drive programmatic JMX Server creation (jetty plus appears to do this past on the example jetty-jmx.xml config file, but i haven't actually confirmed this), then Solr won't detect it because it's looking explicitly for the system properties. based on the javadocs your findMBeanServer(null) idea seems right on the money ... i'm attaching a tweak to your patch that uses this appraoch, and it seems to work great, what do you think? (was there a reason you decided to look for the properties explicitly instead of try this appraoch?) Any JMX experts want to chime in whether we should be doing something differently?
        Hide
        Sharad Agarwal added a comment -

        I earlier tried with findMBeanServer(null) with tomcat; and it always return me non-empty list of mbeanservers irrespective of JMX enabled or not. In tomcat it gets enabled via system properties.
        So to me it seems that having mbean server instance may not really confirm that JMX is enabled or not.

        Saying that, i think it may be OK to have the MonitoredMap even when JMX is not enabled. (dont think any overhead in using MonitoredMap); whenever JMX is enabled by either of the ways (standard system properties or via jetty config), both would work fine.

        Show
        Sharad Agarwal added a comment - I earlier tried with findMBeanServer(null) with tomcat; and it always return me non-empty list of mbeanservers irrespective of JMX enabled or not. In tomcat it gets enabled via system properties. So to me it seems that having mbean server instance may not really confirm that JMX is enabled or not. Saying that, i think it may be OK to have the MonitoredMap even when JMX is not enabled. (dont think any overhead in using MonitoredMap); whenever JMX is enabled by either of the ways (standard system properties or via jetty config), both would work fine.
        Hide
        Hoss Man added a comment -

        Hmmm... some quick poking around makes it seem that tomcat always uses MBeanServers to manage things – regardless of whether you explicitly turn on JMX monitoring or not (there seems to be an assumption that of course you want to monitor you server – i can't really argue with that)

        i don't really know enough about JMX to know if there really is any sort of overhead here – i suppose it wouldn't hurt to only use the MOnitoredMap if an explicit option is set (but we shouldn't startup our own MBeanServer like hte orriginal patch – just use the main one) but i'm also fine committing the patch as is.

        any tomcat/jmx experts want to chime in here?

        Show
        Hoss Man added a comment - Hmmm... some quick poking around makes it seem that tomcat always uses MBeanServers to manage things – regardless of whether you explicitly turn on JMX monitoring or not (there seems to be an assumption that of course you want to monitor you server – i can't really argue with that) i don't really know enough about JMX to know if there really is any sort of overhead here – i suppose it wouldn't hurt to only use the MOnitoredMap if an explicit option is set (but we shouldn't startup our own MBeanServer like hte orriginal patch – just use the main one) but i'm also fine committing the patch as is. any tomcat/jmx experts want to chime in here?
        Hide
        Sami Dalouche added a comment -

        Hoss: It would probably be interesting to look at how Spring Framework solved the problem.

        http://static.springframework.org/spring/docs/2.0.x/reference/jmx.html

        Spring JMX allows to export beans using JMX, and has an autodetection of the MBeanServer.. ("locateExistingServerIfPossible"). So, their code should probably either be linked, or reused to achieve similar effect...

        Regards,
        Sami Dalouche

        Show
        Sami Dalouche added a comment - Hoss: It would probably be interesting to look at how Spring Framework solved the problem. http://static.springframework.org/spring/docs/2.0.x/reference/jmx.html Spring JMX allows to export beans using JMX, and has an autodetection of the MBeanServer.. ("locateExistingServerIfPossible"). So, their code should probably either be linked, or reused to achieve similar effect... Regards, Sami Dalouche
        Hide
        Hoss Man added a comment -

        Sami: thanks for the pointer to the spring docs ... both because it's informative and because it made me realize this issue is still open, i thought i'd committed this a long time ago ... doh!

        Spring's approach to figuring out if there is an MBeanServer is basically the same as what we've been looking at doing: call MBeanServerFactory.findMBeanServer and see what we get ... they just put some more bells and whistles on it to warn you if more then one is found and you haven't configured the agentId of the one you want (which is a good idea)

        the general approach spring takes to JMX as a whole however is subtly different then what we've been talking about: so far we've discussed:

        • having Solr specific configs for creating a Solr specific MBeanServer and if those are set, then wrap all of our info registry stuff so they are MBean aware.
          ...OR...
        • having no Solr specific config options and wrapping the info registry stuff in MBeans if we detect an MBeanServer already running.

        Spring's appraoch:

        • have spring specific configs that specify things should be wraped in MBeans
        • have spring specific configs that specify if a spring specific MBeanServer should be created, and if not which existing MBeanServer to try and use by agentId.
        • if the first config option is set, but not the second, then register those MBeans with whatever MBeanServer you can find.

        ...this seems like the right way to go. people that don't want any JMX at all won't get it. people who want JMX if-and-only-if they've activate an MBeanServer via the servlet container can configure the MBean wrapping option. people that want Solr to do it all can configure solr with that info as well.

        basically it's a merge of both styles of patches so far.

        Unfortunately most of the existing patches are now unusable because of the Multi-core refactoring ... but i will try to work on a new patch (not sure when though, so if anyone else is gung ho don't let me stop you)

        Show
        Hoss Man added a comment - Sami: thanks for the pointer to the spring docs ... both because it's informative and because it made me realize this issue is still open, i thought i'd committed this a long time ago ... doh! Spring's approach to figuring out if there is an MBeanServer is basically the same as what we've been looking at doing: call MBeanServerFactory.findMBeanServer and see what we get ... they just put some more bells and whistles on it to warn you if more then one is found and you haven't configured the agentId of the one you want (which is a good idea) the general approach spring takes to JMX as a whole however is subtly different then what we've been talking about: so far we've discussed: having Solr specific configs for creating a Solr specific MBeanServer and if those are set, then wrap all of our info registry stuff so they are MBean aware. ...OR... having no Solr specific config options and wrapping the info registry stuff in MBeans if we detect an MBeanServer already running. Spring's appraoch: have spring specific configs that specify things should be wraped in MBeans have spring specific configs that specify if a spring specific MBeanServer should be created, and if not which existing MBeanServer to try and use by agentId. if the first config option is set, but not the second, then register those MBeans with whatever MBeanServer you can find. ...this seems like the right way to go. people that don't want any JMX at all won't get it. people who want JMX if-and-only-if they've activate an MBeanServer via the servlet container can configure the MBean wrapping option. people that want Solr to do it all can configure solr with that info as well. basically it's a merge of both styles of patches so far. Unfortunately most of the existing patches are now unusable because of the Multi-core refactoring ... but i will try to work on a new patch (not sure when though, so if anyone else is gung ho don't let me stop you)
        Hide
        Hoss Man added a comment -

        proposed solrconfig.xml syntax...

        <jmx />

        ...wraps the solr registry stuff in MBeans, adds them to the first MBeanSearver it finds (if any) but will not create one.

        <jmx agentId="foo" />

        ...wraps the solr registry stuff in MBeans, adds them to the first MBeanServer it finds matching that agentId, error if MBeanServer can't be found.

        <jmx serviceurl="service:jmx:rmi:///jndi/rmi://localhost:9999/solr" />

        ...wraps the solr registry stuff in MBeans, foracbly spins up a new MBeanServer exposed for remote monitoring at the specific service url, if the JMXConnectorServer can't be started (probably because the serviceurl is bad) then error.

        (we can punt on the issue of security – if users want Solr to manage their JMX remote debuging then it's wide open; if you want password files or ssl, then configure that with the JVM system properties for remote debugging and just tell solr "<jmx />")

        Show
        Hoss Man added a comment - proposed solrconfig.xml syntax... <jmx /> ...wraps the solr registry stuff in MBeans, adds them to the first MBeanSearver it finds (if any) but will not create one. <jmx agentId="foo" /> ...wraps the solr registry stuff in MBeans, adds them to the first MBeanServer it finds matching that agentId, error if MBeanServer can't be found. <jmx serviceurl="service:jmx:rmi:///jndi/rmi://localhost:9999/solr" /> ...wraps the solr registry stuff in MBeans, foracbly spins up a new MBeanServer exposed for remote monitoring at the specific service url, if the JMXConnectorServer can't be started (probably because the serviceurl is bad) then error. (we can punt on the issue of security – if users want Solr to manage their JMX remote debuging then it's wide open; if you want password files or ssl, then configure that with the JVM system properties for remote debugging and just tell solr "<jmx />")
        Hide
        Shalin Shekhar Mangar added a comment -

        A new patch which incorporates Hoss's proposed syntax. All three syntax such as jmx, jmx with agentId and jmx with serviceUrl are supported.

        SolrConfig is modified to load JMX configuration. A new class called JmxManager is introduced which intializes JMX support and registers all SolrInfoMBeans (using core.getInfoRegistry). JmxManager is created per core in the SolrCore constructor. It is destroyed in the SolrCore.close() method.

        Show
        Shalin Shekhar Mangar added a comment - A new patch which incorporates Hoss's proposed syntax. All three syntax such as jmx, jmx with agentId and jmx with serviceUrl are supported. SolrConfig is modified to load JMX configuration. A new class called JmxManager is introduced which intializes JMX support and registers all SolrInfoMBeans (using core.getInfoRegistry). JmxManager is created per core in the SolrCore constructor. It is destroyed in the SolrCore.close() method.
        Hide
        Shalin Shekhar Mangar added a comment -

        Fixed SOLR-256.patch to avoid NullPointerException during core shutdown when JMX is not enabled.

        Show
        Shalin Shekhar Mangar added a comment - Fixed SOLR-256 .patch to avoid NullPointerException during core shutdown when JMX is not enabled.
        Hide
        Shalin Shekhar Mangar added a comment - - edited

        Changes

        • Changed the type of the SolrInfoMBeans to the key specified in registry. Previously, I had used the SolrInfoMBean class's simple name which lead to duplicate registration of LRUCache (which is used as filterCache, queryResultCache and documentCache).
        • Fixed the parent name of the MBeans to solr (if using single cores). With multicores, the parent name is solr/core-name
        • Throws an exception if no server is found with the given agentId
        • Removed code I had added for debugging

        Also, I've added <jmx /> in the example solrconfig.xml to turn on JMX by default.

        Show
        Shalin Shekhar Mangar added a comment - - edited Changes Changed the type of the SolrInfoMBeans to the key specified in registry. Previously, I had used the SolrInfoMBean class's simple name which lead to duplicate registration of LRUCache (which is used as filterCache, queryResultCache and documentCache). Fixed the parent name of the MBeans to solr (if using single cores). With multicores, the parent name is solr/core-name Throws an exception if no server is found with the given agentId Removed code I had added for debugging Also, I've added <jmx /> in the example solrconfig.xml to turn on JMX by default.
        Hide
        Shalin Shekhar Mangar added a comment -

        No changes, just synchronizing with the changes in SolrCore made by SOLR-509 commit

        Show
        Shalin Shekhar Mangar added a comment - No changes, just synchronizing with the changes in SolrCore made by SOLR-509 commit
        Hide
        Shalin Shekhar Mangar added a comment -

        No changes. Just synchronizing the patch with the latest trunk code.

        Show
        Shalin Shekhar Mangar added a comment - No changes. Just synchronizing the patch with the latest trunk code.
        Hide
        Grant Ingersoll added a comment -

        Is it possible to write JUnit tests for this? I don't know much about JMX, but was thinking it still makes sense to have tests.

        If not, what is the best way to test?

        FWIW, the latest patch applies cleanly and all current tests pass.

        Show
        Grant Ingersoll added a comment - Is it possible to write JUnit tests for this? I don't know much about JMX, but was thinking it still makes sense to have tests. If not, what is the best way to test? FWIW, the latest patch applies cleanly and all current tests pass.
        Hide
        Marshall Weir added a comment -

        Tested the latest patch against the trunk. Almost everything works great. I get one warning:

        WARNING: Could not getStatistics on info bean org.apache.solr.handler.admin.AdminHandlers

        It seems to work fine, but I'm new to both Solr and JMX.

        Thanks for the updated patch Shalin
        Marshall

        Show
        Marshall Weir added a comment - Tested the latest patch against the trunk. Almost everything works great. I get one warning: WARNING: Could not getStatistics on info bean org.apache.solr.handler.admin.AdminHandlers It seems to work fine, but I'm new to both Solr and JMX. Thanks for the updated patch Shalin Marshall
        Hide
        Shalin Shekhar Mangar added a comment -

        Marshall - Thank you for trying the patch. I've noticed those warnings too. I'll take a look into it.

        Grant - Yes, I believe we can write JUnit tests for this (or at least a part of the functionality). I'll try to submit a new patch with tests by the end of this week.

        Show
        Shalin Shekhar Mangar added a comment - Marshall - Thank you for trying the patch. I've noticed those warnings too. I'll take a look into it. Grant - Yes, I believe we can write JUnit tests for this (or at least a part of the functionality). I'll try to submit a new patch with tests by the end of this week.
        Hide
        Hoss Man added a comment -

        skimming the patch, i think it's worth trying to get this in 1.3 assuming Shalin is able to ad some unit tests.

        Shalin: the three things that jumped out at me when reading the patch (unfortunately didn't have time to try testing it) is...

        • not sure why the separate init method is needed in the JmxManager class ... particularly since hte rest of hte constructor should be skipped if the init method doesn't set server ... seems simpler to just put lal of that in the constructor.
        • we should think about the situation when new "stuff" is dynamical registered after the SolrCore starts up ... SolrCore.registerRequestHandler for example can be called at any time. We may need to sprinkle some more hooks in SolrCore to call JmxManager.register (and to make JmxManager.register more resilient in case it gets called on an already register object, or when JMX is disabled anyway)
        • we should be careful about documenting what <jmx /> does ... i think in the example config you said it turns JMX monitoring on by default, but that's not true – it turns it on if and only if an existing MBeanServer is found ... we should be clear about that.
        Show
        Hoss Man added a comment - skimming the patch, i think it's worth trying to get this in 1.3 assuming Shalin is able to ad some unit tests. Shalin: the three things that jumped out at me when reading the patch (unfortunately didn't have time to try testing it) is... not sure why the separate init method is needed in the JmxManager class ... particularly since hte rest of hte constructor should be skipped if the init method doesn't set server ... seems simpler to just put lal of that in the constructor. we should think about the situation when new "stuff" is dynamical registered after the SolrCore starts up ... SolrCore.registerRequestHandler for example can be called at any time. We may need to sprinkle some more hooks in SolrCore to call JmxManager.register (and to make JmxManager.register more resilient in case it gets called on an already register object, or when JMX is disabled anyway) we should be careful about documenting what <jmx /> does ... i think in the example config you said it turns JMX monitoring on by default, but that's not true – it turns it on if and only if an existing MBeanServer is found ... we should be clear about that.
        Hide
        Shalin Shekhar Mangar added a comment -

        Hoss – I agree on all three points. I would like to see it in 1.3 too. I can take this up after I'm finished with SOLR-572. If someone else wants to work on this, please go ahead.

        Show
        Shalin Shekhar Mangar added a comment - Hoss – I agree on all three points. I would like to see it in 1.3 too. I can take this up after I'm finished with SOLR-572 . If someone else wants to work on this, please go ahead.
        Hide
        Shalin Shekhar Mangar added a comment -

        I'm taking a look again at this feature. Hope to have a patch ready soon.

        A few issues I found was that apart from SolrCore.registerRequestHandler, the SolrIndexSearcher is also dynamic and needs to be handled. I also found other issues like SolrIndexSearcher puts the cache objects into the InfoRegistry map but that they are never removed from the map. The SolrCore#closeSearcher calls:

        infoRegistry.remove("currentSearcher");

        but the SolrIndexSearcher adds/removes itself using its name. I think we should use "currentSearcher" itself as the key instead of name because the name changes after commits and that would make it very difficult to setup monitoring for SolrIndexSearcher using JMX.

        To handle dynamic registration and unregistration, I think we should use the approach that Sharad used in his original patch which was to create a class which extends Map and overrides the put and remove methods. That will make sure that SolrInfoMBeans are automatically registered and unregistered and will keep the JMX related code in one place.

        Show
        Shalin Shekhar Mangar added a comment - I'm taking a look again at this feature. Hope to have a patch ready soon. A few issues I found was that apart from SolrCore.registerRequestHandler, the SolrIndexSearcher is also dynamic and needs to be handled. I also found other issues like SolrIndexSearcher puts the cache objects into the InfoRegistry map but that they are never removed from the map. The SolrCore#closeSearcher calls: infoRegistry.remove( "currentSearcher" ); but the SolrIndexSearcher adds/removes itself using its name. I think we should use "currentSearcher" itself as the key instead of name because the name changes after commits and that would make it very difficult to setup monitoring for SolrIndexSearcher using JMX. To handle dynamic registration and unregistration, I think we should use the approach that Sharad used in his original patch which was to create a class which extends Map and overrides the put and remove methods. That will make sure that SolrInfoMBeans are automatically registered and unregistered and will keep the JMX related code in one place.
        Hide
        Shalin Shekhar Mangar added a comment - - edited

        Changes

        • Updated the documentation in example solrconfig.xml as per Hoss's suggestion
        • Renamed JmxManager to JmxMonitoredMap which extends a ConcurrentHashMap and overrides put, remove and clear methods
        • The JmxMonitoredMap is used to hold the infoRegistry (String->SolrInfoMBean) so that registration and unregistration is done automatically (which takes care of dynamic registration of request handlers, lazy loading, new SolrIndexSearchers etc.)
        • The SolrIndexSearcher is added with the name "searcher" to the map so that the name of the MBean doesn't change which helps setup monitoring
        • Instead of removing SolrIndexSearcher from the registry, it is simply replaced when SolrIndexSearcher#register method is called. This makes sure that the searcher registered with JMX is the current searcher (with a small margin of error)
        • Added two tests
          • TestJmxMonitoredMap passes a service url syntax to JmxMonitoredMap and uses a mocked SolrInfoMBean to test the put, remove and clear methods. Note that this test uses a hard-coded port number – unavailability of this port will fail the test.
          • TestJmxIntegration uses the AbstractSolrTestCase with the platform MBeanServer to test availability of Solr MBeans and tests for its updation by adding a document.
        Show
        Shalin Shekhar Mangar added a comment - - edited Changes Updated the documentation in example solrconfig.xml as per Hoss's suggestion Renamed JmxManager to JmxMonitoredMap which extends a ConcurrentHashMap and overrides put, remove and clear methods The JmxMonitoredMap is used to hold the infoRegistry (String->SolrInfoMBean) so that registration and unregistration is done automatically (which takes care of dynamic registration of request handlers, lazy loading, new SolrIndexSearchers etc.) The SolrIndexSearcher is added with the name "searcher" to the map so that the name of the MBean doesn't change which helps setup monitoring Instead of removing SolrIndexSearcher from the registry, it is simply replaced when SolrIndexSearcher#register method is called. This makes sure that the searcher registered with JMX is the current searcher (with a small margin of error) Added two tests TestJmxMonitoredMap passes a service url syntax to JmxMonitoredMap and uses a mocked SolrInfoMBean to test the put, remove and clear methods. Note that this test uses a hard-coded port number – unavailability of this port will fail the test. TestJmxIntegration uses the AbstractSolrTestCase with the platform MBeanServer to test availability of Solr MBeans and tests for its updation by adding a document.
        Hide
        Shalin Shekhar Mangar added a comment -

        Syncing the patch with trunk. No other changes in the code base.

        I've created a wiki page to document the configuration, functionality and usage of this feature at http://wiki.apache.org/solr/SolrJmx (work in progress)

        Show
        Shalin Shekhar Mangar added a comment - Syncing the patch with trunk. No other changes in the code base. I've created a wiki page to document the configuration, functionality and usage of this feature at http://wiki.apache.org/solr/SolrJmx (work in progress)
        Hide
        Shalin Shekhar Mangar added a comment -

        Syncing the patch with the trunk code.

        It seems that the Query.instanceOf method that I was using in TestJmxIntegration was added only in Java 6. Strangely, eclipse did not complain about it even though the compiler in eclipse was set to 5.0. The test has been changed and uses an alternate 5.0 compatible way.

        Show
        Shalin Shekhar Mangar added a comment - Syncing the patch with the trunk code. It seems that the Query.instanceOf method that I was using in TestJmxIntegration was added only in Java 6. Strangely, eclipse did not complain about it even though the compiler in eclipse was set to 5.0. The test has been changed and uses an alternate 5.0 compatible way.
        Hide
        Shalin Shekhar Mangar added a comment -

        Syncronizing with the trunk code. No other changes.

        Hoss – Is there anything else we need to do to commit this?

        Show
        Shalin Shekhar Mangar added a comment - Syncronizing with the trunk code. No other changes. Hoss – Is there anything else we need to do to commit this?
        Hide
        Hoss Man added a comment -

        Shalin:

        I'm really sorry, I'm way behind schedule on my "patch review" responsibilities.

        skimming the patch, I see no red flags.

        Some minor misc nitpicks...

        • I'd prefer if we removed the hard coded port number in TestJmxMonitoredMap (hard coded stuff like that has caused us nothing but problems in the past). If i'm understanding the JMXServiceURL javadocs correctly, let's hardcode port=0 for the url constructed by the test ... then add a getJMXServiceURL() to JmxMonitoredMap that the test can then call and pass to to the JMXConnectorFactory ... that should give us either a random port that isn't in use by anyone else, right?
        • it seems like it would be a little cleaner if SolrIndexSearcher.register() registered itself before it registered it's subcomponents .. not sure that it really matters, but it certain reads a little weird.
        • SolrIndexSearcher.register() is logging '"Registering new searcher: " + System.currentTimeMillis()' for every cache it registers ... that seems like a cut/paste mistake.
        • is there any reason not to have the searcher's add/remove themselves using their true name on register()/close() and have register() call put("searcher", this) like you have it now? ... that way you'd get the benefits you mentioned before (continuous monitoring of the current searcher) but you could also get information about how many "live" searchers there currently are, and what their stats look like (so you could, for example, notice when there is a really old Searcher hanging around for some inexplicable reason, probably a bug.)
        • couldn't we completely eliminate any overhead of JMX for people who haven't enabled it by adding an "isEnabled()" method to JmxMonitoredMap that returns true if server!=null and then make the SolrCore changes look like...
               //Initialize Registry w/JMX if enabled
               JmxMonitoredMap<String,SolrInfoMBean> tmp = new JmxMonitoredMap<String,SolrInfoMBean>(name, config.jmxConfig);
               infoRegistry = (tmp.isEnabled() ? tmp : new HashMap<String,SolrInfoMBean>() );
          
        Show
        Hoss Man added a comment - Shalin: I'm really sorry, I'm way behind schedule on my "patch review" responsibilities. skimming the patch, I see no red flags. Some minor misc nitpicks... I'd prefer if we removed the hard coded port number in TestJmxMonitoredMap (hard coded stuff like that has caused us nothing but problems in the past). If i'm understanding the JMXServiceURL javadocs correctly, let's hardcode port=0 for the url constructed by the test ... then add a getJMXServiceURL() to JmxMonitoredMap that the test can then call and pass to to the JMXConnectorFactory ... that should give us either a random port that isn't in use by anyone else, right? it seems like it would be a little cleaner if SolrIndexSearcher.register() registered itself before it registered it's subcomponents .. not sure that it really matters, but it certain reads a little weird. SolrIndexSearcher.register() is logging '"Registering new searcher: " + System.currentTimeMillis()' for every cache it registers ... that seems like a cut/paste mistake. is there any reason not to have the searcher's add/remove themselves using their true name on register()/close() and have register() call put("searcher", this) like you have it now? ... that way you'd get the benefits you mentioned before (continuous monitoring of the current searcher) but you could also get information about how many "live" searchers there currently are, and what their stats look like (so you could, for example, notice when there is a really old Searcher hanging around for some inexplicable reason, probably a bug.) couldn't we completely eliminate any overhead of JMX for people who haven't enabled it by adding an "isEnabled()" method to JmxMonitoredMap that returns true if server!=null and then make the SolrCore changes look like... //Initialize Registry w/JMX if enabled JmxMonitoredMap< String ,SolrInfoMBean> tmp = new JmxMonitoredMap< String ,SolrInfoMBean>(name, config.jmxConfig); infoRegistry = (tmp.isEnabled() ? tmp : new HashMap< String ,SolrInfoMBean>() );
        Hide
        Hoss Man added a comment -

        Shalin's been doing all the work on this puppy, no reason for it to be assigned to me anymore.

        Commit whenever you're ready dude.

        Show
        Hoss Man added a comment - Shalin's been doing all the work on this puppy, no reason for it to be assigned to me anymore. Commit whenever you're ready dude.
        Hide
        Shalin Shekhar Mangar added a comment -

        Hoss, thanks for the comments.

        • Specifying port=0 to ServiceUrl does not work, it blindly tries to connect to localhost on port 0 and fails with a java.rmi.ConnectException. We can try to do something like this:
          ServerSocket server = new ServerSocket(0);
          int port = server.getLocalPort();
          server.close();
          // Use the port
          

          However it will not be reliable on every environment. I can put this into a loop and retry a fixed number of times before failing. That should be good enough.

        • Agreed
        • Copy/paste error
        • That's a very good idea.
        • Not sure if it really matters but that's very easy to do.

        I shall give a patch shortly.

        Show
        Shalin Shekhar Mangar added a comment - Hoss, thanks for the comments. Specifying port=0 to ServiceUrl does not work, it blindly tries to connect to localhost on port 0 and fails with a java.rmi.ConnectException. We can try to do something like this: ServerSocket server = new ServerSocket(0); int port = server.getLocalPort(); server.close(); // Use the port However it will not be reliable on every environment. I can put this into a loop and retry a fixed number of times before failing. That should be good enough. Agreed Copy/paste error That's a very good idea. Not sure if it really matters but that's very easy to do. I shall give a patch shortly.
        Hide
        Shalin Shekhar Mangar added a comment - - edited

        Incorporates Hoss's suggestions as per comment above.

        Another change is that the UpdateHandler constructor adds itself to the infoRegistry. This leads to an NullPointerException in DirectUpdateHandler2#getStatistics because when JmxMonitoredMap tries to read the statistics, the instance variables of DUH2 haven't been initialized yet. The call to infoRegistry.put has now been moved to SolrCore constructor right after it creates the UpdateHandler.

        I shall commit this shortly.

        Show
        Shalin Shekhar Mangar added a comment - - edited Incorporates Hoss's suggestions as per comment above. Another change is that the UpdateHandler constructor adds itself to the infoRegistry. This leads to an NullPointerException in DirectUpdateHandler2#getStatistics because when JmxMonitoredMap tries to read the statistics, the instance variables of DUH2 haven't been initialized yet. The call to infoRegistry.put has now been moved to SolrCore constructor right after it creates the UpdateHandler. I shall commit this shortly.
        Hide
        Shalin Shekhar Mangar added a comment -

        Committed revision 680795.

        Show
        Shalin Shekhar Mangar added a comment - Committed revision 680795.

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Sharad Agarwal
          • Votes:
            6 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development