errose28 commented on code in PR #4337:
URL: https://github.com/apache/ozone/pull/4337#discussion_r1125053856
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/MutableVolumeSet.java:
##########
@@ -165,6 +165,8 @@ private void initializeVolumeSet() throws IOException {
}
for (String locationString : rawLocations) {
+ LOG.info("Start initializing raw location {}", locationString);
Review Comment:
Do we need this new log line? It looks like it would give the same info as
the log on line 176
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerController.java:
##########
@@ -196,6 +196,10 @@ public Iterator<Container<?>> getContainers() {
return containerSet.getContainerIterator();
}
+ public Iterable<Container<?>> getContainerSet() {
Review Comment:
Can we use the `getContainers` method above instead of adding a new method?
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/container/InspectSubcommand.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.debug.container;
+
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.container.common.impl.ContainerData;
+import org.apache.hadoop.ozone.container.common.interfaces.Container;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import
org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerMetadataInspector;
+import org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils;
+import org.apache.hadoop.ozone.container.metadata.DatanodeStore;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+import java.util.concurrent.Callable;
+
+/**
+ * {@code ozone debug container inspect},
+ * a command to run {@link KeyValueContainerMetadataInspector}.
+ */
+@Command(
+ name = "inspect",
+ description = "Check the deletion information for all the containers.")
Review Comment:
```suggestion
description = "Check the metadata of all container replicas on this
datanode")
```
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/container/InspectSubcommand.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.debug.container;
+
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.container.common.impl.ContainerData;
+import org.apache.hadoop.ozone.container.common.interfaces.Container;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
+import
org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerMetadataInspector;
+import org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils;
+import org.apache.hadoop.ozone.container.metadata.DatanodeStore;
+import picocli.CommandLine;
+import picocli.CommandLine.Command;
+
+import java.io.IOException;
+import java.util.concurrent.Callable;
+
+/**
+ * {@code ozone debug container inspect},
+ * a command to run {@link KeyValueContainerMetadataInspector}.
+ */
+@Command(
+ name = "inspect",
+ description = "Check the deletion information for all the containers.")
+public class InspectSubcommand implements Callable<Void> {
+
+ @CommandLine.ParentCommand
+ private ContainerCommands parent;
+
+ @Override
+ public Void call() throws IOException {
+ final OzoneConfiguration conf = parent.getOzoneConf();
+ parent.loadContainersFromVolumes();
+
+ final KeyValueContainerMetadataInspector inspector
+ = new KeyValueContainerMetadataInspector(
+ KeyValueContainerMetadataInspector.Mode.INSPECT);
+ for (Container<?> container : parent.getController().getContainerSet()) {
+ final ContainerData data = container.getContainerData();
+ if (!(data instanceof KeyValueContainerData)) {
+ continue;
+ }
+ final KeyValueContainerData kvData = (KeyValueContainerData) data;
+ try (DatanodeStore store = BlockUtils.getUncachedDatanodeStore(
+ kvData, conf, true)) {
+ final String json = inspector.process(kvData, store, null);
+ System.out.println(json);
Review Comment:
By printing to stdout instead of logging, we lose the ability to only
display containers with errors. I'm ok with this since changing log4j configs
isn't always intuitive. We could filter the incorrect containers with jq, or
maybe add a flag to only dump the containers that have errors and leave more
advanced filtering to jq.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -437,41 +497,107 @@ private JsonObject buildErrorAndRepair(String property,
JsonElement expected,
return error;
}
- private long countPendingDeletesSchemaV2(DatanodeStoreSchemaTwoImpl
- schemaTwoStore) throws IOException {
+ enum PendingDeleteType {
+ COUNT("pendingDeleteBlocks"),
+ BYTES("pendingDeleteBytes");
+
+ private final String jsonKey;
+
+ PendingDeleteType(String jsonKey) {
+ this.jsonKey = jsonKey;
+ }
+
+ String getJsonKey() {
+ return jsonKey;
+ }
+
+ void addProperty(EnumMap<PendingDeleteType, Long> map,
+ JsonObject json) {
+ Optional.of(map.get(this))
+ .ifPresent(bytes -> json.addProperty(getJsonKey(), bytes));
+ }
+
+ static EnumMap<PendingDeleteType, Long> newMap(long count, long bytes) {
+ final EnumMap<PendingDeleteType, Long> map
+ = new EnumMap<>(PendingDeleteType.class);
+ map.put(PendingDeleteType.COUNT, count);
+ map.put(PendingDeleteType.BYTES, bytes);
+ return map;
+ }
+ }
+
+ static EnumMap<PendingDeleteType, Long> countPendingDeletesSchemaV2(
+ DatanodeStoreSchemaTwoImpl schemaTwoStore,
+ KeyValueContainerData containerData) throws IOException {
long pendingDeleteBlockCountTotal = 0;
+ long pendingDeleteBytes = 0;
+
Table<Long, DeletedBlocksTransaction> delTxTable =
schemaTwoStore.getDeleteTransactionTable();
+ final Table<String, BlockData> blockDataTable
+ = schemaTwoStore.getBlockDataTable();
+
try (TableIterator<Long, ? extends Table.KeyValue<Long,
DeletedBlocksTransaction>> iterator = delTxTable.iterator()) {
while (iterator.hasNext()) {
DeletedBlocksTransaction txn = iterator.next().getValue();
+ final List<Long> localIDs = txn.getLocalIDList();
// In schema 2, pending delete blocks are stored in the
// transaction object. Since the actual blocks still exist in the
// block data table with no prefix, they have already been
// counted towards bytes used and total block count above.
- pendingDeleteBlockCountTotal += txn.getLocalIDList().size();
+ pendingDeleteBlockCountTotal += localIDs.size();
+ pendingDeleteBytes += computePendingDeleteBytes(
+ localIDs, containerData, blockDataTable);
}
}
- return pendingDeleteBlockCountTotal;
+ return PendingDeleteType.newMap(pendingDeleteBlockCountTotal,
+ pendingDeleteBytes);
+ }
+
+ static long computePendingDeleteBytes(List<Long> localIDs,
+ KeyValueContainerData containerData,
+ Table<String, BlockData> blockDataTable) {
+ long pendingDeleteBytes = 0;
+ for (long id : localIDs) {
+ try {
+ final String blockKey = containerData.getBlockKey(id);
+ final BlockData blockData = blockDataTable.get(blockKey);
+ if (blockData != null) {
+ pendingDeleteBytes += blockData.getSize();
+ }
+ } catch (Throwable t) {
Review Comment:
```suggestion
} catch (IOException ex) {
```
We should keep the exception handling as specific as possible.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -437,41 +497,107 @@ private JsonObject buildErrorAndRepair(String property,
JsonElement expected,
return error;
}
- private long countPendingDeletesSchemaV2(DatanodeStoreSchemaTwoImpl
- schemaTwoStore) throws IOException {
+ enum PendingDeleteType {
Review Comment:
Since this being used as the return type for each `countPendingDelete*`
method depending on the schema, can we simplify this to just contain two long
fields: one for pending delete block count and one for pending delete bytes,
and get rid of the enum map in `getAggregateValues`? That would be shorter and
easier to follow.
--
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]
