[
https://issues.apache.org/jira/browse/SOLR-13677?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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<T>)}}
(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 should be changed to reflect the name of the
{{SolrMetrics}} class.
* {{FastLRUCache.close()}} and the same method in other caches should simply
call {{SolrMetricProducer.super.close()}} instead of replicating the logic from
that method.
* there are no unit tests for the new functionality of managing gauges with
hierarchical metric tags.
* the new field {{SolrMetrics metrics}} should be declared together with the
declarations of other fields.
* I see that in general many {{SolrMetricProducer}} implementations on master
still use the old API - they should be converted to the new API if we're
serious about using it and deprecating the old one. This can be done in a
separate issue.
> 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
