KAFKA-13973: Fix inflated block cache metrics by nicktelford · Pull Request #14317 · apache/kafka

nicktelford · 2023-08-31T11:22:02Z

All block cache metrics are being multiplied by the total number of column families. In a RocksDBTimestampedStore, we have 2 column families (the default, and the timestamped values), which causes all block cache metrics in these stores to become doubled.

The cause is that our metrics recorder uses getAggregatedLongProperty to fetch block cache metrics. getAggregatedLongProperty queries the property on each column family in the database, and sums the results.

Since we always configure all column families to share the same block cache, that causes the same block cache to be queried multiple times for its metrics, with the results added togehter, effectively multiplying the real value by the total number of column families.

To fix this, we should simply use getLongProperty, which queries a single column family (the default one). Since all column families share the same block cache, querying just one of them will give us the correct metrics for that shared block cache.

Note: the same block cache is shared among all column families of a store irrespective of whether the user has configured a shared block cache across multiple stores.

All block cache metrics are being multiplied by the total number of column families. In a `RocksDBTimestampedStore`, we have 2 column families (the default, and the timestamped values), which causes all block cache metrics in these stores to become doubled. The cause is that our metrics recorder uses `getAggregatedLongProperty` to fetch block cache metrics. `getAggregatedLongProperty` queries the property on each column family in the database, and sums the results. Since we always configure all column families to share the same block cache, that causes the same block cache to be queried multiple times for its metrics, with the results added togehter, effectively multiplying the real value by the total number of column families. To fix this, we should simply use `getLongProperty`, which queries a single column family (the default one). Since all column families share the same block cache, querying just one of them will give us the correct metrics for that shared block cache. Note: the same block cache is shared among all column families of a store irrespective of whether the user has configured a shared block cache across multiple stores.

cadonna · 2023-09-05T07:29:46Z

...s/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java

                        // BigInteger and construct the object from the byte representation of the value
                        result = new BigInteger(1, longToBytes(
-                            valueProvider.db.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)
+                            valueProvider.db.getLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)


This indeed seems a bug! Thanks for finding and fixing it!

cadonna · 2023-09-05T07:30:25Z

...s/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java

                        // BigInteger and construct the object from the byte representation of the value
                        result = result.add(new BigInteger(1, longToBytes(
-                            valueProvider.db.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)
+                            valueProvider.db.getLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)


How do you know that column families share the same block cache?

In this blog post, it says that:

Each column family can have its own block cache but may also be shared between column families or multiple database instances

We need to understand how we can discover both cases and compute the metric accordingly.

I think you are right about that we share the same cache across the two column families. We create and add a cache to an options object and pass the same option object to both column families. I am waiting for confirmation from the speedb chat.

Yeah, so block caches are configurable per-CF in both RocksDB and Speedb, but the specific implementation in TimestampedRocksDBStore always assigns the same block cache to both column families. In fact, the design of RocksDBStore makes it difficult to assign different block caches to column families.

The only way an implementation could have different block caches for different column families is if RocksDBStore was extended and overridden, such that the configuration of the TableFormatConfig is provided per-CF. In which case, such a custom implementation would necessitate a custom RocksDBMetricsRecorder, as it's tightly coupled to RocksDBStore anyway.

I got confirmation, the column families share the cache.

Btw: Maybe speedb-io/speedb#583 (comment) helps?

cadonna · 2023-09-05T10:36:13Z

@nicktelford Could you please add new unit tests for the case of multiple column families and adapt existing unit tests to your fix?

nicktelford · 2023-09-05T11:01:53Z

@nicktelford Could you please add new unit tests for the case of multiple column families and adapt existing unit tests to your fix?

No existing tests appear to be affected by this bug/change, but I can most likely add a test that verifies the bug and fix.

cadonna · 2023-09-05T12:03:39Z

I think this part of the unit tests should be affected since the called method changes:

kafka/streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderGaugesTest.java

Line 234 in fd6c9f1

    
           when(dbToAdd1.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue);

Our tests were verifying the use of `getAggregatedLongProperty`, when they should instead use `getLongProperty` for _only_ block cache metrics.

nicktelford · 2023-09-05T13:28:46Z

I think this part of the unit tests should be affected since the called method changes:

kafka/streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderGaugesTest.java

Line 234 in fd6c9f1

when(dbToAdd1.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue);

Woops! I must have missed those test failures. I've pushed an update to that test suite. Do you think this is sufficient or should there be more tests?

cadonna

@nicktelford Thanks for the update!

Could you also look why we did not catch this bug in RocksDBMetricsIntegrationTestor other metrics integration tests and add tests if needed?

.../java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderGaugesTest.java

nicktelford · 2023-09-05T14:08:24Z

@nicktelford Thanks for the update!

Could you also look why we did not catch this bug in RocksDBMetricsIntegrationTestor other metrics integration tests and add tests if needed?

@cadonna Looks like that test doesn't verify the value of those metrics; it only checks the number of metrics registered under each name (i.e. that the expected metrics are registered and available, but not what they are).

cadonna · 2023-09-05T15:45:56Z

@nicktelford Yes, you are right about the existing tests! Sorry I was in a hurry and only skimmed them and missed that fact.
I would like to have some tests that verify the behavior with one single column family (non-timestamped RocksDB) and multiple column families (timestamped RocksDB) so that we get a signal when something changes on RocksDB side and to kind of document the behavior.

Tests that `RocksDBStore` and `RocksDBTimestampedStore` produce the expected values in their block cache metrics. The test has been verified to fail without the fix provided by apache#14317, and pass with the fix applied. Note: the primary constructor of `RocksDBTimestampedStore` was made `public` to match the visibility of the same constructor on the parent `RocksDBStore`.

nicktelford · 2023-09-12T14:03:55Z

@cadonna I've added a test now that I think tests this suitably. Crucially, the test fails without this fix applied, and passes with this fix applied, as you would expect.

cadonna

Thanks for the updates, @nicktelford !

I have one ask.

For the rest the PR looks good to me!

cadonna · 2023-09-13T12:39:52Z

...test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBBlockCacheMetricsTest.java

+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;


We are migrating to JUnit 5. New tests should use JUnit 5. Could you please adapt the test?

I've pushed this change, although it's not particularly pretty: JUnit 5 doesn't have support for Parameterized tests where the test parameters are shared among all tests in the class. This issue is tracked here: junit-team/junit-framework#944

I suspect this might be an issue for various other tests in the codebase as they get migrated to JUnit 5.

For now, we have to setup and teardown the state store on every test method, which slows it down a bit.

cadonna

LGTM!

@nicktelford Thanks again!

All block cache metrics are being multiplied by the total number of column families. In a `RocksDBTimestampedStore`, we have 2 column families (the default, and the timestamped values), which causes all block cache metrics in these stores to become doubled. The cause is that our metrics recorder uses `getAggregatedLongProperty` to fetch block cache metrics. `getAggregatedLongProperty` queries the property on each column family in the database, and sums the results. Since we always configure all column families to share the same block cache, that causes the same block cache to be queried multiple times for its metrics, with the results added togehter, effectively multiplying the real value by the total number of column families. To fix this, we should simply use `getLongProperty`, which queries a single column family (the default one). Since all column families share the same block cache, querying just one of them will give us the correct metrics for that shared block cache. Note: the same block cache is shared among all column families of a store irrespective of whether the user has configured a shared block cache across multiple stores. Reviewers: Matthias J. Sax <matthias@confluent.io>, Bruno Cadonna <cadonna@apache.org>

mjsax · 2023-10-19T03:22:23Z

Cherry-picked this to 3.6 branch. Also tries to cherry-pick to 3.5 but the RocksDBMetricsRecorderGaugesTest test fails on 3.5 -- not sure why yet.

All block cache metrics are being multiplied by the total number of column families. In a `RocksDBTimestampedStore`, we have 2 column families (the default, and the timestamped values), which causes all block cache metrics in these stores to become doubled. The cause is that our metrics recorder uses `getAggregatedLongProperty` to fetch block cache metrics. `getAggregatedLongProperty` queries the property on each column family in the database, and sums the results. Since we always configure all column families to share the same block cache, that causes the same block cache to be queried multiple times for its metrics, with the results added togehter, effectively multiplying the real value by the total number of column families. To fix this, we should simply use `getLongProperty`, which queries a single column family (the default one). Since all column families share the same block cache, querying just one of them will give us the correct metrics for that shared block cache. Note: the same block cache is shared among all column families of a store irrespective of whether the user has configured a shared block cache across multiple stores. Reviewers: Matthias J. Sax <matthias@confluent.io>, Bruno Cadonna <cadonna@apache.org>

divijvaidya added the streams label Sep 1, 2023

cadonna reviewed Sep 5, 2023

View reviewed changes

Update tests for new behaviour

e2f08cf

Our tests were verifying the use of `getAggregatedLongProperty`, when they should instead use `getLongProperty` for _only_ block cache metrics.

cadonna reviewed Sep 5, 2023

View reviewed changes

.../java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderGaugesTest.java Outdated Show resolved Hide resolved

Simplify tests

355f0d8

mjsax mentioned this pull request Sep 8, 2023

Cannot delete default column family speedb-io/speedb#583

Closed

cadonna reviewed Sep 13, 2023

View reviewed changes

Use JUnit 5 for tests

9b44f72

cadonna approved these changes Sep 18, 2023

View reviewed changes

cadonna merged commit f041efa into apache:trunk Sep 18, 2023

nicktelford deleted the KAFKA-13973 branch September 18, 2023 16:25

cadonna mentioned this pull request Oct 19, 2023

MINOR: cherry pick KAFKA-13973 to 3.5 #14592

Closed

3 tasks

Conversation

nicktelford commented Aug 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna commented Sep 5, 2023

Uh oh!

nicktelford commented Sep 5, 2023

Uh oh!

cadonna commented Sep 5, 2023

Uh oh!

nicktelford commented Sep 5, 2023

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicktelford commented Sep 5, 2023

Uh oh!

cadonna commented Sep 5, 2023

Uh oh!

nicktelford commented Sep 12, 2023

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadonna left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mjsax commented Oct 19, 2023 •

edited

Loading