You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "nicktelford (via GitHub)" <gi...@apache.org> on 2023/08/31 11:22:04 UTC
[GitHub] [kafka] nicktelford opened a new pull request, #14317: KAFKA-13973: Fix inflated block cache metrics
nicktelford opened a new pull request, #14317:
URL: https://github.com/apache/kafka/pull/14317
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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on PR #14317:
URL: https://github.com/apache/kafka/pull/14317#issuecomment-1706873185
@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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] nicktelford commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "nicktelford (via GitHub)" <gi...@apache.org>.
nicktelford commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1315734550
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java:
##########
@@ -373,14 +373,14 @@ private Gauge<BigInteger> gaugeToComputeBlockCacheMetrics(final String propertyN
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
));
break;
} else {
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
Review Comment:
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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-13973: Fix inflated block cache metrics [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on PR #14317:
URL: https://github.com/apache/kafka/pull/14317#issuecomment-1769829743
Cherry-picked this to `3.6` branch. Also tries to cherry-pick to `3.5` but the newly added test fails on 3.5 -- not sure why yet.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1319266372
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java:
##########
@@ -373,14 +373,14 @@ private Gauge<BigInteger> gaugeToComputeBlockCacheMetrics(final String propertyN
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
));
break;
} else {
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
Review Comment:
Btw: Maybe https://github.com/speedb-io/speedb/issues/583#issuecomment-1616710762 helps?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1315706213
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java:
##########
@@ -373,14 +373,14 @@ private Gauge<BigInteger> gaugeToComputeBlockCacheMetrics(final String propertyN
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
));
break;
} else {
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
Review Comment:
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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] nicktelford commented on pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "nicktelford (via GitHub)" <gi...@apache.org>.
nicktelford commented on PR #14317:
URL: https://github.com/apache/kafka/pull/14317#issuecomment-1715792093
@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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1324450860
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBBlockCacheMetricsTest.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals.metrics;
+
+import org.apache.kafka.common.MetricName;
+import org.apache.kafka.common.metrics.KafkaMetric;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.streams.processor.StateStoreContext;
+import org.apache.kafka.streams.processor.TaskId;
+import org.apache.kafka.streams.processor.internals.ProcessorContextUtils;
+import org.apache.kafka.streams.processor.internals.metrics.StreamsMetricsImpl;
+import org.apache.kafka.streams.state.internals.BlockBasedTableConfigWithAccessibleCache;
+import org.apache.kafka.streams.state.internals.RocksDBStore;
+import org.apache.kafka.streams.state.internals.RocksDBTimestampedStore;
+import org.apache.kafka.test.MockInternalProcessorContext;
+import org.apache.kafka.test.TestUtils;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
Review Comment:
We are migrating to JUnit 5. New tests should use JUnit 5. Could you please adapt the test?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] nicktelford commented on pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "nicktelford (via GitHub)" <gi...@apache.org>.
nicktelford commented on PR #14317:
URL: https://github.com/apache/kafka/pull/14317#issuecomment-1706697411
> @nicktelford Thanks for the update!
>
> Could you also look why we did not catch this bug in `RocksDBMetricsIntegrationTest`or 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).
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna merged pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna merged PR #14317:
URL: https://github.com/apache/kafka/pull/14317
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on PR #14317:
URL: https://github.com/apache/kafka/pull/14317#issuecomment-1706367819
@nicktelford Could you please add new unit tests for the case of multiple column families and adapt existing unit tests to your fix?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1315491175
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java:
##########
@@ -373,14 +373,14 @@ private Gauge<BigInteger> gaugeToComputeBlockCacheMetrics(final String propertyN
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
));
break;
} else {
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
Review Comment:
In this [blog post](https://www.speedb.io/blog-posts/how-does-rocksdb-memory-management-work), 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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] nicktelford commented on pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "nicktelford (via GitHub)" <gi...@apache.org>.
nicktelford commented on PR #14317:
URL: https://github.com/apache/kafka/pull/14317#issuecomment-1706624785
> I think this part of the unit tests should be affected since the called method changes:
>
> https://github.com/apache/kafka/blob/fd6c9f16bacf26261fee2755c1d3679b9703a8a0/streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderGaugesTest.java#L234
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?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] nicktelford commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "nicktelford (via GitHub)" <gi...@apache.org>.
nicktelford commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1327598817
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBBlockCacheMetricsTest.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.streams.state.internals.metrics;
+
+import org.apache.kafka.common.MetricName;
+import org.apache.kafka.common.metrics.KafkaMetric;
+import org.apache.kafka.common.utils.Utils;
+import org.apache.kafka.streams.processor.StateStoreContext;
+import org.apache.kafka.streams.processor.TaskId;
+import org.apache.kafka.streams.processor.internals.ProcessorContextUtils;
+import org.apache.kafka.streams.processor.internals.metrics.StreamsMetricsImpl;
+import org.apache.kafka.streams.state.internals.BlockBasedTableConfigWithAccessibleCache;
+import org.apache.kafka.streams.state.internals.RocksDBStore;
+import org.apache.kafka.streams.state.internals.RocksDBTimestampedStore;
+import org.apache.kafka.test.MockInternalProcessorContext;
+import org.apache.kafka.test.TestUtils;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
Review Comment:
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: https://github.com/junit-team/junit5/issues/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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1315798587
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java:
##########
@@ -373,14 +373,14 @@ private Gauge<BigInteger> gaugeToComputeBlockCacheMetrics(final String propertyN
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
));
break;
} else {
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
Review Comment:
I got confirmation, the column families share the cache.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] nicktelford commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "nicktelford (via GitHub)" <gi...@apache.org>.
nicktelford commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1315955627
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderGaugesTest.java:
##########
@@ -211,14 +215,19 @@ private void runAndVerifySumOfProperties(final String propertyName) throws Excep
final long recordedValue1 = 5L;
final long recordedValue2 = 3L;
- when(dbToAdd1.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue1);
- when(dbToAdd2.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue2);
+ if (aggregated) {
+ when(dbToAdd1.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue1);
+ when(dbToAdd2.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue2);
+ } else {
+ when(dbToAdd1.getLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue1);
+ when(dbToAdd2.getLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue2);
+ }
Review Comment:
Done.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] nicktelford commented on pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "nicktelford (via GitHub)" <gi...@apache.org>.
nicktelford commented on PR #14317:
URL: https://github.com/apache/kafka/pull/14317#issuecomment-1706403793
> @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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1315913915
##########
streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderGaugesTest.java:
##########
@@ -211,14 +215,19 @@ private void runAndVerifySumOfProperties(final String propertyName) throws Excep
final long recordedValue1 = 5L;
final long recordedValue2 = 3L;
- when(dbToAdd1.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue1);
- when(dbToAdd2.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue2);
+ if (aggregated) {
+ when(dbToAdd1.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue1);
+ when(dbToAdd2.getAggregatedLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue2);
+ } else {
+ when(dbToAdd1.getLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue1);
+ when(dbToAdd2.getLongProperty(ROCKSDB_PROPERTIES_PREFIX + propertyName)).thenReturn(recordedValue2);
+ }
Review Comment:
I would move this part into `runAndVerifyBlockCacheMetricsWithMultipleCaches()`. Before we forwarded the call to this because the verifications were basically the same. That changed now, so I think it is cleaner to separate both.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #14317:
URL: https://github.com/apache/kafka/pull/14317#discussion_r1315487328
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java:
##########
@@ -373,14 +373,14 @@ private Gauge<BigInteger> gaugeToComputeBlockCacheMetrics(final String propertyN
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
));
break;
} else {
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
Review Comment:
How do you know that column families share the same block cache?
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorder.java:
##########
@@ -373,14 +373,14 @@ private Gauge<BigInteger> gaugeToComputeBlockCacheMetrics(final String propertyN
// values of RocksDB properties are of type unsigned long in C++, i.e., in Java we need to use
// 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)
Review Comment:
This indeed seems a bug! Thanks for finding and fixing it!
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on pull request #14317: KAFKA-13973: Fix inflated block cache metrics
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on PR #14317:
URL: https://github.com/apache/kafka/pull/14317#issuecomment-1706493373
I think this part of the unit tests should be affected since the called method changes:
https://github.com/apache/kafka/blob/fd6c9f16bacf26261fee2755c1d3679b9703a8a0/streams/src/test/java/org/apache/kafka/streams/state/internals/metrics/RocksDBMetricsRecorderGaugesTest.java#L234
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org