[jira] Commented: (SOLR-256) Stats via JMX

["Hoss Man (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SOLR-256?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12617632#action_12617632 ] 

Hoss Man commented on SOLR-256:
-------------------------------

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...
{code}
     //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>() );
{code}





> Stats via JMX
> -------------
>
>                 Key: SOLR-256
>                 URL: https://issues.apache.org/jira/browse/SOLR-256
>             Project: Solr
>          Issue Type: New Feature
>          Components: search, update
>            Reporter: Sharad Agarwal
>            Assignee: Hoss Man
>            Priority: Minor
>             Fix For: 1.3
>
>         Attachments: jmx.patch, jmx.patch, jmx.patch, jmx.patch, jmx.patch, SOLR-256.patch, SOLR-256.patch, SOLR-256.patch, SOLR-256.patch, SOLR-256.patch, SOLR-256.patch, SOLR-256.patch, SOLR-256.patch, SOLR-256.patch
>
>
> 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)

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.