You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/09/15 17:09:26 UTC
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2900: Update "du" command to compute disk usage by scanning the metadata table
ctubbsii commented on code in PR #2900:
URL: https://github.com/apache/accumulo/pull/2900#discussion_r972228117
##########
core/src/main/java/org/apache/accumulo/core/client/admin/DiskUsage.java:
##########
@@ -21,10 +21,13 @@
import java.util.Objects;
import java.util.SortedSet;
+/**
+ * This class is used to track the shared disk usage between multiple tables.
+ */
public class DiskUsage {
- protected final SortedSet<String> tables;
- protected Long usage;
+ private final SortedSet<String> tables;
+ private final Long usage;
Review Comment:
I'm not sure why we use the boxed version of Long here instead of the primitive. That could be fixed throughout this class. Not sure if we depend on null references anywhere... but if we do, they could be replaced with OptionalLong instead. This comment is not necessarily related to this PR's changes. Just an observation.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TableDiskUsageResult.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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
+ *
+ * https://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.accumulo.core.client.admin;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.SortedMap;
+import java.util.SortedSet;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.stream.Collectors;
+
+import org.apache.accumulo.core.data.TableId;
+
+import com.google.common.collect.ImmutableMap;
+
+/**
+ * Class for returning the disk usage results computed by
+ * {@link org.apache.accumulo.core.util.tables.MetadataTableDiskUsage}
+ */
+public class TableDiskUsageResult {
+
+ // Track the total disk usage across all files for the Table
+ private final Map<TableId,AtomicLong> tableUsages;
+
+ // Track the total disk usage across a volume for each Table
+ private final Map<TableId,Map<String,AtomicLong>> volumeUsages;
+
+ // This tracks disk usage shared across a set of tables
+ private final List<DiskUsage> sharedDiskUsages;
+
+ // This tracks any shared tables (tables that have shared files)
+ private final Map<TableId,Set<TableId>> sharedTables;
+
+ public TableDiskUsageResult(final Map<TableId,AtomicLong> tableUsages,
+ final Map<TableId,Map<String,AtomicLong>> volumeUsages,
+ final SortedMap<SortedSet<String>,Long> usageMap,
+ final Map<TableId,Set<TableId>> referencedTables) {
+
+ this.tableUsages = ImmutableMap.copyOf(tableUsages);
+ this.volumeUsages = ImmutableMap.copyOf(volumeUsages);
+ this.sharedTables = ImmutableMap.copyOf(referencedTables);
Review Comment:
These don't need to use Guava's ImmutableMap. They can use the built-in Map.copyOf
##########
server/base/src/main/java/org/apache/accumulo/server/util/HdfsTableDiskUsage.java:
##########
@@ -61,94 +60,16 @@
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Scope;
-public class TableDiskUsage {
-
- private static final Logger log = LoggerFactory.getLogger(TableDiskUsage.class);
- private int nextInternalId = 0;
- private Map<TableId,Integer> internalIds = new HashMap<>();
- private Map<Integer,TableId> externalIds = new HashMap<>();
- private Map<String,Integer[]> tableFiles = new HashMap<>();
- private Map<String,Long> fileSizes = new HashMap<>();
-
- void addTable(TableId tableId) {
- if (internalIds.containsKey(tableId))
- throw new IllegalArgumentException("Already added table " + tableId);
-
- // Keep an internal counter for each table added
- int iid = nextInternalId++;
-
- // Store the table id to the internal id
- internalIds.put(tableId, iid);
- // Store the internal id to the table id
- externalIds.put(iid, tableId);
- }
-
- void linkFileAndTable(TableId tableId, String file) {
- // get the internal id for this table
- int internalId = internalIds.get(tableId);
-
- // Initialize a bitset for tables (internal IDs) that reference this file
- Integer[] tables = tableFiles.get(file);
- if (tables == null) {
- tables = new Integer[internalIds.size()];
- for (int i = 0; i < tables.length; i++)
- tables[i] = 0;
- tableFiles.put(file, tables);
- }
-
- // Update the bitset to track that this table has seen this file
- tables[internalId] = 1;
- }
-
- void addFileSize(String file, long size) {
- fileSizes.put(file, size);
- }
-
- Map<List<TableId>,Long> calculateUsage() {
-
- // Bitset of tables that contain a file and total usage by all files that share that usage
- Map<List<Integer>,Long> usage = new HashMap<>();
-
- if (log.isTraceEnabled()) {
- log.trace("fileSizes {}", fileSizes);
- }
- // For each file w/ referenced-table bitset
- for (Entry<String,Integer[]> entry : tableFiles.entrySet()) {
- if (log.isTraceEnabled()) {
- log.trace("file {} table bitset {}", entry.getKey(), Arrays.toString(entry.getValue()));
- }
- List<Integer> key = Arrays.asList(entry.getValue());
- Long size = fileSizes.get(entry.getKey());
-
- Long tablesUsage = usage.get(key);
- if (tablesUsage == null)
- tablesUsage = 0L;
-
- tablesUsage += size;
-
- usage.put(key, tablesUsage);
-
- }
-
- Map<List<TableId>,Long> externalUsage = new HashMap<>();
-
- for (Entry<List<Integer>,Long> entry : usage.entrySet()) {
- List<TableId> externalKey = new ArrayList<>();
- List<Integer> key = entry.getKey();
- // table bitset
- for (int i = 0; i < key.size(); i++)
- if (key.get(i) != 0)
- // Convert by internal id to the table id
- externalKey.add(externalIds.get(i));
-
- // list of table ids and size of files shared across the tables
- externalUsage.put(externalKey, entry.getValue());
- }
+/**
+ * Class that computes shared disk usage across tables by scanning HDFS files.
+ *
+ * @deprecated This class has been deprecated in favor of
+ * {@link org.apache.accumulo.core.util.tables.MetadataTableDiskUsage}
+ */
+@Deprecated
+public class HdfsTableDiskUsage extends TableDiskUsage {
Review Comment:
I don't think we need subclasses for disk usage. That seems unnecessarily complex. I think when we get the list of files, we can just keep a pair instead of a set... the pair including the filename and the size. Then, get the size from that mapping, rather than lookup in HDFS.
I think a lot of the complexity of this PR is trying to preserve both old and new code... and that's not necessary.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java:
##########
@@ -1082,10 +1082,40 @@ Map<String,Integer> listConstraints(String tableName)
* a set of tables
* @return a list of disk usage objects containing linked table names and sizes
* @since 1.6.0
+ *
+ * @deprecated since 2.1.0 use {@link #getEstimatedDiskUsage(Set, boolean, Authorizations)}
+ * instead.
*/
+ @Deprecated(since = "2.1.0")
List<DiskUsage> getDiskUsage(Set<String> tables)
throws AccumuloException, AccumuloSecurityException, TableNotFoundException;
+ /**
+ * Gets the number of bytes being used in by the files for a set of tables. This operation will
+ * use the client to scan the metadata table to compute the size metrics for the tables.
+ * Optionally shared usage information can be computed across tables.
+ *
+ * Because the metadata table is used for computing usage and not the actual files in HDFS the
+ * results will be an estimate. Older entries may exist with no file metadata (resulting in size
+ * 0) and other actions in the cluster can impact the estimated size such as flushes, tablet
+ * splits, compactions, etc.
+ *
+ * For the most accurate information a compaction should first be run on the set of tables being
+ * computed.
+ *
+ * @param tables
+ * set of tables to compute usage across
+ * @param computeShared
+ * whether to compute size metrics across shared files
+ * @param auths
+ * authorizations to scan the metadata table
+ * @return The disk usage result
+ *
+ * @since 2.1.0
+ */
+ TableDiskUsageResult getEstimatedDiskUsage(Set<String> tables, boolean computeShared,
+ Authorizations auths) throws TableNotFoundException;
Review Comment:
I don't think we need this to be a new API. We can just change the existing implementation to get the sizes from a different source. As long as the size is reliable, it doesn't matter if we get it from HDFS or the metadata.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/DUCommand.java:
##########
@@ -80,13 +109,53 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
}
try {
- String valueFormat = prettyPrint ? "%9s" : "%,24d";
- for (DiskUsage usage : shellState.getAccumuloClient().tableOperations()
- .getDiskUsage(tables)) {
- Object value = prettyPrint ? NumUtil.bigNumberForSize(usage.getUsage()) : usage.getUsage();
- shellState.getWriter()
- .println(String.format(valueFormat + " %s", value, usage.getTables()));
+ final String valueFormat = prettyPrint ? "%s" : "%,d";
+ final TableDiskUsageResult usageResult = shellState.getAccumuloClient().tableOperations()
+ .getEstimatedDiskUsage(tables, verbose, auths);
+
+ // Print results for each table queried
+ for (Map.Entry<TableId,AtomicLong> entry : usageResult.getTableUsages().entrySet()) {
+ final Object usage =
+ prettyPrint ? NumUtil.bigNumberForSize(entry.getValue().get()) : entry.getValue().get();
+
+ // Print the usage for the table
+ shellState.getWriter().print(String.format("%s %s used total: " + valueFormat,
+ shellState.getContext().getTableName(entry.getKey()), entry.getKey(), usage));
+
+ // Print usage for all volume(s) that contain files for this table
+ Optional.ofNullable(usageResult.getVolumeUsages().get(entry.getKey()))
+ .ifPresent(tableVolUsage -> tableVolUsage.entrySet()
+ .forEach(vuEntry -> shellState.getWriter()
+ .print(String.format(" %s: " + valueFormat, vuEntry.getKey(),
+ prettyPrint ? NumUtil.bigNumberForSize(vuEntry.getValue().get())
+ : vuEntry.getValue().get()))));
+
+ // Check/print if this table has any shared files
+ final Optional<Set<TableId>> sharedTables =
+ Optional.ofNullable(usageResult.getSharedTables().get(entry.getKey()));
+ final boolean hasShared = sharedTables.map(st -> st.size() > 0).orElse(false);
+
+ shellState.getWriter().print(String.format(" has_shared: %s", hasShared));
+ if (hasShared) {
+ shellState.getWriter().print(String.format(" shared with: %s", sharedTables.get()));
+ }
+ shellState.getWriter().println();
Review Comment:
I don't think it's necessary to change this shell command at all. The only differences should be in the API method's implementation, and maybe this command's description to emphasize it's an estimate. None of the printing of the output needs to be changed at all to achieve the desired goal of this PR, to get size info from the metadata.
##########
core/src/main/java/org/apache/accumulo/core/util/tables/TableDiskUsage.java:
##########
@@ -0,0 +1,180 @@
+/*
+ * 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
+ *
+ * https://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.accumulo.core.util.tables;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.SortedMap;
+import java.util.SortedSet;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+import org.apache.accumulo.core.clientImpl.ClientContext;
+import org.apache.accumulo.core.data.TableId;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public abstract class TableDiskUsage {
+
+ private static final Logger log = LoggerFactory.getLogger(TableDiskUsage.class);
+ private int nextInternalId = 0;
+ private Map<TableId,Integer> internalIds = new HashMap<>();
+ private Map<Integer,TableId> externalIds = new HashMap<>();
+ private Map<String,Integer[]> tableFiles = new HashMap<>();
+ private Map<String,Long> fileSizes = new HashMap<>();
+
+ protected void addTableIfAbsent(TableId tableId) {
+ if (!internalIds.containsKey(tableId)) {
+ addTable(tableId);
+ }
+ }
+
+ protected void addTable(TableId tableId) {
+ if (internalIds.containsKey(tableId)) {
+ throw new IllegalArgumentException("Already added table " + tableId);
+ }
+
+ // Keep an internal counter for each table added
+ int iid = nextInternalId++;
+
+ // Store the table id to the internal id
+ internalIds.put(tableId, iid);
+ // Store the internal id to the table id
+ externalIds.put(iid, tableId);
+ }
+
+ protected void linkFileAndTable(TableId tableId, String file) {
+ // get the internal id for this table
+ int internalId = internalIds.get(tableId);
+
+ // Initialize a bitset for tables (internal IDs) that reference this file
+ Integer[] tables = tableFiles.get(file);
+ if (tables == null) {
+ tables = new Integer[internalIds.size()];
+ for (int i = 0; i < tables.length; i++) {
+ tables[i] = 0;
+ }
+ tableFiles.put(file, tables);
+ }
+
+ // Update the bitset to track that this table has seen this file
+ tables[internalId] = 1;
+ }
+
+ protected void addFileSize(String file, long size) {
+ fileSizes.put(file, size);
+ }
+
+ protected Map<List<TableId>,Long> calculateSharedUsage() {
+ // Bitset of tables that contain a file and total usage by all files that share that usage
+ Map<List<Integer>,Long> usage = new HashMap<>();
+
+ if (log.isTraceEnabled()) {
+ log.trace("fileSizes {}", fileSizes);
+ }
+ // For each file w/ referenced-table bitset
+ for (Entry<String,Integer[]> entry : tableFiles.entrySet()) {
+ if (log.isTraceEnabled()) {
+ log.trace("file {} table bitset {}", entry.getKey(), Arrays.toString(entry.getValue()));
+ }
+ List<Integer> key = Arrays.asList(entry.getValue());
+ Long size = fileSizes.get(entry.getKey());
+
+ Long tablesUsage = usage.getOrDefault(key, 0L);
+ tablesUsage += size;
+ usage.put(key, tablesUsage);
+ }
+
+ final Map<List<TableId>,Long> externalUsage = new HashMap<>();
+
+ for (Entry<List<Integer>,Long> entry : usage.entrySet()) {
+ List<TableId> externalKey = new ArrayList<>();
+ List<Integer> key = entry.getKey();
+ // table bitset
+ for (int i = 0; i < key.size(); i++)
+ if (key.get(i) != 0) {
+ // Convert by internal id to the table id
+ externalKey.add(externalIds.get(i));
+ }
+
+ // list of table ids and size of files shared across the tables
+ externalUsage.put(externalKey, entry.getValue());
+ }
+
+ // mapping of all enumerations of files being referenced by tables and total size of files who
+ // share the same reference
+ return externalUsage;
+ }
+
+ protected static SortedMap<SortedSet<String>,Long> buildSharedUsageMap(final TableDiskUsage tdu,
+ final ClientContext clientContext, final Set<TableId> emptyTableIds) {
+ final Map<TableId,String> reverseTableIdMap = clientContext.getTableIdToNameMap();
+
+ final SortedMap<SortedSet<String>,Long> usage = new TreeMap<>((o1, o2) -> {
+ int len1 = o1.size();
+ int len2 = o2.size();
+
+ int min = Math.min(len1, len2);
+ Iterator<String> iter1 = o1.iterator();
+ Iterator<String> iter2 = o2.iterator();
+
+ int count = 0;
+ while (count < min) {
+ String s1 = iter1.next();
+ String s2 = iter2.next();
+
+ int cmp = s1.compareTo(s2);
+ if (cmp != 0) {
+ return cmp;
+ }
+
+ count++;
+ }
+
+ return len1 - len2;
+ });
Review Comment:
This comparator seems to be non-trivial, but has no comments explaining what it's trying to do
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org