cadonna commented on a change in pull request #9098: URL: https://github.com/apache/kafka/pull/9098#discussion_r469095757
########## File path: streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBStore.java ########## @@ -181,26 +181,42 @@ void openDB(final ProcessorContext context) { throw new ProcessorStateException(fatal); } - // Setup metrics before the database is opened, otherwise the metrics are not updated + // Setup statistics before the database is opened, otherwise the statistics are not updated // with the measurements from Rocks DB - maybeSetUpMetricsRecorder(configs); + maybeSetUpStatistics(configs); openRocksDB(dbOptions, columnFamilyOptions); open = true; + + addValueProvidersToMetricsRecorder(); } - private void maybeSetUpMetricsRecorder(final Map<String, Object> configs) { - if (userSpecifiedOptions.statistics() == null && + private void maybeSetUpStatistics(final Map<String, Object> configs) { + if (userSpecifiedOptions.statistics() != null) { Review comment: I see what you are aiming at. However, why should we throw an `IllegalStateException` when `userSpecifiedOptions.statistics() != null`? The user could provide a statistics object through the config setter and use it to read the statistics externally to our metric framework. Such a pattern was supported before and was not changed in this PR or the PR that introduced the statistics-based RocksDB metrics. Anyways, it is good that you mentioned this, because I checked again the code in the metrics recorder and a `null` check was missing. Plus I included some checks to ensure that either all statistics of a metrics recorder are`null` or all statistics of a metrics recorder are NOT `null`. ---------------------------------------------------------------- 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: us...@infra.apache.org