azagrebin commented on a change in pull request #13316:
URL: https://github.com/apache/flink/pull/13316#discussion_r487982557
##########
File path: docs/monitoring/metrics.md
##########
@@ -1013,17 +1013,37 @@ Thus, in order to infer the metric identifier:
</thead>
<tbody>
<tr>
- <th rowspan="2"><strong>TaskManager</strong></th>
- <td rowspan="2">Status.Network</td>
+ <th rowspan="6"><strong>TaskManager</strong></th>
+ <td rowspan="6">Status.Shuffle.Netty</td>
Review comment:
Why are the deprecated metrics changed and extended?
Afaik, we still have
`NettyShuffleMetricFactory::METRIC_GROUP_NETWORK_DEPRECATED = "Network"`.
I suggest to leave the deprecated metrics untouched and document only
`Default shuffle service` table.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/taskmanager/TaskManagerDetailsHandler.java
##########
@@ -137,8 +137,13 @@ private static TaskManagerMetricsInfo
createTaskManagerMetricsInfo(MetricStore.T
long mappedUsed =
Long.valueOf(tmMetrics.getMetric("Status.JVM.Memory.Mapped.MemoryUsed", "0"));
long mappedMax =
Long.valueOf(tmMetrics.getMetric("Status.JVM.Memory.Mapped.TotalCapacity",
"0"));
- long memorySegmentsAvailable =
Long.valueOf(tmMetrics.getMetric("Status.Network.AvailableMemorySegments",
"0"));
- long memorySegmentsTotal =
Long.valueOf(tmMetrics.getMetric("Status.Network.TotalMemorySegments", "0"));
+ long networkMemorySegmentsAvailable =
Long.valueOf(tmMetrics.getMetric("Status.Network.AvailableMemorySegments",
"0"));
+ long networkMemorySegmentsUsed =
Long.valueOf(tmMetrics.getMetric("Status.Network.UsedMemorySegments", "0"));
+ long networkMemorySegmentsTotal =
Long.valueOf(tmMetrics.getMetric("Status.Network.TotalMemorySegments", "0"));
+
+ long networkMemoryAvailable =
Long.valueOf(tmMetrics.getMetric("Status.Network.AvailableMemory", "0"));
+ long networkMemoryUsed =
Long.valueOf(tmMetrics.getMetric("Status.Network.UsedMemory", "0"));
+ long networkMemoryTotal =
Long.valueOf(tmMetrics.getMetric("Status.Network.TotalMemory", "0"));
Review comment:
I also suggest to use new `Status.Shuffle.Netty.*` metric prefix here
instead of the deprecated one `Status.Network`.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/taskmanager/TaskManagerMetricsInfo.java
##########
@@ -58,8 +58,16 @@
public static final String FIELD_NAME_NETWORK_MEMORY_SEGMENTS_AVAILABLE
= "memorySegmentsAvailable";
+ public static final String FIELD_NAME_NETWORK_MEMORY_SEGMENTS_USED =
"memorySegmentsUsed";
Review comment:
if the json is part of public REST API, I would consider adding
`network*` or `shuffleNetty*` prefix for the related fields. Just `memory*`
looks confusing.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/io/network/buffer/NetworkBufferPoolTest.java
##########
@@ -109,6 +115,49 @@ public void testCreatePoolAfterDestroy() {
}
+ @Test
+ public void testMemoryUsageProperties() {
Review comment:
can this test be split into 3 tests? e.g.
- `testMemoryUsageOfCreatePool` (btw looks already covered by
`testCreatePoolAfterDestroy `)
- `testMemoryUsageOfPoolWithAllocation`
- `testMemoryUsageOfDestroyedPool`
----------------------------------------------------------------
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]
