yihua commented on code in PR #18484:
URL: https://github.com/apache/hudi/pull/18484#discussion_r3059065256
##########
hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/sink/partitioner/index/RocksDBIndexBackend.java:
##########
@@ -52,6 +61,41 @@ public void update(String recordKey,
HoodieRecordGlobalLocation recordGlobalLoca
this.rocksDBDAO.put(COLUMN_FAMILY, recordKey, recordGlobalLocation);
}
+ @Override
+ public void registerMetrics(MetricGroup metricGroup) {
+ if (rocksDBIndexMetrics != null) {
+ return;
+ }
+ this.rocksDBIndexMetrics = new FlinkRocksDBIndexMetrics(metricGroup, this);
+ this.rocksDBIndexMetrics.registerMetrics();
+ }
+
+ @VisibleForTesting
+ void flush() {
+ this.rocksDBDAO.flush();
+ }
+
+ public long getLongMetric(String property) {
+ try {
+ return this.rocksDBDAO.getLongProperty(property);
+ } catch (RocksDBException | RuntimeException e) {
+ log.debug("Failed to read RocksDB metric property {}", property, e);
Review Comment:
🤖 The two `getTickerCount` calls are not atomic — the lock is released
between them. On shutdown, this could produce a brief 100% hit ratio (e.g.,
first call returns a real hit count, then close() runs, second call returns 0
for misses). Is this acceptable for your use case, or would it be worth adding
a single synchronized method on `RocksDBDAO` that fetches both tickers
atomically?
<sub><i>- Generated by an AI agent and may contain mistakes. Please verify
any suggestions before applying.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/util/collection/RocksDBDAO.java:
##########
@@ -465,6 +470,32 @@ public void addColumnFamily(String columnFamilyName) {
});
}
+ /**
+ * Retrieves a numeric property aggregated across all column families.
+ */
+ public synchronized long getLongProperty(String property) throws
RocksDBException {
+ return closed ? 0L : getRocksDB().getAggregatedLongProperty(property);
+ }
+
+ /**
+ * Retrieves the current ticker count.
+ */
+ public synchronized long getTickerCount(TickerType tickerType) {
+ return closed || statistics == null ? 0L :
statistics.getTickerCount(tickerType);
+ }
+
+ /**
Review Comment:
🤖 Unlike `getLongProperty` and `getTickerCount`, `flush()` is not
`synchronized`. This means it can race with `close()` — if `close()` runs
concurrently, the RocksDB instance and column family handles could be closed
while `flush()` is still executing, leading to a native crash or undefined
behavior. Could you add `synchronized` here for consistency, or at least guard
with the `closed` flag?
<sub><i>- Generated by an AI agent and may contain mistakes. Please verify
any suggestions before applying.</i></sub>
--
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]
