chihsuan commented on code in PR #10913: URL: https://github.com/apache/ozone/pull/10913#discussion_r3694450476
########## hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.hadoop.ozone.container.common.volume; + +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.annotation.Metrics; +import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem; +import org.apache.hadoop.metrics2.lib.Interns; +import org.apache.hadoop.metrics2.lib.MetricsRegistry; +import org.apache.hadoop.ozone.OzoneConsts; +import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport; + +/** + * Node-level storage totals for a DataNode, aggregated across all HDDS data + * volumes. Registered as {@code Hadoop:service=HddsDatanode,name=DatanodeStorageMetrics}. + * Reads {@link MutableVolumeSet#getStorageReport()} so values stay consistent + * with what the DataNode reports to SCM. + */ +@Metrics(about = "Ozone DataNode node-level storage totals", + context = OzoneConsts.OZONE) +public final class DatanodeStorageMetrics implements MetricsSource { + + static final String SOURCE_NAME = "DatanodeStorageMetrics"; + + private static final MetricsInfo CAPACITY = Interns.info("Capacity", + "Total Ozone usable capacity across the DataNode's data volumes (bytes," + + " post reserved-space adjustment)"); + private static final MetricsInfo USED = Interns.info("Used", + "Total Ozone used space across the DataNode's data volumes (bytes)"); Review Comment: These values represent Ozone capacity and usage rather than raw filesystem values. I wonder if we should name them `OzoneCapacity` and `OzoneUsed`, consistent with `VolumeInfoMetrics`? https://github.com/apache/ozone/blob/0c4c66e9f4b4b7bf84d6135235b2d0ae0d74dd92/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfoMetrics.java#L46-L49 ########## hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/DatanodeStorageMetrics.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.hadoop.ozone.container.common.volume; + +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.annotation.Metrics; +import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem; +import org.apache.hadoop.metrics2.lib.Interns; +import org.apache.hadoop.metrics2.lib.MetricsRegistry; +import org.apache.hadoop.ozone.OzoneConsts; +import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport; + +/** + * Node-level storage totals for a DataNode, aggregated across all HDDS data + * volumes. Registered as {@code Hadoop:service=HddsDatanode,name=DatanodeStorageMetrics}. + * Reads {@link MutableVolumeSet#getStorageReport()} so values stay consistent + * with what the DataNode reports to SCM. + */ +@Metrics(about = "Ozone DataNode node-level storage totals", + context = OzoneConsts.OZONE) +public final class DatanodeStorageMetrics implements MetricsSource { + + static final String SOURCE_NAME = "DatanodeStorageMetrics"; Review Comment: Nit: could this use `DatanodeStorageMetrics.class.getSimpleName()` for consistency? https://github.com/apache/ozone/blob/0c4c66e9f4b4b7bf84d6135235b2d0ae0d74dd92/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/VolumeInfoMetrics.java#L43-L44 ########## hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java: ########## @@ -156,6 +157,7 @@ public class OzoneContainer { private final ContainerMetrics metrics; private WitnessedContainerMetadataStore witnessedContainerMetadataStore; + private DatanodeStorageMetrics datanodeStorageMetrics; Review Comment: Nit: this field is initialized on every successful constructor path, so it can be final. The corresponding `null` check in `stop()` would then be unnecessary. ########## hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestDatanodeStorageMetrics.java: ########## @@ -0,0 +1,115 @@ +/* + * 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.hadoop.ozone.container.common.volume; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import org.apache.hadoop.metrics2.AbstractMetric; +import org.apache.hadoop.metrics2.impl.MetricsCollectorImpl; +import org.apache.hadoop.metrics2.impl.MetricsRecordImpl; +import org.apache.hadoop.ozone.container.common.impl.StorageLocationReport; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for {@link DatanodeStorageMetrics}. + * + * <p>Tests verify: + * <ul> + * <li>Correct aggregation of Capacity and Used across multiple volumes.</li> + * <li>Attribute names match exactly (guards the CSD context regex contract).</li> Review Comment: Curious what the `CSD context regex contract` is here? Could this be described more clearly? ########## hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java: ########## @@ -646,6 +650,9 @@ public void stop() { diskBalancerService.shutdown(); } recoveringContainerScrubbingService.shutdown(); + if (datanodeStorageMetrics != null) { + datanodeStorageMetrics.unregister(); + } Review Comment: This metrics source reads from `volumeSet`, but it is unregistered after `volumeSet.shutdown()`. Would it make sense to unregister it before shutting down? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
