XComp commented on a change in pull request #15039:
URL: https://github.com/apache/flink/pull/15039#discussion_r584052160
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/metrics/util/MetricUtilsTest.java
##########
@@ -161,7 +222,7 @@ public void testHeapMetricUsageNotStatic() throws Exception
{
}
@Test
- public void testMetaspaceMetricUsageNotStatic() throws
InterruptedException {
+ public void testMetaspaceMetricUsageNotStatic() throws Exception {
Review comment:
```suggestion
private static boolean hasMetaspaceMemoryPool() {
return ManagementFactory.getMemoryPoolMXBeans().stream()
.anyMatch(bean -> "Metaspace".equals(bean.getName()));
}
@Test
public void testMetaspaceMetricUsageNotStatic() throws Exception {
assumeTrue(
"The test expects to run in a JVM providing the Metaspace
memory pool",
hasMetaspaceMemoryPool());
```
(be careful with just applying my proposed change as I couldn't include the
`@Test` to the diff for some strange GitHub reasons)
Since I ran into it again: Should we ignore the test if the JVM does not
provide a Metaspace memory pool? We document this assumption already in [memory
configuration's
docs](https://ci.apache.org/projects/flink/flink-docs-stable/ops/metrics.html#memory).
This applies to `testMetaspaceCompleteness` as well.
@zentol What's your opinion on that?
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/metrics/util/MetricUtilsTest.java
##########
@@ -180,21 +241,54 @@ public MetricGroup addGroup(String name) {
// check memory usage difference multiple times since other tests may
affect memory usage as
// well
for (int x = 0; x < 10; x++) {
- List<Runnable> consumerList = new ArrayList<>();
- for (int i = 0; i < 10; i++) {
- consumerList.add(() -> {});
- }
+ Class<MetricUtils> redefinedMetricUtilsClass =
redefineAsNewClass(MetricUtils.class);
final long usedMetaspaceAfterAllocation = used.getValue();
if (usedMetaspaceInitially != usedMetaspaceAfterAllocation) {
return;
}
+
+ // This assertion try to prevent local variable from gc in absent
of
+ // Reference.reachabilityFence.
+ Assert.assertNotSame(MetricUtils.class, redefinedMetricUtilsClass);
+
Thread.sleep(50);
}
Assert.fail("Metaspace usage metric never changed it's value.");
}
+ @Test
+ public void testNonHeapMetricUsageNotStatic() throws Exception {
+ final InterceptingOperatorMetricGroup nonHeapMetrics =
+ new InterceptingOperatorMetricGroup();
+
+ MetricUtils.instantiateNonHeapMemoryMetrics(nonHeapMetrics);
+
+ @SuppressWarnings("unchecked")
+ final Gauge<Long> used = (Gauge<Long>)
nonHeapMetrics.get(MetricNames.MEMORY_USED);
+
+ final long usedNonHeapInitially = used.getValue();
+
+ // check memory usage difference multiple times since other tests may
affect memory usage as
+ // well
+ for (int x = 0; x < 10; x++) {
Review comment:
Some minor thing: Shall we move the for loop into a `static` assert
utility method to remove code redundancy?
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/metrics/util/MetricUtilsTest.java
##########
@@ -59,6 +68,58 @@
private static final Logger LOG =
LoggerFactory.getLogger(MetricUtilsTest.class);
Review comment:
```suggestion
```
That's a bit unrelated to your change but we could actually remove the `LOG`
here as it's not used anywhere.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/metrics/util/MetricUtilsTest.java
##########
@@ -180,21 +241,54 @@ public MetricGroup addGroup(String name) {
// check memory usage difference multiple times since other tests may
affect memory usage as
// well
for (int x = 0; x < 10; x++) {
- List<Runnable> consumerList = new ArrayList<>();
- for (int i = 0; i < 10; i++) {
- consumerList.add(() -> {});
- }
+ Class<MetricUtils> redefinedMetricUtilsClass =
redefineAsNewClass(MetricUtils.class);
final long usedMetaspaceAfterAllocation = used.getValue();
if (usedMetaspaceInitially != usedMetaspaceAfterAllocation) {
return;
}
+
+ // This assertion try to prevent local variable from gc in absent
of
+ // Reference.reachabilityFence.
+ Assert.assertNotSame(MetricUtils.class, redefinedMetricUtilsClass);
Review comment:
Out of curiosity: Could you elaborate a bit on why this `assert`
prevents the GC from removing the class immediately? I would have assumed that
collecting the classes in a collection would be the way to go to not lose the
reference and avoid triggering GC on it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
