Skip to content

Commit 92b0c32

Browse files
sk0x50sk0x50
authored
IGNITE-27296 Fix table metrics registration on recovery (#7208)
1 parent a4011c9 commit 92b0c32

File tree

5 files changed

+150
-25
lines changed

5 files changed

+150
-25
lines changed

‎modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,16 +65,20 @@ public interface MetricManager extends IgniteComponent {
6565
void registerSource(MetricSource src);
6666

6767
/**
68-
* Unregister metric source. See {@link MetricRegistry#unregisterSource(MetricSource)}.
68+
* Disables and unregisters metric source.
6969
*
7070
* @param src Metric source.
71+
* @see #disable(MetricSource)
72+
* @see MetricRegistry#unregisterSource(MetricSource)
7173
*/
7274
void unregisterSource(MetricSource src);
7375

7476
/**
75-
* Unregister metric source by name. See {@link MetricRegistry#unregisterSource(String)}.
77+
* Disables and unregisters metric source by name.
7678
*
7779
* @param srcName Metric source name.
80+
* @see #disable(String)
81+
* @see MetricRegistry#unregisterSource(String)
7882
*/
7983
void unregisterSource(String srcName);
8084

‎modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManagerImpl.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -186,12 +186,18 @@ public void registerSource(MetricSource src) {
186186

187187
@Override
188188
public void unregisterSource(MetricSource src) {
189-
inBusyLockSafe(busyLock, () -> registry.unregisterSource(src));
189+
inBusyLockSafe(busyLock, () -> {
190+
disable(src);
191+
registry.unregisterSource(src);
192+
});
190193
}
191194

192195
@Override
193196
public void unregisterSource(String srcName) {
194-
inBusyLockSafe(busyLock, () -> registry.unregisterSource(srcName));
197+
inBusyLockSafe(busyLock, () -> {
198+
disable(srcName);
199+
registry.unregisterSource(srcName);
200+
});
195201
}
196202

197203
@Override

‎modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/jmx/MetricSetMbean.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
*/
4646
public class MetricSetMbean implements DynamicMBean {
4747
/** Metric set. */
48-
private MetricSet metricSet;
48+
private final MetricSet metricSet;
4949

5050
/**
5151
* Constructs new MBean.
@@ -63,6 +63,9 @@ public Object getAttribute(String attribute) throws AttributeNotFoundException {
6363
}
6464

6565
Metric metric = metricSet.get(attribute);
66+
if (metric == null) {
67+
throw new AttributeNotFoundException("Attribute not found [attribute=" + attribute + ']');
68+
}
6669

6770
if (metric instanceof DoubleMetric) {
6871
return ((DoubleMetric) metric).value();
@@ -81,7 +84,8 @@ public Object getAttribute(String attribute) throws AttributeNotFoundException {
8184
return ((UuidGauge) metric).value();
8285
}
8386

84-
throw new AttributeNotFoundException("Unknown metric class [class=" + metric.getClass().getName() + ']');
87+
throw new AttributeNotFoundException(
88+
"Unknown metric class [attribute=" + attribute + ", class=" + metric.getClass().getName() + ']');
8589
}
8690

8791
@Override
@@ -91,13 +95,10 @@ public AttributeList getAttributes(String[] attributes) {
9195

9296
try {
9397
for (String attribute : attributes) {
98+
// getAttribute must return a value of the attribute, not Attribute instance.
9499
Object val = getAttribute(attribute);
95100

96-
if (val instanceof Attribute) {
97-
attrList.add((Attribute) val);
98-
} else {
99-
attrList.add(new Attribute(attribute, val));
100-
}
101+
attrList.add(new Attribute(attribute, val));
101102
}
102103
} catch (AttributeNotFoundException e) {
103104
throw new IllegalArgumentException(e);

‎modules/metrics/src/test/java/org/apache/ignite/internal/metrics/MetricManagerTest.java‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
package org.apache.ignite.internal.metrics;
19+
20+
import static org.apache.ignite.internal.metrics.exporters.jmx.JmxExporter.JMX_EXPORTER_NAME;
21+
import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willSucceedFast;
22+
import static org.apache.ignite.internal.util.IgniteUtils.makeMbeanName;
23+
import static org.hamcrest.MatcherAssert.assertThat;
24+
import static org.hamcrest.Matchers.is;
25+
import static org.hamcrest.Matchers.notNullValue;
26+
27+
import java.lang.management.ManagementFactory;
28+
import java.util.UUID;
29+
import javax.management.ObjectName;
30+
import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
31+
import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
32+
import org.apache.ignite.internal.logger.IgniteLogger;
33+
import org.apache.ignite.internal.logger.Loggers;
34+
import org.apache.ignite.internal.manager.ComponentContext;
35+
import org.apache.ignite.internal.metrics.configuration.MetricConfiguration;
36+
import org.apache.ignite.internal.metrics.sources.OsMetricSource;
37+
import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
38+
import org.junit.jupiter.api.Test;
39+
import org.junit.jupiter.api.extension.ExtendWith;
40+
41+
/**
42+
* Tests {@link MetricManager}.
43+
*/
44+
@ExtendWith(ConfigurationExtension.class)
45+
public class MetricManagerTest extends BaseIgniteAbstractTest {
46+
private static final IgniteLogger LOG = Loggers.forClass(MetricManagerTest.class);
47+
48+
private static final String NODE_NAME = "test-node-name";
49+
50+
private static final UUID NODE_ID = UUID.randomUUID();
51+
52+
@InjectConfiguration("mock.exporters = {" + JMX_EXPORTER_NAME + " = {exporterName = " + JMX_EXPORTER_NAME + "}}")
53+
private MetricConfiguration jmxMetricConfiguration;
54+
55+
@Test
56+
void metricSourceRegistration() throws Exception {
57+
MetricManager metricManager = createAndStartManager(jmxMetricConfiguration);
58+
59+
OsMetricSource source = new OsMetricSource();
60+
metricManager.registerSource(source);
61+
62+
MetricSet metricSet = metricManager.enable(source);
63+
assertThat(metricSet, notNullValue());
64+
65+
ObjectName name = makeMbeanName(NODE_NAME, metricSet.group(), metricSet.name());
66+
assertThat(ManagementFactory.getPlatformMBeanServer().isRegistered(name), is(true));
67+
68+
stopManager(metricManager);
69+
70+
assertThat(ManagementFactory.getPlatformMBeanServer().isRegistered(name), is(false));
71+
}
72+
73+
@Test
74+
void metricSourceCleanup() throws Exception {
75+
MetricManager metricManager = createAndStartManager(jmxMetricConfiguration);
76+
77+
OsMetricSource source = new OsMetricSource();
78+
metricManager.registerSource(source);
79+
80+
MetricSet metricSet = metricManager.enable(source);
81+
assertThat(metricSet, notNullValue());
82+
83+
ObjectName name = makeMbeanName(NODE_NAME, metricSet.group(), metricSet.name());
84+
assertThat(
85+
"Metric source was not registered.",
86+
ManagementFactory.getPlatformMBeanServer().isRegistered(name),
87+
is(true));
88+
89+
metricManager.unregisterSource(source);
90+
91+
assertThat(
92+
"Metric source was not disabled before unregistering.",
93+
ManagementFactory.getPlatformMBeanServer().isRegistered(name),
94+
is(false));
95+
96+
stopManager(metricManager);
97+
}
98+
99+
private static MetricManager createAndStartManager(MetricConfiguration configuration) {
100+
MetricManager metricManager = new MetricManagerImpl(LOG, null);
101+
102+
metricManager.configure(configuration, () -> NODE_ID, NODE_NAME);
103+
104+
assertThat(metricManager.startAsync(new ComponentContext()), willSucceedFast());
105+
106+
return metricManager;
107+
}
108+
109+
private static void stopManager(MetricManager metricManager) {
110+
metricManager.beforeNodeStop();
111+
assertThat(metricManager.stopAsync(new ComponentContext()), willSucceedFast());
112+
}
113+
}

‎modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1834,8 +1834,7 @@ private CompletableFuture<Void> recoverTables(long recoveryRevision, @Nullable H
18341834
// Handle missed table drop event.
18351835
int tableId = tableDescriptor.id();
18361836

1837-
boolean destroyTableEventFired = nextCatalog != null && nextCatalog.table(tableId) == null;
1838-
if (destroyTableEventFired) {
1837+
if (nextCatalog != null && nextCatalog.table(tableId) == null) {
18391838
destructionEventsQueue.enqueue(new DestroyTableEvent(nextCatalog.version(), tableId));
18401839
}
18411840

@@ -1853,12 +1852,6 @@ private CompletableFuture<Void> recoverTables(long recoveryRevision, @Nullable H
18531852
schemaDescriptor
18541853
);
18551854

1856-
if (destroyTableEventFired) {
1857-
// prepareTableResourcesOnRecovery registers a table metric source, so we need to unregister it here,
1858-
// just because the table is being dropped and there is no need to keep the metric source.
1859-
unregisterMetricsSource(tables.get(tableId));
1860-
}
1861-
18621855
startTableFutures.add(startTableFuture);
18631856
}
18641857

@@ -2090,10 +2083,15 @@ private void addTableToZone(int zoneId, TableImpl table) throws IgniteInternalEx
20902083
}
20912084

20922085
private TableMetricSource createAndRegisterMetricsSource(StorageTableDescriptor tableDescriptor, QualifiedName tableName) {
2086+
// The table might be created during the recovery phase.
2087+
// In that case, we should only register the metric source for the actual tables that exist in the latest catalog.
2088+
boolean registrationNeeded =
2089+
catalogService.latestCatalog().table(tableDescriptor.getId()) != null;
2090+
20932091
StorageEngine engine = dataStorageMgr.engineByStorageProfile(tableDescriptor.getStorageProfile());
20942092

20952093
// Engine can be null sometimes, see "TableManager.createTableStorage".
2096-
if (engine != null) {
2094+
if (engine != null && registrationNeeded) {
20972095
StorageEngineTablesMetricSource engineMetricSource = new StorageEngineTablesMetricSource(engine.name(), tableName);
20982096

20992097
engine.addTableMetrics(tableDescriptor, engineMetricSource);
@@ -2109,11 +2107,13 @@ private TableMetricSource createAndRegisterMetricsSource(StorageTableDescriptor
21092107

21102108
TableMetricSource source = new TableMetricSource(tableName);
21112109

2112-
try {
2113-
metricManager.registerSource(source);
2114-
metricManager.enable(source);
2115-
} catch (Exception e) {
2116-
LOG.warn("Failed to register metrics source for table [id={}, name={}].", e, tableDescriptor.getId(), tableName);
2110+
if (registrationNeeded) {
2111+
try {
2112+
metricManager.registerSource(source);
2113+
metricManager.enable(source);
2114+
} catch (Exception e) {
2115+
LOG.warn("Failed to register metrics source for table [id={}, name={}].", e, tableDescriptor.getId(), tableName);
2116+
}
21172117
}
21182118

21192119
return source;
@@ -2134,6 +2134,7 @@ private void unregisterMetricsSource(TableViewInternal table) {
21342134

21352135
String storageProfile = table.internalTable().storage().getTableDescriptor().getStorageProfile();
21362136
StorageEngine engine = dataStorageMgr.engineByStorageProfile(storageProfile);
2137+
21372138
// Engine can be null sometimes, see "TableManager.createTableStorage".
21382139
if (engine != null) {
21392140
try {

0 commit comments

Comments
 (0)