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
