dsmiley commented on code in PR #3458:
URL: https://github.com/apache/solr/pull/3458#discussion_r2280729254
##########
solr/core/src/java/org/apache/solr/metrics/SolrCoreMetricManager.java:
##########
@@ -105,30 +112,35 @@ public void loadReporters() {
}
/**
- * Make sure that metrics already collected that correspond to the old core
name are carried over
- * and will be used under the new core name. This method also reloads
reporters so that they use
- * the new core name.
+ * Re-register all metric producers associated with this core. This
recreates the metric registry
+ * resetting its state
*/
- // NOCOMMIT SOLR-17458: Update for core renaming
- public void afterCoreRename() {
- assert core.getCoreDescriptor().getCloudDescriptor() == null;
- String oldRegistryName = solrMetricsContext.getRegistryName();
- String oldLeaderRegistryName = leaderRegistryName;
- String newRegistryName =
- createRegistryName(cloudMode, collectionName, shardName, replicaName,
core.getName());
- leaderRegistryName = createLeaderRegistryName(cloudMode, collectionName,
shardName);
- if (oldRegistryName.equals(newRegistryName)) {
- return;
- }
- // close old reporters
- metricManager.closeReporters(oldRegistryName, solrMetricsContext.getTag());
- if (oldLeaderRegistryName != null) {
- metricManager.closeReporters(oldLeaderRegistryName,
solrMetricsContext.getTag());
- }
- solrMetricsContext =
- new SolrMetricsContext(metricManager, newRegistryName,
solrMetricsContext.getTag());
- // load reporters again, using the new core name
- loadReporters();
+ public void reregisterCoreMetrics() {
+ this.solrMetricsContext =
+ new SolrMetricsContext(
+ metricManager,
+ createRegistryName(cloudMode, collectionName, shardName,
replicaName, core.getName()),
+ solrMetricsContext.getTag());
+ metricManager.removeRegistry(solrMetricsContext.getRegistryName());
+ if (leaderRegistryName != null)
metricManager.removeRegistry(leaderRegistryName);
+
+ var attributesBuilder =
+ Attributes.builder()
+ .put(CORE_ATTR, core.getName())
Review Comment:
my point about the duplication, for example, can be seen in the attributes
construction. This is duplicated over in `registerMetricProcducer`. But it's
not a lot of duplication. It could be ameliorated with comments on both sides.
##########
solr/core/src/java/org/apache/solr/metrics/SolrCoreMetricManager.java:
##########
@@ -105,30 +112,35 @@ public void loadReporters() {
}
/**
- * Make sure that metrics already collected that correspond to the old core
name are carried over
- * and will be used under the new core name. This method also reloads
reporters so that they use
- * the new core name.
+ * Re-register all metric producers associated with this core. This
recreates the metric registry
+ * resetting its state
*/
- // NOCOMMIT SOLR-17458: Update for core renaming
- public void afterCoreRename() {
- assert core.getCoreDescriptor().getCloudDescriptor() == null;
- String oldRegistryName = solrMetricsContext.getRegistryName();
- String oldLeaderRegistryName = leaderRegistryName;
- String newRegistryName =
- createRegistryName(cloudMode, collectionName, shardName, replicaName,
core.getName());
- leaderRegistryName = createLeaderRegistryName(cloudMode, collectionName,
shardName);
- if (oldRegistryName.equals(newRegistryName)) {
- return;
- }
- // close old reporters
- metricManager.closeReporters(oldRegistryName, solrMetricsContext.getTag());
- if (oldLeaderRegistryName != null) {
- metricManager.closeReporters(oldLeaderRegistryName,
solrMetricsContext.getTag());
- }
- solrMetricsContext =
- new SolrMetricsContext(metricManager, newRegistryName,
solrMetricsContext.getTag());
- // load reporters again, using the new core name
- loadReporters();
+ public void reregisterCoreMetrics() {
+ this.solrMetricsContext =
+ new SolrMetricsContext(
+ metricManager,
+ createRegistryName(cloudMode, collectionName, shardName,
replicaName, core.getName()),
+ solrMetricsContext.getTag());
+ metricManager.removeRegistry(solrMetricsContext.getRegistryName());
+ if (leaderRegistryName != null)
metricManager.removeRegistry(leaderRegistryName);
+
+ var attributesBuilder =
Review Comment:
you should just build this; have it be the attributes itself. You're
building in a loop below but it's redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
