Zakelly commented on code in PR #25884:
URL: https://github.com/apache/flink/pull/25884#discussion_r1900800573
##########
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/ForStResourceContainer.java:
##########
@@ -120,21 +121,24 @@ public final class ForStResourceContainer implements
AutoCloseable {
@Nullable private java.nio.file.Path relocatedDbLogBaseDir;
+ /** The metric group for reporting metrics. */
+ @Nullable private MetricGroup metricGroup;
Review Comment:
Could be `final`
##########
docs/content/docs/ops/metrics.md:
##########
@@ -1526,6 +1526,37 @@ Note that for failed checkpoints, metrics are updated on
a best efforts basis an
### RocksDB
Certain RocksDB native metrics are available but disabled by default, you can
find full documentation [here]({{< ref "docs/deployment/config"
>}}#rocksdb-native-metrics)
+### ForStDB
+Certain ForStDB native metrics are available but disabled by default, you can
find full documentation [here]({{< ref "docs/deployment/config"
>}}#rocksdb-native-metrics)
+<table class="table table-bordered">
+ <thead>
+ <tr>
+ <th class="text-left" style="width: 18%">Scope</th>
+ <th class="text-left" style="width: 26%">Metrics</th>
+ <th class="text-left" style="width: 48%">Description</th>
+ <th class="text-left" style="width: 8%">Type</th>
+ </tr>
+ </thead>
+ <tbody>
+ <tr>
+ <th rowspan="3"><strong>Task/Operator</strong></th>
+ <td>forst-cache.hit</td>
Review Comment:
I suggest we use `forst` as a single metric group name? like `rocksdb` does
```suggestion
<td>forst.file-cache.hit</td>
```
##########
docs/content.zh/docs/ops/metrics.md:
##########
@@ -1536,6 +1536,37 @@ Note that for failed checkpoints, metrics are updated on
a best efforts basis an
### RocksDB
Certain RocksDB native metrics are available but disabled by default, you can
find full documentation [here]({{< ref "docs/deployment/config"
>}}#rocksdb-native-metrics)
+### ForStDB
+Certain ForStDB native metrics are available but disabled by default, you can
find full documentation [here]({{< ref "docs/deployment/config"
>}}#rocksdb-native-metrics)
Review Comment:
Remove this? Since we don't have the mentioned full documentation.
##########
docs/content/docs/ops/metrics.md:
##########
@@ -1526,6 +1526,37 @@ Note that for failed checkpoints, metrics are updated on
a best efforts basis an
### RocksDB
Certain RocksDB native metrics are available but disabled by default, you can
find full documentation [here]({{< ref "docs/deployment/config"
>}}#rocksdb-native-metrics)
+### ForStDB
+Certain ForStDB native metrics are available but disabled by default, you can
find full documentation [here]({{< ref "docs/deployment/config"
>}}#rocksdb-native-metrics)
+<table class="table table-bordered">
+ <thead>
+ <tr>
+ <th class="text-left" style="width: 18%">Scope</th>
+ <th class="text-left" style="width: 26%">Metrics</th>
+ <th class="text-left" style="width: 48%">Description</th>
+ <th class="text-left" style="width: 8%">Type</th>
+ </tr>
+ </thead>
+ <tbody>
+ <tr>
+ <th rowspan="3"><strong>Task/Operator</strong></th>
+ <td>forst-cache.hit</td>
+ <td>The hit count of ForSt state backend cache.</td>
+ <td>Counter</td>
+ </tr>
+ <tr>
+ <td>forst-cache.miss</td>
+ <td>The miss count of ForSt state backend cache.</td>
+ <td>Counter</td>
+ </tr>
+ <tr>
+ <td>forst-cache.cachedBytes</td>
Review Comment:
We'd better unify the naming style, using kebab-case instead.
```suggestion
<td>forst.file-cache.used-bytes</td>
```
##########
flink-state-backends/flink-statebackend-forst/src/main/java/org/apache/flink/state/forst/fs/cache/FileBasedCache.java:
##########
@@ -46,12 +52,37 @@ public class FileBasedCache extends LruCache<String,
FileCacheEntry> {
/** Whether the cache is closed. */
private volatile boolean closed;
+ private MetricGroup metricGroup;
+
+ /** Hit metric. */
+ private transient Counter hitCounter;
+
+ /** Miss metric. */
+ private transient Counter missCounter;
+
+ /** Cached bytes metric. */
+ private transient Gauge<Long> cachedBytesGauge;
+
public FileBasedCache(
- int capacity, CacheLimitPolicy cacheLimitPolicy, FileSystem
cacheFs, Path basePath) {
+ int capacity,
+ CacheLimitPolicy cacheLimitPolicy,
+ FileSystem cacheFs,
+ Path basePath,
+ MetricGroup metricGroup) {
super(capacity, cacheLimitPolicy);
this.closed = false;
this.cacheFs = cacheFs;
this.basePath = basePath;
+ if (metricGroup != null) {
+ this.metricGroup = metricGroup;
+ this.hitCounter =
+ metricGroup.counter(FORST_CACHE + ".hit", new
ThreadSafeSimpleCounter());
+ this.missCounter =
+ metricGroup.counter(FORST_CACHE + ".miss", new
ThreadSafeSimpleCounter());
+ this.cachedBytesGauge =
+ metricGroup.gauge(
+ FORST_CACHE + ".cachedBytes", () ->
cacheLimitPolicy.cachedBytes());
Review Comment:
Besides this, each policy may have its' own metrics. e.g. The
`SpaceBasedCacheLimitPolicy` may need report the remaining space.
--
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]
