[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16925756#comment-16925756 ] ASF subversion and git services commented on SOLR-13677: Commit b1bccf7cace424cb895ca6d05b30926697bfe86b in lucene-solr's branch refs/heads/branch_8x from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=b1bccf7 ] SOLR-13677: reverting the last commit > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 50m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16925658#comment-16925658 ] ASF subversion and git services commented on SOLR-13677: Commit a288710a64acdde6abc8ce96a0d3b3e18739ac32 in lucene-solr's branch refs/heads/master from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=a288710 ] SOLR-13677: reverting the last commit (#863) > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 50m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16925657#comment-16925657 ] ASF subversion and git services commented on SOLR-13677: Commit 042478cfa795dd537dcd4863a0524a73bad9a740 in lucene-solr's branch refs/heads/master from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=042478c ] SOLR-13677: reverting the last commit > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 50m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16925083#comment-16925083 ] Noble Paul commented on SOLR-13677: --- As planned, I have committed the revert to [SOLR-13677_3|https://github.com/apache/lucene-solr/tree/jira/SOLR-13677_3]. I'll give it a few hours. run some testing and merge this to master / branch_8x > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16925082#comment-16925082 ] ASF subversion and git services commented on SOLR-13677: Commit 042478cfa795dd537dcd4863a0524a73bad9a740 in lucene-solr's branch refs/heads/jira/SOLR-13677_3 from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=042478c ] SOLR-13677: reverting the last commit > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16925033#comment-16925033 ] Noble Paul commented on SOLR-13677: --- Yes, I'm planning to do this ASAP. Why should I be the only one bothered about a memory leak > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Assignee: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16924905#comment-16924905 ] Ishan Chattopadhyaya commented on SOLR-13677: - [~noble.paul], [~ab] Instead of this multi-day revert plan (which meet seem confusing to understand), I recommend an immediate revert in master and branch_8x. > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Assignee: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16924738#comment-16924738 ] Noble Paul commented on SOLR-13677: --- [~ab] My plan of action * I'll revert *all* the changes I have made as a part of this issue in the branch [SOLR-13677_3|https://github.com/apache/lucene-solr/tree/jira/SOLR-13677_3]. * This will also mean I'll "Ignore" the tests which are failing due to this * I'll leave it there for a couple of days for you to take any further action * After that I'll merge those changes back into master & branch_8x > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Assignee: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16924616#comment-16924616 ] David Smiley commented on SOLR-13677: - +Yes, lets revert this now then+. I appreciate that [~ab] is setting a good bar for quality software! > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Assignee: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16924391#comment-16924391 ] Mark Miller commented on SOLR-13677: Reverts should take place right away, else we can easily end up in a bad situation. Development should be worked out on the branch, not in our release branches. Please let's do a simple and fast revert and then commit when we have consensus. > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Assignee: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16922453#comment-16922453 ] Andrzej Bialecki commented on SOLR-13677: -- I marked this as blocker for 8.3 to make sure the changes already committed are fixed before the release. > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Assignee: Noble Paul >Priority: Blocker > Fix For: 8.3 > > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16918563#comment-16918563 ] Noble Paul commented on SOLR-13677: --- let's make the changes/reverts in [https://github.com/apache/lucene-solr/tree/jira/SOLR-13677_3]. > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Assignee: Noble Paul >Priority: Major > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16918546#comment-16918546 ] Andrzej Bialecki commented on SOLR-13677: -- [~noble.paul] - please revert these changes until the above problems are addressed. I'll be happy to help you fix them - let's create a branch. > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Assignee: Noble Paul >Priority: Major > Time Spent: 3h 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian Jira (v8.3.2#803003) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16916695#comment-16916695
]
Andrzej Bialecki commented on SOLR-13677:
--
There were several issues that we pointed out in your patch before the last
refactoring - and yet after the last refactoring you didn't give anyone a
chance to review your changes before committing.
I object to this on principle - knowing that there are issues to be resolved
you should've waited a reasonable time for review, and 5 hours is not
reasonable, especially during summer holidays. On this grounds alone I'd like
to request that you revert these commits.
However, in addition to that there are still several issues remaining in the
committed changes that need to be fixed - and it would be just easier and
cleaner to do this by reverting, fixing and committing them again after review:
* {{SolrMetrics}} is a misleading name - it implies more functionality than it
really provides, which is just a dumb context to keep several references
together. Previous patch used {{MetricsInfo}} which was not ideal but better
that this. I propose {{SolrMetricsContext}}.
* Also, the class and its methods have 0 javadocs - I hope that one day this
will actually become a precommit error.
* looking at usages of this class it seems to me that perhaps it should not
include {{scope}} - take for example
{{SolrCoreMetricManager.registerMetricProducer}} (which hasn't been converted
yet to the new API!), or any other use of the old
{{SolrMetricProducer.initializeMetrics}}: it would be so much easier to create
just one {{SolrMetrics}} object and pass it to each producer, regardless of its
scope.
* I don't understand the need to keep a reference {{SolrMetrics.parent}}. This
is used only in one place in
{{MetricsHandlerTest.RefreshablePluginHolder.closeHandler()}}, which looks like
a hack and has no way of working anyway (comparing hex hashCode to plain int
hashCode).
* {{GaugeWrapper.unregisterGauges}}: expression
{{wrapper.getTag().contains(tagSegment)}} already includes
{{tagSegment.equals(wrapper.getTag())}}.
* I don't understand the conditional in {{SolrMetricProducer.close()}} -
basically it means that you unregister on {{close}} only non-root components
and all their children. Why exclude root components?
* {{SolrMetrics.gauge}} really doesn't need to mess with explicitly creating a
{{GaugeWrapper}}, and the new (undocumented) method in {{SolrMetricManager}}
that you added is dangerous - because it exposes the ability to easily register
unwrapped gauges. Instead {{SolrMetric.gauge}} should simply delegate to
{{SolrMetricManager.registerGauge}}. Also, providing a null metricName is an
error and should throw an exception - is there any situation where you would
want that?
* I don't see any need for the newly added methods in {{SolrMetricManager}} -
existing methods were sufficient for implementing the functionality in
{{SolrMetrics}}. Please remove them, they only increase the size of the API
without adding any benefit.
* {{SolrMetricManager.unregisterGauges(String registryName, String
tagSegment)}} should have some javadoc now, because it's not obvious what is a
"tagSegment".
* {{SolrMetricProducer.getUniqueMetricTag}} has a misleading javadoc comment -
it does not produce a metric name but a tag for tying together objects of the
same life-cycle.
* Additionally, if {{parentName.contains(name)}} should be an error - I don't
see when it would be a valid occurrence?
* {{SolrMetricProducer.initializeMetrics(SolrMetrics)}} has this peculiar
message in the exception: ""This means , the class has not implemented both of
these methods". We can do so much better here...
* the separator for tag hierarchy is ":" even though the javadoc claims it's
"A.B.C". FWIW I think ":" is a better choice, so just the javadoc needs to be
fixed.
* {{SolrMetricProducer.getMetrics()}} should not have a default implementation
on master, only on branch_8x. Also, the name of the method should reflect the
name of the object it returns, whatever we end up with.
* similarly, {{SolrMetricProducer.initializeMetrics(SolrMetricManager ...)}}
should be deprecated on branch_8x and removed from master.
* {{PluginBag.put(String, T)}} should not take care of closing old instances of
plugins - instead the corresponding {{PluginBag.put(String, PluginHolder)}}
(which is also called directly from SolrCore) should do this, otherwise in some
cases the old plugin will be closed and in some others it won't. This method
should also check for object identity, if the code re-registers the same
instance of plugin holder.
* {{RequestHandlerBase.getMetricRegistry}} will throw an NPE when called before
{{initializeMetrics}} is called.
* why does {{SuggestComponent}} still call
{{metricsInfo.metricManager.registerGauge}} instead of {{metricsInfo.gauge}} ?
BTW, the name of this field
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16910159#comment-16910159 ] ASF subversion and git services commented on SOLR-13677: Commit 2a7d2df5ff52bb09daf3223c4144aaf6964acdfe in lucene-solr's branch refs/heads/jira/SOLR-13677_2 from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=2a7d2df ] SOLR-13677: new fresh branch > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > Time Spent: 2h 40m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16907892#comment-16907892 ] ASF subversion and git services commented on SOLR-13677: Commit 956f61dde9b1ce3fee6b609955d41d0b90c67285 in lucene-solr's branch refs/heads/jira/SOLR-13677_1 from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=956f61d ] SOLR-13677: minimize changes > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > Time Spent: 2h 40m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16902043#comment-16902043 ] Noble Paul commented on SOLR-13677: --- [~ab] please take a look at the new PR > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > Time Spent: 0.5h > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16902040#comment-16902040 ] ASF subversion and git services commented on SOLR-13677: Commit 9b0003a7037206d937b9f4aa48e5dc4cf80fdd0f in lucene-solr's branch refs/heads/jira/SOLR-13677_1 from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=9b0003a ] SOLR-13677: Take 2 > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > Time Spent: 20m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901459#comment-16901459 ] Noble Paul commented on SOLR-13677: --- bq.the tag properly represents the object or group of objects with the same life-cycle -... I think this is a better solution. I was not sure about the implications of such a change. I will try to implement it and you can review it > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > Time Spent: 10m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901449#comment-16901449
]
Andrzej Bialecki commented on SOLR-13677:
--
bq. usually close is called always
:D There's no hard guarantee in any of these scenarios, just a degree of
likelihood.
I think that the patch over-complicates things by keeping an explicit reference
to the gauges. It should not be needed if we properly use the {{tag}} argument
when registering gauges.
The {{tag}} attribute was added to solve a problem of non-deterministic order
of gauge registration during core reload - the new core would already register
some of the new gauges but the old core would linger for a while and when it
tried to unregister gauges with the same names it would unregister the new ones
instead of the old ones.
So the important thing about the {{tag}} argument in
{{SolrMetricManager.registerGauge}} and {{SolrMetricManager.unregisterGauges}}
is this: the tag represents an object or a group of objects with the same
life-cycle. Until now the tag was generated by {{SolrCore}} and passed to all
its components because they had the same lifecycle. Also, it was {{SolrCore}}
(via {{SolrCoreMetricManager}}) that would call {{unregisterGauges}} on behalf
of all its components.
Now, if the life-cycle of components is different from that of {{SolrCore}}
then we need to make sure of two things:
* the {{tag}} properly represents the object or group of objects with the same
life-cycle - so if eg. SolrCache-s can be re-loaded without reloading SolrCore
then they should no longer use the same tag as their parent SolrCore. Instead
they should generate their own tags.
* each component must be now responsible for unregistering its own gauges, as
identified by its own tag. We can strongly encourage implementors to do this in
each component's {{AutoCloseable.close()}} but I don't see any easy way to
actually enforce it.
This approach doesn't require keeping actual references to gauges in each
component.
For convenience we could also extend the concept of {{tag}} so that it's
multi-valued - eg. a cache would use its own tag and the parent SolrCore-s tag.
This way both the cache and the SolrCore could each easily unregister gauges
that they (or their parent) created.
> All Metrics Gauges should be unregistered by the objects that registered them
> -
>
> Key: SOLR-13677
> URL: https://issues.apache.org/jira/browse/SOLR-13677
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: metrics
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The life cycle of Metrics producers are managed by the core (mostly). So, if
> the lifecycle of the object is different from that of the core itself, these
> objects will never be unregistered from the metrics registry. This will lead
> to memory leaks
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901418#comment-16901418
]
Noble Paul commented on SOLR-13677:
---
[~cpoerschke], the problem is not with adding those {{remember()}} {{forget()}}
methods. There is no way to ensure that those methods are invoked . We may be
able to check them today. But who will ensure that about the future components.
The advantage of piggybacking on {{close()}} is that it is a well known and
usually close is called always
> All Metrics Gauges should be unregistered by the objects that registered them
> -
>
> Key: SOLR-13677
> URL: https://issues.apache.org/jira/browse/SOLR-13677
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: metrics
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The life cycle of Metrics producers are managed by the core (mostly). So, if
> the lifecycle of the object is different from that of the core itself, these
> objects will never be unregistered from the metrics registry. This will lead
> to memory leaks
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901175#comment-16901175
]
Andrzej Bialecki commented on SOLR-13677:
--
Also, {{GaugeRef}} can be an interface that {{GaugeWrapper}} implements, then
there's no need to create even more objects.
> All Metrics Gauges should be unregistered by the objects that registered them
> -
>
> Key: SOLR-13677
> URL: https://issues.apache.org/jira/browse/SOLR-13677
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: metrics
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The life cycle of Metrics producers are managed by the core (mostly). So, if
> the lifecycle of the object is different from that of the core itself, these
> objects will never be unregistered from the metrics registry. This will lead
> to memory leaks
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901172#comment-16901172
]
Andrzej Bialecki commented on SOLR-13677:
--
Thanks Christine, this looks somewhat cleaner.
For historical reasons (a complicated refactoring of SolrInfoMBean, UI
dependencies, JMX, etc) some of the methods one would expect from
{{SolrMetricProducer}} ended up in {{SolrInfoBean}} instead, and this is also
the interface that is passed to {{registerGauge}}. Unless we want to do a
larger refactoring now, we could treat {{SolrInfoBean}} as the memory, and add
the default methods for remembering and forgetting the gauges to this interface.
> All Metrics Gauges should be unregistered by the objects that registered them
> -
>
> Key: SOLR-13677
> URL: https://issues.apache.org/jira/browse/SOLR-13677
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: metrics
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The life cycle of Metrics producers are managed by the core (mostly). So, if
> the lifecycle of the object is different from that of the core itself, these
> objects will never be unregistered from the metrics registry. This will lead
> to memory leaks
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16900959#comment-16900959
]
Christine Poerschke commented on SOLR-13677:
{code:java}
- public void registerGauge(SolrInfoBean info, String registry, Gauge gauge,
String tag, boolean force, String metricName, String... metricPath) {
+ public GaugeRef registerGauge(SolrInfoBean info, String registry, Gauge
gauge, String tag, boolean force, String metricName, String... metricPath) {
{code}
The above {{registerGauge}} method change to make it return a gauge reference
encourages but does not ensure that the caller 'remembers' the reference and so
that it is later then included in the unregister calls. There's also a small
amount of repetition w.r.t. iterating over the {{myGauges}} collection and
unregistering the elements.
I wonder if some sort of container or wrapper class might be helpful i.e.
{{SolrMetricManager.registerGauge}} would be sure to call 'remember' for the
gauge and the close(\?) method of the producer would call the 'forgetAll'
method. What do you think?
{code:java}
+ class FooBar {
+ private final List gaugeRefs = new ArrayList<>();
+ void remember(GaugeRef gaugeRef) {
+ gaugeRefs.add(gaugeRef);
+ }
+ void forgetAll() {
+ for (GaugeRef gaugeRef : gaugeRefs) {
+ gaugeRef.unregister();
+ }
+ gaugeRefs.clear();
+ }
+ }
+
+ public void registerGauge(FooBar memory, SolrInfoBean info, String registry,
Gauge gauge, String tag, boolean force, String metricName, String...
metricPath) {
+ memory.remember(registerGauge(info, registry, gauge, tag, force,
metricName, metricPath));
+ }
+
+ private GaugeRef registerGauge(SolrInfoBean info, String registry, Gauge
gauge, String tag, boolean force, String metricName, String... metricPath) {
- public void registerGauge(SolrInfoBean info, String registry, Gauge gauge,
String tag, boolean force, String metricName, String... metricPath) {
...
{code}
> All Metrics Gauges should be unregistered by the objects that registered them
> -
>
> Key: SOLR-13677
> URL: https://issues.apache.org/jira/browse/SOLR-13677
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: metrics
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The life cycle of Metrics producers are managed by the core (mostly). So, if
> the lifecycle of the object is different from that of the core itself, these
> objects will never be unregistered from the metrics registry. This will lead
> to memory leaks
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16900610#comment-16900610 ] ASF subversion and git services commented on SOLR-13677: Commit f983ede5fc7d14cb004d1e8674b709ffefd8b06e in lucene-solr's branch refs/heads/jira/SOLR-13677 from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=f983ede ] SOLR-13677: Created an interface called GaugeRef instead of exposing the internal class GaugeWrapper > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > Time Spent: 10m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16900424#comment-16900424
]
Noble Paul commented on SOLR-13677:
---
bq. In {{SolrMetricProducer}} instead of using {{AutoCloseable.close()}} simply
add a different default method eg. {{unregisterMetrics()}}.
I thought of doing it. But the problem is that there is a chance that
{{unregisterMetrics()}} is not invoked at all as it is not the responsibility
of the metrics framework. {{AutoCloseable.close()}} is called anyway and any
and all the cleanup should be performed there. Even if we have an
{{unregisterMetrics()}} method, we should automatically call it from the
{{AutoCloseable.close()}} method.
bq.I don't think we should expose {{GaugeWrapper}} outside of
{{SolrMetricManager}}, this is really an internal detail. ..
The call to unregister should be just as simple as a method call
{{someObject.unregister()}} . As a component writer, I don't want to gather the
various parameters required by the metrics framework to unregister my
component. It should be totally hidden outside of the metrics framework. I
agree that the {{GaugeWrapper}} is an internal component. It may make sense to
rename it and keep it as a top level class.
> All Metrics Gauges should be unregistered by the objects that registered them
> -
>
> Key: SOLR-13677
> URL: https://issues.apache.org/jira/browse/SOLR-13677
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: metrics
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The life cycle of Metrics producers are managed by the core (mostly). So, if
> the lifecycle of the object is different from that of the core itself, these
> objects will never be unregistered from the metrics registry. This will lead
> to memory leaks
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16900152#comment-16900152
]
Andrzej Bialecki commented on SOLR-13677:
--
The current patch really complicates the API and the expectations on the
implementors. I think we need a different strategy.
How about the following:
* in {{SolrMetricProducer}} instead of using {{AutoCloseable.close()}} simply
add a different default method eg. {{unregisterMetrics()}}. Chances are that
many of existing components already implement {{close()}} so the implementors
will not be aware that they need to also unregister the component's metrics.
* I don't think we should expose {{GaugeWrapper}} outside of SolrMetricManager,
this is really an internal detail. Adding the method above should be sufficient
to explicitly unregister the metrics - and all {{SolrInfoBean}}-s already know
what metric names (and what tag) they registered, because the tag is passed in
{{initializeMetrics}} and the names are recorded in {{registerMetricName}}.
> All Metrics Gauges should be unregistered by the objects that registered them
> -
>
> Key: SOLR-13677
> URL: https://issues.apache.org/jira/browse/SOLR-13677
> Project: Solr
> Issue Type: Improvement
> Security Level: Public(Default Security Level. Issues are Public)
> Components: metrics
>Reporter: Noble Paul
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The life cycle of Metrics producers are managed by the core (mostly). So, if
> the lifecycle of the object is different from that of the core itself, these
> objects will never be unregistered from the metrics registry. This will lead
> to memory leaks
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16899269#comment-16899269 ] Noble Paul commented on SOLR-13677: --- What we need is avoid notifications for commits to all the JIRA branches . Using a feature branch is good for collaboration. Every committer automatically had access to your branch > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > Time Spent: 10m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16899268#comment-16899268 ] Jan Høydahl commented on SOLR-13677: Noble, is there a reason why you push your feature branch to Apache GIT instead of keeping it in your own fork and do a PR when it’s ready? Do you expect there to be collaboration? Reason I ask is that it adds some “noise” to the lists for every push, merge etc. > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > Time Spent: 10m > Remaining Estimate: 0h > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Commented] (SOLR-13677) All Metrics Gauges should be unregistered by the objects that registered them
[ https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16899211#comment-16899211 ] ASF subversion and git services commented on SOLR-13677: Commit 3ce75aac49c79a023a9f1519badfe769e6a8f797 in lucene-solr's branch refs/heads/jira/SOLR-13677 from Noble Paul [ https://gitbox.apache.org/repos/asf?p=lucene-solr.git;h=3ce75aa ] SOLR-13677: initial commit > All Metrics Gauges should be unregistered by the objects that registered them > - > > Key: SOLR-13677 > URL: https://issues.apache.org/jira/browse/SOLR-13677 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: metrics >Reporter: Noble Paul >Priority: Major > > The life cycle of Metrics producers are managed by the core (mostly). So, if > the lifecycle of the object is different from that of the core itself, these > objects will never be unregistered from the metrics registry. This will lead > to memory leaks -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
