cadonna commented on code in PR #18275:
URL: https://github.com/apache/kafka/pull/18275#discussion_r1893638920
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java:
##########
@@ -462,8 +462,10 @@ public void record(final long now) {
writeStallDuration +=
valueProviders.statistics.getAndResetTickerCount(TickerType.STALL_MICROS);
bytesWrittenDuringCompaction +=
valueProviders.statistics.getAndResetTickerCount(TickerType.COMPACT_WRITE_BYTES);
bytesReadDuringCompaction +=
valueProviders.statistics.getAndResetTickerCount(TickerType.COMPACT_READ_BYTES);
- numberOfOpenFiles +=
valueProviders.statistics.getAndResetTickerCount(TickerType.NO_FILE_OPENS)
- -
valueProviders.statistics.getAndResetTickerCount(TickerType.NO_FILE_CLOSES);
+
+ // NO_FILE_CLOSES metric was removed in RocksDB 9.7.3.
+ // This now tracks the total number of file opens since the last
reset.
+ numberOfOpenFiles +=
valueProviders.statistics.getAndResetTickerCount(TickerType.NO_FILE_OPENS);
Review Comment:
@swikarpat I believe @mjsax means to not return the number of opened files
but just `-1`. I agree with him. The number of opened files without the number
of closed files does not give us much info. Furthermore, by recording the
number of opened files, the metric would seem correct, but actually would not
show the correct values because the metric is defined as the number of
currently open files. Returning constant `-1` would make it clear that the
metric does not work.
In addition to returning `-1`, could you please also update the
documentation of the metrics in this PR:
https://github.com/apache/kafka/blob/a0a501952b6d61f6f273bdb8f842346b51e9dfce/docs/ops.html#L3457
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBGenericOptionsToDbOptionsColumnFamilyOptionsAdapter.java:
##########
@@ -842,12 +830,6 @@ public Options setSstFileManager(final SstFileManager
sstFileManager) {
return this;
}
- @Override
- public Options setLogger(final org.rocksdb.Logger logger) {
- dbOptions.setLogger(logger);
Review Comment:
It is fine to change it like you did. If users use a logger class that is
derived from `Logger` they have to modify it in any case because `Logger` now
implements interface `LoggerInterface`. Thus, the change of the method will
most likely not directly affect users on a source level. It will affect them on
binary level, though, but that is the trade-off here.
--
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]
