You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/10/20 05:46:29 UTC
[GitHub] [ozone] duongkame opened a new pull request, #3865: HDDS-7025. Add cache metrics in OM.
duongkame opened a new pull request, #3865:
URL: https://github.com/apache/ozone/pull/3865
## What changes were proposed in this pull request?
Create table cache metrics. Initially, only the cache size is included.
https://issues.apache.org/jira/browse/HDDS-7025
## How was this patch tested?
Standard CI.
Also local test to ensure expected metrics are emitted to Prometheus.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongkame commented on a diff in pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
duongkame commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1005076786
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java:
##########
@@ -72,9 +73,9 @@
public TypedTable(
Table<byte[], byte[]> rawTable,
CodecRegistry codecRegistry, Class<KEY> keyType,
- Class<VALUE> valueType) throws IOException {
+ Class<VALUE> valueType, boolean statsEnabled) throws IOException {
Review Comment:
Passing parameters is always the most explicit way of communication between software components. I think what's missing here is a neat way of passing them like a Builder or Parameter object.
I think only the tables needing exposing metrics should have stats enabled initially. And that would avoid other tables from paying the cost of keeping the counters (mainly on the locks of `AutomicLong`.
On second thought, that cost may be small and eventually, we'll enable metrics for all tables. So stats should be always there and that leaves the Table interface intact.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongkame commented on a diff in pull request #3865: HDDS-7025. Add cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
duongkame commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1000972055
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java:
##########
@@ -24,10 +24,12 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongkame commented on a diff in pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
duongkame commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1008360085
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/TableCacheMetrics.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.hdds.utils;
+
+import org.apache.hadoop.hdds.utils.db.cache.CacheStats;
+import org.apache.hadoop.hdds.utils.db.cache.TableCache;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.MetricsSystem;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+
+/**
+ * This class emits table level cache metrics.
+ */
+public final class TableCacheMetrics implements MetricsSource {
+ private enum MetricsInfos implements MetricsInfo {
+ TableName("Table Name."),
+ Size("Size of the cache."),
+ HitCount("Number of time the lookup methods return a cached value."),
+ MissCount("Number of times the requested value is not in the cache."),
+ IterationCount("Number of times the table cache is iterated through.");
+
+ private final String desc;
+
+ MetricsInfos(String desc) {
+ this.desc = desc;
+ }
+
+ @Override
+ public String description() {
+ return desc;
+ }
+ }
+
+ public static final String SOURCE_NAME =
+ TableCacheMetrics.class.getSimpleName();
+
+ public static final String NAME = TableCacheMetrics.class.getSimpleName();
Review Comment:
Removed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3865: HDDS-7025. Add cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #3865:
URL: https://github.com/apache/ozone/pull/3865#issuecomment-1287168929
Should we wait for more metrics to show up in this PR or do you plan to add that in subsequent PRs?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
Galsza commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1004076171
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/TableCacheMetrics.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.hdds.utils;
+
+import org.apache.hadoop.hdds.utils.db.cache.CacheStats;
+import org.apache.hadoop.hdds.utils.db.cache.TableCache;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.MetricsSystem;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+
+/**
+ * This class emits table level cache metrics.
+ */
+public final class TableCacheMetrics implements MetricsSource {
+ private enum MetricsInfos implements MetricsInfo {
+ TableName("Table Name."),
+ Size("Size of the cache."),
+ HitCount("Number of time the lookup methods return a cached value."),
+ MissCount("Number of times the requested value is not in the cache."),
+ IterationCount("Number of times the table cache is iterated through.");
+
+ private final String desc;
+
+ MetricsInfos(String desc) {
+ this.desc = desc;
+ }
+
+ @Override
+ public String description() {
+ return desc;
+ }
+ }
+
+ public static final String SOURCE_NAME =
+ TableCacheMetrics.class.getSimpleName();
+
+ public static final String NAME = TableCacheMetrics.class.getSimpleName();
Review Comment:
This NAME constant is the same as SOURCE_NAME. NAME is not used, please remove 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongkame commented on pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
duongkame commented on PR #3865:
URL: https://github.com/apache/ozone/pull/3865#issuecomment-1287550275
> Should we wait for more metrics to show up in this PR or do you plan to add that in subsequent PRs?
Thanks for looking at this. I've added some more basic counters. I think it's quite sufficient to investigate any table cache related issue.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on a diff in pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1008586624
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -256,6 +258,7 @@ public class OmMetadataManagerImpl implements OMMetadataManager {
private final long omEpoch;
private Map<String, Table> tableMap = new HashMap<>();
+ private List<TableCacheMetrics> tableCacheMetrics = new LinkedList<>();
Review Comment:
Are we keeping the list to only `unregister()` later?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
Galsza commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1004077856
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/TableCacheMetrics.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.hdds.utils;
+
+import org.apache.hadoop.hdds.utils.db.cache.CacheStats;
+import org.apache.hadoop.hdds.utils.db.cache.TableCache;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.MetricsSystem;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+
+/**
+ * This class emits table level cache metrics.
+ */
+public final class TableCacheMetrics implements MetricsSource {
+ private enum MetricsInfos implements MetricsInfo {
+ TableName("Table Name."),
+ Size("Size of the cache."),
+ HitCount("Number of time the lookup methods return a cached value."),
+ MissCount("Number of times the requested value is not in the cache."),
+ IterationCount("Number of times the table cache is iterated through.");
+
+ private final String desc;
+
+ MetricsInfos(String desc) {
+ this.desc = desc;
+ }
+
+ @Override
+ public String description() {
+ return desc;
+ }
+ }
+
+ public static final String SOURCE_NAME =
+ TableCacheMetrics.class.getSimpleName();
+
+ public static final String NAME = TableCacheMetrics.class.getSimpleName();
+
+ private final TableCache<?, ?> cache;
+ private final String tableName;
+
+ private TableCacheMetrics(TableCache<?, ?> cache, String name) {
+ this.cache = cache;
+ this.tableName = name;
+ }
+
+ public static TableCacheMetrics create(TableCache<?, ?> cache,
+ String tableName) {
+ MetricsSystem ms = DefaultMetricsSystem.instance();
+ return ms.register(sourceName(tableName), "Table cache metrics",
+ new TableCacheMetrics(cache, tableName));
+ }
+
+ private static String sourceName(String tableName) {
Review Comment:
Nit: this function name is a bit misleading I'd suggest naming it getTableSourceName
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongkame commented on a diff in pull request #3865: HDDS-7025. Add cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
duongkame commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1000970099
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/TableCacheMetrics.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.hdds.utils;
+
+import org.apache.hadoop.hdds.utils.db.cache.TableCache;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.MetricsSystem;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+
+/**
+ * This class emits table level cache metrics.
+ */
+public class TableCacheMetrics implements MetricsSource {
Review Comment:
```suggestion
public final class TableCacheMetrics implements MetricsSource {
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongkame commented on a diff in pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
duongkame commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1009669536
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -256,6 +258,7 @@ public class OmMetadataManagerImpl implements OMMetadataManager {
private final long omEpoch;
private Map<String, Table> tableMap = new HashMap<>();
+ private List<TableCacheMetrics> tableCacheMetrics = new LinkedList<>();
Review Comment:
Yes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime merged pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
kerneltime merged PR #3865:
URL: https://github.com/apache/ozone/pull/3865
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] Galsza commented on a diff in pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
Galsza commented on code in PR #3865:
URL: https://github.com/apache/ozone/pull/3865#discussion_r1004225798
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java:
##########
@@ -72,9 +73,9 @@
public TypedTable(
Table<byte[], byte[]> rawTable,
CodecRegistry codecRegistry, Class<KEY> keyType,
- Class<VALUE> valueType) throws IOException {
+ Class<VALUE> valueType, boolean statsEnabled) throws IOException {
Review Comment:
Have you considered other ways to avoid passing statsEnabled all the way through these methods? I'm not sure there is an easier solution, but maybe creating multiple methods instead of passing the boolean would be clearer.
Another thing to consider is it's not clear now (at least to me) when the statsEnabled should be true or false.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] duongkame commented on pull request #3865: HDDS-7025. Add table cache metrics in OM.
Posted by GitBox <gi...@apache.org>.
duongkame commented on PR #3865:
URL: https://github.com/apache/ozone/pull/3865#issuecomment-1297411026
> The changes look good to me. I have one question though, Why can't the `TableCacheMetrics` be part of the `TypedTable` instance itself?
Thanks @aswinshakil for the review. Basically, I think it's better not to extend the `TypedTable` responsibility to maintain TypeTable complexity. Metrics should belong to the clients of `TypedTable` instead.
Yet, I had to add a metrics creator function in `TypedTable` because the cache instance is not (and should not) be visible outside.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org