From b5153306711e4fd01dfcbd5c1adfa690a198a1c9 Mon Sep 17 00:00:00 2001 From: Mehakmeet Singh Date: Thu, 30 Apr 2020 19:20:18 +0530 Subject: [PATCH 1/6] HADOOP-17016. Adding Common Counters in ABFS --- .../fs/azurebfs/AbfsInstrumentation.java | 272 +++++++++++++++ .../hadoop/fs/azurebfs/AbfsStatistic.java | 93 ++++++ .../fs/azurebfs/AzureBlobFileSystem.java | 71 +++- .../fs/azurebfs/ITestAbfsStatistics.java | 309 ++++++++++++++++++ .../fs/azurebfs/TestAbfsStatistics.java | 66 ++++ 5 files changed, 803 insertions(+), 8 deletions(-) create mode 100644 hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java create mode 100644 hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsStatistic.java create mode 100644 hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java create mode 100644 hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java new file mode 100644 index 0000000000000..f270bcfb76d11 --- /dev/null +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java @@ -0,0 +1,272 @@ +/** + * 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.fs.azurebfs; + +import java.net.URI; +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; + +import com.google.common.annotations.VisibleForTesting; + +import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.metrics2.AbstractMetric; +import org.apache.hadoop.metrics2.MetricStringBuilder; +import org.apache.hadoop.metrics2.MetricsCollector; +import org.apache.hadoop.metrics2.MetricsInfo; +import org.apache.hadoop.metrics2.MetricsRecordBuilder; +import org.apache.hadoop.metrics2.MetricsTag; +import org.apache.hadoop.metrics2.lib.MetricsRegistry; +import org.apache.hadoop.metrics2.lib.MutableCounterLong; +import org.apache.hadoop.metrics2.lib.MutableMetric; + +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.*; + +/** + * Instrumentation of Abfs counters. + */ +@InterfaceStability.Unstable +public class AbfsInstrumentation { + + /** + * Single context for all the Abfs counters to separate them from other + * counters. + */ + private static final String CONTEXT = "AbfsContext"; + /** + * The name of a field added to metrics records that uniquely identifies a + * specific FileSystem instance. + */ + private static final String REGISTRY_ID = "AbfsID"; + /** + * The name of a field added to metrics records that indicates the hostname + * portion of the FS URL. + */ + private static final String METRIC_BUCKET = "AbfsBucket"; + + private final MetricsRegistry registry = + new MetricsRegistry("abfsMetrics").setContext(CONTEXT); + + private static final AbfsStatistic[] STATISTIC_LIST = { + CALL_CREATE, + CALL_OPEN, + CALL_GET_FILE_STATUS, + CALL_APPEND, + CALL_CREATE_NON_RECURSIVE, + CALL_DELETE, + CALL_EXIST, + CALL_GET_DELEGATION_TOKEN, + CALL_LIST_STATUS, + CALL_MKDIRS, + CALL_RENAME, + DIRECTORIES_CREATED, + DIRECTORIES_DELETED, + FILES_CREATED, + FILES_DELETED, + ERROR_IGNORED + }; + + public AbfsInstrumentation(URI uri) { + UUID fileSystemInstanceId = UUID.randomUUID(); + registry.tag(REGISTRY_ID, + "A unique identifier for the instance", + fileSystemInstanceId.toString()); + registry.tag(METRIC_BUCKET, "Hostname from the FS URL", uri.getHost()); + + for (AbfsStatistic stats : STATISTIC_LIST) { + createCounter(stats); + } + } + + /** + * Look up a Metric from Registered set. + * + * @param name name of metric. + * @return the metric or null. + */ + private MutableMetric lookupMetric(String name) { + MutableMetric metric = getRegistry().get(name); + return metric; + } + + /** + * Look up counter by name. + * + * @param name name of counter. + * @return counter if found, else null. + */ + private MutableCounterLong lookupCounter(String name) { + MutableMetric metric = lookupMetric(name); + if (metric == null) { + return null; + } + if (!(metric instanceof MutableCounterLong)) { + throw new IllegalStateException("Metric " + name + + " is not a MutableCounterLong: " + metric); + } + return (MutableCounterLong) metric; + } + + /** + * Create a counter in the registry. + * + * @param stats AbfsStatistic whose counter needs to be made. + * @return counter or null. + */ + private MutableCounterLong createCounter(AbfsStatistic stats) { + return registry.newCounter(stats.getStatName(), + stats.getStatDescription(), 0L); + } + + /** + * Increment a Statistic with some value. + * + * @param statistic AbfsStatistic need to be incremented. + * @param value long value to be incremented by. + */ + protected void incrementStat(AbfsStatistic statistic, long value) { + MutableCounterLong counter = lookupCounter(statistic.getStatName()); + if (counter != null) { + counter.incr(value); + } + } + + /** + * Getter for MetricRegistry. + * + * @return MetricRegistry or null. + */ + private MetricsRegistry getRegistry() { + return registry; + } + + /** + * Method to aggregate all the counters in the MetricRegistry and form a + * string with prefix, separator and suffix. + * + * @param prefix string that would be before metric. + * @param separator string that would be between metric name and value. + * @param suffix string that would be after metric value. + * @param all gets all the values even if unchanged. + * @return a String with all the metrics and their values. + */ + protected String formString(String prefix, String separator, String suffix, + boolean all) { + + MetricStringBuilder metricStringBuilder = new MetricStringBuilder(null, + prefix, separator, suffix); + registry.snapshot(metricStringBuilder, all); + return metricStringBuilder.toString(); + } + + /** + * Creating a map of all the counters for testing. + * + * @return a map of the metrics. + */ + @VisibleForTesting + protected Map toMap() { + MetricsToMap metricBuilder = new MetricsToMap(null); + registry.snapshot(metricBuilder, true); + return metricBuilder.getMap(); + } + + protected static class MetricsToMap extends MetricsRecordBuilder { + private final MetricsCollector parent; + private final Map map = + new HashMap<>(); + + MetricsToMap(MetricsCollector parent) { + this.parent = parent; + } + + @Override + public MetricsRecordBuilder tag(MetricsInfo info, String value) { + return this; + } + + @Override + public MetricsRecordBuilder add(MetricsTag tag) { + return this; + } + + @Override + public MetricsRecordBuilder add(AbstractMetric metric) { + return this; + } + + @Override + public MetricsRecordBuilder setContext(String value) { + return this; + } + + @Override + public MetricsRecordBuilder addCounter(MetricsInfo info, int value) { + return tuple(info, value); + } + + @Override + public MetricsRecordBuilder addCounter(MetricsInfo info, long value) { + return tuple(info, value); + } + + @Override + public MetricsRecordBuilder addGauge(MetricsInfo info, int value) { + return tuple(info, value); + } + + @Override + public MetricsRecordBuilder addGauge(MetricsInfo info, long value) { + return tuple(info, value); + } + + public MetricsToMap tuple(MetricsInfo info, long value) { + return tuple(info.name(), value); + } + + public MetricsToMap tuple(String name, long value) { + map.put(name, value); + return this; + } + + @Override + public MetricsRecordBuilder addGauge(MetricsInfo info, float value) { + return tuple(info, (long) value); + } + + @Override + public MetricsRecordBuilder addGauge(MetricsInfo info, double value) { + return tuple(info, (long) value); + } + + @Override + public MetricsCollector parent() { + return parent; + } + + /** + * Get the map. + * + * @return the map of metrics. + */ + public Map getMap() { + return map; + } + } +} diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsStatistic.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsStatistic.java new file mode 100644 index 0000000000000..1b144c37e23b9 --- /dev/null +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsStatistic.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 + * + * 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.fs.azurebfs; + +import org.apache.hadoop.fs.StorageStatistics.CommonStatisticNames; + +/** + * Statistic which are collected in Abfs. + * Available as metrics in {@link AbfsInstrumentation}. + */ +public enum AbfsStatistic { + + CALL_CREATE(CommonStatisticNames.OP_CREATE, + "Calls of create()."), + CALL_OPEN(CommonStatisticNames.OP_OPEN, + "Calls of open()."), + CALL_GET_FILE_STATUS(CommonStatisticNames.OP_GET_FILE_STATUS, + "Calls of getFileStatus()."), + CALL_APPEND(CommonStatisticNames.OP_APPEND, + "Calls of append()."), + CALL_CREATE_NON_RECURSIVE(CommonStatisticNames.OP_CREATE_NON_RECURSIVE, + "Calls of createNonRecursive()."), + CALL_DELETE(CommonStatisticNames.OP_DELETE, + "Calls of delete()."), + CALL_EXIST(CommonStatisticNames.OP_EXISTS, + "Calls of exist()."), + CALL_GET_DELEGATION_TOKEN(CommonStatisticNames.OP_GET_DELEGATION_TOKEN, + "Calls of getDelegationToken()."), + CALL_LIST_STATUS(CommonStatisticNames.OP_LIST_STATUS, + "Calls of listStatus()."), + CALL_MKDIRS(CommonStatisticNames.OP_MKDIRS, + "Calls of mkdirs()."), + CALL_RENAME(CommonStatisticNames.OP_RENAME, + "Calls of rename()."), + DIRECTORIES_CREATED("directories_created", + "Total number of directories created through the object store."), + DIRECTORIES_DELETED("directories_deleted", + "Total number of directories deleted through the object store."), + FILES_CREATED("files_created", + "Total number of files created through the object store."), + FILES_DELETED("files_deleted", + "Total number of files deleted from the object store."), + ERROR_IGNORED("error_ignored", + "Errors caught and ignored."); + + private String statName; + private String statDescription; + + /** + * Constructor of AbfsStatistic to set statistic name and description. + * + * @param statName Name of the statistic. + * @param statDescription Description of the statistic. + */ + AbfsStatistic(String statName, String statDescription) { + this.statName = statName; + this.statDescription = statDescription; + } + + /** + * Getter for Statistic Name. + * + * @return Name of Statistic. + */ + public String getStatName() { + return statName; + } + + /** + * Getter for Statistic Description. + * + * @return Description of Statistic. + */ + public String getStatDescription() { + return statDescription; + } +} diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java index 4ddc2e396c4de..2d34f1f7f2c47 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java @@ -78,6 +78,7 @@ import org.apache.hadoop.security.UserGroupInformation; import org.apache.hadoop.util.Progressable; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.*; import static org.apache.hadoop.fs.impl.PathCapabilitiesSupport.validatePathCapabilityArgs; /** @@ -94,6 +95,7 @@ public class AzureBlobFileSystem extends FileSystem { private boolean delegationTokenEnabled = false; private AbfsDelegationTokenManager delegationTokenManager; + private AbfsInstrumentation instrumentation; @Override public void initialize(URI uri, Configuration configuration) @@ -109,7 +111,7 @@ public void initialize(URI uri, Configuration configuration) LOG.trace("AzureBlobFileSystemStore init complete"); final AbfsConfiguration abfsConfiguration = abfsStore.getAbfsConfiguration(); - + instrumentation = new AbfsInstrumentation(uri); this.setWorkingDirectory(this.getHomeDirectory()); if (abfsConfiguration.getCreateRemoteFileSystemDuringInitialization()) { @@ -146,6 +148,11 @@ public String toString() { sb.append("uri=").append(uri); sb.append(", user='").append(abfsStore.getUser()).append('\''); sb.append(", primaryUserGroup='").append(abfsStore.getPrimaryGroup()).append('\''); + if (instrumentation != null) { + sb.append(", Statistics: {").append(instrumentation.formString("{", "=", + "}", true)); + sb.append("}"); + } sb.append('}'); return sb.toString(); } @@ -162,7 +169,7 @@ public URI getUri() { @Override public FSDataInputStream open(final Path path, final int bufferSize) throws IOException { LOG.debug("AzureBlobFileSystem.open path: {} bufferSize: {}", path, bufferSize); - + statIncrement(CALL_OPEN); Path qualifiedPath = makeQualified(path); try { @@ -183,6 +190,7 @@ public FSDataOutputStream create(final Path f, final FsPermission permission, fi overwrite, blockSize); + statIncrement(CALL_CREATE); trailingPeriodCheck(f); Path qualifiedPath = makeQualified(f); @@ -190,6 +198,7 @@ public FSDataOutputStream create(final Path f, final FsPermission permission, fi try { OutputStream outputStream = abfsStore.createFile(qualifiedPath, statistics, overwrite, permission == null ? FsPermission.getFileDefault() : permission, FsPermission.getUMask(getConf())); + statIncrement(FILES_CREATED); return new FSDataOutputStream(outputStream, statistics); } catch(AzureBlobFileSystemException ex) { checkException(f, ex); @@ -203,6 +212,7 @@ public FSDataOutputStream createNonRecursive(final Path f, final FsPermission pe final boolean overwrite, final int bufferSize, final short replication, final long blockSize, final Progressable progress) throws IOException { + statIncrement(CALL_CREATE_NON_RECURSIVE); final Path parent = f.getParent(); final FileStatus parentFileStatus = tryGetFileStatus(parent); @@ -246,7 +256,7 @@ public FSDataOutputStream append(final Path f, final int bufferSize, final Progr "AzureBlobFileSystem.append path: {} bufferSize: {}", f.toString(), bufferSize); - + statIncrement(CALL_APPEND); Path qualifiedPath = makeQualified(f); try { @@ -261,7 +271,7 @@ public FSDataOutputStream append(final Path f, final int bufferSize, final Progr public boolean rename(final Path src, final Path dst) throws IOException { LOG.debug( "AzureBlobFileSystem.rename src: {} dst: {}", src.toString(), dst.toString()); - + statIncrement(CALL_RENAME); trailingPeriodCheck(dst); Path parentFolder = src.getParent(); @@ -328,7 +338,7 @@ public boolean rename(final Path src, final Path dst) throws IOException { public boolean delete(final Path f, final boolean recursive) throws IOException { LOG.debug( "AzureBlobFileSystem.delete path: {} recursive: {}", f.toString(), recursive); - + statIncrement(CALL_DELETE); Path qualifiedPath = makeQualified(f); if (f.isRoot()) { @@ -353,7 +363,7 @@ public boolean delete(final Path f, final boolean recursive) throws IOException public FileStatus[] listStatus(final Path f) throws IOException { LOG.debug( "AzureBlobFileSystem.listStatus path: {}", f.toString()); - + statIncrement(CALL_LIST_STATUS); Path qualifiedPath = makeQualified(f); try { @@ -365,6 +375,25 @@ public FileStatus[] listStatus(final Path f) throws IOException { } } + /** + * Increment of an Abfs statistic. + * + * @param statistic AbfsStatistic that needs increment. + */ + private void statIncrement(AbfsStatistic statistic) { + incrementStatistic(statistic, 1); + } + + /** + * Method for incrementing AbfsStatistic by a long value. + * + * @param statistic the Statistic to be incremented. + * @param value value to be incremented. + */ + private void incrementStatistic(AbfsStatistic statistic, long value) { + instrumentation.incrementStat(statistic, value); + } + /** * Performs a check for (.) until root in the path to throw an exception. * The purpose is to differentiate between dir/dir1 and dir/dir1. @@ -394,7 +423,7 @@ private void trailingPeriodCheck(Path path) throws IllegalArgumentException { public boolean mkdirs(final Path f, final FsPermission permission) throws IOException { LOG.debug( "AzureBlobFileSystem.mkdirs path: {} permissions: {}", f, permission); - + statIncrement(CALL_MKDIRS); trailingPeriodCheck(f); final Path parentFolder = f.getParent(); @@ -408,6 +437,7 @@ public boolean mkdirs(final Path f, final FsPermission permission) throws IOExce try { abfsStore.createDirectory(qualifiedPath, permission == null ? FsPermission.getDirDefault() : permission, FsPermission.getUMask(getConf())); + statIncrement(DIRECTORIES_CREATED); return true; } catch (AzureBlobFileSystemException ex) { checkException(f, ex, AzureServiceErrorCode.PATH_ALREADY_EXISTS); @@ -425,12 +455,13 @@ public synchronized void close() throws IOException { LOG.debug("AzureBlobFileSystem.close"); IOUtils.cleanupWithLogger(LOG, abfsStore, delegationTokenManager); this.isClosed = true; + LOG.debug("Closing Abfs: " + toString()); } @Override public FileStatus getFileStatus(final Path f) throws IOException { LOG.debug("AzureBlobFileSystem.getFileStatus path: {}", f); - + statIncrement(CALL_GET_FILE_STATUS); Path qualifiedPath = makeQualified(f); try { @@ -567,6 +598,11 @@ private boolean deleteRoot() throws IOException { @Override public Void call() throws Exception { delete(fs.getPath(), fs.isDirectory()); + if (fs.isDirectory()) { + statIncrement(DIRECTORIES_DELETED); + } else { + statIncrement(FILES_DELETED); + } return null; } }); @@ -926,11 +962,23 @@ public void access(final Path path, final FsAction mode) throws IOException { } } + /** + * Incrementing exists() calls from superclass for Statistic collection. + * @param f source path + * @return true if the path exists + * @throws IOException + */ + @Override public boolean exists(Path f) throws IOException { + statIncrement(CALL_EXIST); + return super.exists(f); + } + private FileStatus tryGetFileStatus(final Path f) { try { return getFileStatus(f); } catch (IOException ex) { LOG.debug("File not found {}", f); + statIncrement(ERROR_IGNORED); return null; } } @@ -947,6 +995,7 @@ private boolean fileSystemExists() throws IOException { // there is not way to get the storage error code // workaround here is to check its status code. } catch (FileNotFoundException e) { + statIncrement(ERROR_IGNORED); return false; } } @@ -1120,6 +1169,7 @@ private Throwable getRootCause(Throwable throwable) { */ @Override public synchronized Token getDelegationToken(final String renewer) throws IOException { + statIncrement(CALL_GET_DELEGATION_TOKEN); return this.delegationTokenEnabled ? this.delegationTokenManager.getDelegationToken(renewer) : super.getDelegationToken(renewer); } @@ -1182,6 +1232,11 @@ boolean getIsNamespaceEnabled() throws AzureBlobFileSystemException { return abfsStore.getIsNamespaceEnabled(); } + @VisibleForTesting + AbfsInstrumentation getInstrumentation() { + return instrumentation; + } + @Override public boolean hasPathCapability(final Path path, final String capability) throws IOException { diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java new file mode 100644 index 0000000000000..d9e7bf7c919ff --- /dev/null +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java @@ -0,0 +1,309 @@ +/** + * 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.fs.azurebfs; + +import java.io.IOException; +import java.util.Map; + +import org.junit.Test; + +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.permission.FsPermission; + +/** + * Tests AzureBlobFileSystem Statistics. + */ +public class ITestAbfsStatistics extends AbstractAbfsIntegrationTest { + + private static final int NUMBER_OF_OPS = 10; + + public ITestAbfsStatistics() throws Exception { + } + + /** + * Testing statistics by creating files and directories. + */ + @Test + public void testCreateStatistics() throws IOException { + describe("Testing counter values got by creating directories and files in" + + " Abfs"); + + AzureBlobFileSystem fs = getFileSystem(); + Path createFilePath = path(getMethodName()); + Path createDirectoryPath = path(getMethodName() + "Dir"); + + Map metricMap = fs.getInstrumentation().toMap(); + + /* + Test for initial values of create statistics ; getFileStatus is called + 1 time after Abfs initialisation. + */ + assertEquals("Mismatch in op_create", 0, + (long) metricMap.get("op_create")); + assertEquals("Mismatch in op_create_non_recursive", 0, + (long) metricMap.get("op_create_non_recursive")); + assertEquals("Mismatch in files_created", 0, + (long) metricMap.get("files_created")); + assertEquals("Mismatch in directories_created", 0, + (long) metricMap.get("directories_created")); + assertEquals("Mismatch in op_mkdirs", 0, + (long) metricMap.get("op_mkdirs")); + assertEquals("Mismatch in op_get_file_status", 1, + (long) metricMap.get("op_get_file_status")); + + try { + + fs.mkdirs(createDirectoryPath); + fs.createNonRecursive(createFilePath, FsPermission + .getDefault(), false, 1024, (short) 1, 1024, null); + + metricMap = fs.getInstrumentation().toMap(); + /* + Test of statistic values after creating a directory and a file ; + getFileStatus is called 1 time after creating file and 1 time at + time of initialising. + */ + assertEquals("Mismatch in op_create", 1, + (long) metricMap.get("op_create")); + assertEquals("Mismatch in op_create_non_recursive", 1, + (long) metricMap.get("op_create_non_recursive")); + assertEquals("Mismatch in files_created", 1, + (long) metricMap.get("files_created")); + assertEquals("Mismatch in directories_created", 1, + (long) metricMap.get("directories_created")); + assertEquals("Mismatch in op_mkdirs", 1, + (long) metricMap.get("op_mkdirs")); + assertEquals("Mismatch in op_get_file_status", 2, + (long) metricMap.get("op_get_file_status")); + + } finally { + fs.close(); + } + + //re-initialising Abfs to reset statistic values. + fs.initialize(fs.getUri(), fs.getConf()); + + try { + /*Creating 10 directories and files; Directories and files can't + be created with same name, hence + i to give unique names. + */ + for (int i = 0; i < NUMBER_OF_OPS; i++) { + fs.mkdirs(path(getMethodName() + "Dir" + i)); + fs.createNonRecursive(path(getMethodName() + i), + FsPermission.getDefault(), false, 1024, (short) 1, + 1024, null); + } + + metricMap = fs.getInstrumentation().toMap(); + /* + Test of statistics values after creating 10 directories and files; + getFileStatus is called 1 time at initialise() plus number of + times file is created. + */ + assertEquals("Mismatch in op_create", NUMBER_OF_OPS, + (long) metricMap.get("op_create")); + assertEquals("Mismatch in op_create_non_recursive", NUMBER_OF_OPS, + (long) metricMap.get("op_create_non_recursive")); + assertEquals("Mismatch in files_created", NUMBER_OF_OPS, + (long) metricMap.get("files_created")); + assertEquals("Mismatch in directories_created", NUMBER_OF_OPS, + (long) metricMap.get("directories_created")); + assertEquals("Mismatch in op_mkdirs", NUMBER_OF_OPS, + (long) metricMap.get("op_mkdirs")); + assertEquals("Mismatch in op_get_file_status", 1 + NUMBER_OF_OPS, + (long) metricMap.get("op_get_file_status")); + + } finally { + fs.close(); + } + + } + + /** + * Testing statistics by deleting files and directories. + */ + @Test + public void testDeleteStatistics() throws IOException { + describe("Testing counter values got by deleting directory and files " + + "in Abfs"); + AzureBlobFileSystem fs = getFileSystem(); + /* + This directory path needs to be root for triggering the + directories_deleted counter. + */ + Path createDirectoryPath = path("/"); + Path createFilePath = path(getMethodName()); + + Map metricMap = fs.getInstrumentation().toMap(); + //Test for initial statistic values before deleting any directory or files + assertEquals("Mismatch in op_delete", 0, + (long) metricMap.get("op_delete")); + assertEquals("Mismatch in files_deleted", 0, + (long) metricMap.get("files_deleted")); + assertEquals("Mismatch in directories_deleted", 0, + (long) metricMap.get("directories_deleted")); + assertEquals("Mismatch in op_list_status", 0, + (long) metricMap.get("op_list_status")); + + try { + /* + creating a directory and a file inside that directory. + The directory is root. Hence, no parent. This allows us to invoke + deleteRoot() method to see the population of directories_deleted and + files_deleted counters. + */ + fs.mkdirs(createDirectoryPath); + fs.create(path(createDirectoryPath + getMethodName())); + fs.delete(createDirectoryPath, true); + + metricMap = fs.getInstrumentation().toMap(); + + /* + Test for op_delete, files_deleted, op_list_status. + since directory is delete recursively op_delete is called 2 times. + 1 file is deleted, 1 listStatus() call is made. + */ + assertEquals("Mismatch in op_delete", 2, + (long) metricMap.get("op_delete")); + assertEquals("Mismatch in files_deleted", 1, + (long) metricMap.get("files_deleted")); + assertEquals("Mismatch in op_list_status", 1, + (long) metricMap.get("op_list_status")); + + /* + creating a root directory and deleting it recursively to see if + directories_deleted is called or not. + */ + fs.mkdirs(createDirectoryPath); + fs.create(createFilePath); + fs.delete(createDirectoryPath, true); + metricMap = fs.getInstrumentation().toMap(); + + // Test for directories_deleted. + assertEquals("Mismatch in directories_deleted", 1, + (long) metricMap.get("directories_deleted")); + + } finally { + fs.close(); + } + + } + + /** + * Testing statistics of open, append, rename and exists method calls. + */ + @Test + public void testOpenAppendRenameExists() throws IOException { + describe("Testing counter values on calling open, append and rename and " + + "exists methods on Abfs"); + + AzureBlobFileSystem fs = getFileSystem(); + Path createFilePath = path(getMethodName()); + Path destCreateFilePath = path(getMethodName()+"New"); + + Map metricMap = fs.getInstrumentation().toMap(); + + //Tests for initial values of counters before any method is called. + assertEquals("Mismatch in op_open", 0, + (long) metricMap.get("op_open")); + assertEquals("Mismatch in op_append", 0, + (long) metricMap.get("op_append")); + assertEquals("Mismatch in op_rename", 0, + (long) metricMap.get("op_rename")); + assertEquals("Mismatch in op_exists", 0, + (long) metricMap.get("op_exists")); + + try { + fs.create(createFilePath); + fs.open(createFilePath); + fs.append(createFilePath); + fs.rename(createFilePath, destCreateFilePath); + + metricMap = fs.getInstrumentation().toMap(); + //Testing single method calls to open, append and rename. + assertEquals("Mismatch in op_open", 1, + (long) metricMap.get("op_open")); + assertEquals("Mismatch in op_append", 1, + (long) metricMap.get("op_append")); + assertEquals("Mismatch in op_rename", 1, + (long) metricMap.get("op_rename")); + + //Testing if file exists at path. + assertTrue("Mismatch in file Path existing", + fs.exists(destCreateFilePath)); + assertFalse("Mismatch in file Path existing", fs.exists(createFilePath)); + + metricMap = fs.getInstrumentation().toMap(); + //Testing exists() calls. + assertEquals("Mismatch in op_exists", 2, + (long) metricMap.get("op_exists")); + + } finally { + fs.close(); + } + + //re-initialising Abfs to reset statistic values. + fs.initialize(fs.getUri(), fs.getConf()); + + try { + + fs.create(destCreateFilePath); + + for (int i = 0; i < NUMBER_OF_OPS; i++) { + fs.open(destCreateFilePath); + fs.append(destCreateFilePath); + } + + metricMap = fs.getInstrumentation().toMap(); + + //Testing large number of method calls to open, append + assertEquals("Mismatch in op_open", NUMBER_OF_OPS, + (long) metricMap.get("op_open")); + assertEquals("Mismatch in op_append", NUMBER_OF_OPS, + (long) metricMap.get("op_append")); + + for (int i = 0; i < NUMBER_OF_OPS; i++) { + // rename and then back to earlier name for no error while looping. + fs.rename(destCreateFilePath, createFilePath); + fs.rename(createFilePath, destCreateFilePath); + + //check if first name is existing and 2nd is not existing. + assertTrue("Mismatch in file Path existing", + fs.exists(destCreateFilePath)); + assertFalse("Mismatch in file Path existing", + fs.exists(createFilePath)); + + } + + metricMap = fs.getInstrumentation().toMap(); + System.out.println(metricMap); + + /*Testing exists() calls and rename calls. Since both were called 2 + times in 1 loop. 2*numberOfOps is expectedValue. */ + assertEquals("Mismatch in op_rename", 2 * NUMBER_OF_OPS, + (long) metricMap.get("op_rename")); + assertEquals("Mismatch in op_exists", 2 * NUMBER_OF_OPS, + (long) metricMap.get("op_exists")); + + } finally { + fs.close(); + } + + } +} diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java new file mode 100644 index 0000000000000..a39e378d867eb --- /dev/null +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java @@ -0,0 +1,66 @@ +/** + * 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.fs.azurebfs; + +import java.io.IOException; +import java.util.Map; + +import org.junit.Test; + +public class TestAbfsStatistics extends AbstractAbfsIntegrationTest { + + private static final int LARGE_OPS = 100; + + public TestAbfsStatistics() throws Exception { + } + + /** + * Tests for op_get_delegation_token and error_ignore counter values. + */ + @Test + public void testInitialiseStats() throws IOException { + describe("Testing the counter values after Abfs is initialised"); + + AbfsInstrumentation instrumentation = + new AbfsInstrumentation(getFileSystem().getUri()); + Map metricMap = instrumentation.toMap(); + + System.out.println(metricMap); + + //Testing the statistics values at initial stage. + assertEquals("Mismatch in op_get_delegation_token", 0, + (long) metricMap.get("op_get_delegation_token")); + assertEquals("Mismatch in error_ignored", 0, + (long) metricMap.get("error_ignored")); + + //Testing summation of the counter values. + for (int i = 0; i < LARGE_OPS; i++) { + instrumentation.incrementStat(AbfsStatistic.CALL_GET_DELEGATION_TOKEN, 1); + instrumentation.incrementStat(AbfsStatistic.ERROR_IGNORED, 1); + } + + metricMap = instrumentation.toMap(); + + assertEquals("Mismatch in op_get_delegation_token", LARGE_OPS, + (long) metricMap.get("op_get_delegation_token")); + assertEquals("Mismatch in error_ignored", LARGE_OPS, + (long) metricMap.get("error_ignored")); + + } +} From 41d08b149a668c5104f7bf3f122d91845d24b3be Mon Sep 17 00:00:00 2001 From: Mehakmeet Singh Date: Thu, 30 Apr 2020 23:05:23 +0530 Subject: [PATCH 2/6] HADOOP-17016. Self-review comments fix --- .../fs/azurebfs/AbfsInstrumentation.java | 4 +- .../hadoop/fs/azurebfs/AbfsStatistic.java | 8 +- .../fs/azurebfs/AzureBlobFileSystem.java | 7 +- .../fs/azurebfs/ITestAbfsStatistics.java | 229 ++++++++---------- 4 files changed, 110 insertions(+), 138 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java index f270bcfb76d11..628ae7d668a07 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java @@ -95,7 +95,7 @@ public AbfsInstrumentation(URI uri) { } /** - * Look up a Metric from Registered set. + * Look up a Metric from registered set. * * @param name name of metric. * @return the metric or null. @@ -135,7 +135,7 @@ private MutableCounterLong createCounter(AbfsStatistic stats) { } /** - * Increment a Statistic with some value. + * Increment a statistic with some value. * * @param statistic AbfsStatistic need to be incremented. * @param value long value to be incremented by. diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsStatistic.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsStatistic.java index 1b144c37e23b9..a9867aa12b85e 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsStatistic.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsStatistic.java @@ -74,18 +74,18 @@ public enum AbfsStatistic { } /** - * Getter for Statistic Name. + * Getter for statistic name. * - * @return Name of Statistic. + * @return Name of statistic. */ public String getStatName() { return statName; } /** - * Getter for Statistic Description. + * Getter for statistic description. * - * @return Description of Statistic. + * @return Description of statistic. */ public String getStatDescription() { return statDescription; diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java index 2d34f1f7f2c47..8f705ac4f3545 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java @@ -963,9 +963,10 @@ public void access(final Path path, final FsAction mode) throws IOException { } /** - * Incrementing exists() calls from superclass for Statistic collection. - * @param f source path - * @return true if the path exists + * Incrementing exists() calls from superclass for statistic collection. + * + * @param f source path. + * @return true if the path exists. * @throws IOException */ @Override public boolean exists(Path f) throws IOException { diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java index d9e7bf7c919ff..571fee31a0a2d 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java @@ -67,71 +67,60 @@ public void testCreateStatistics() throws IOException { assertEquals("Mismatch in op_get_file_status", 1, (long) metricMap.get("op_get_file_status")); - try { + fs.mkdirs(createDirectoryPath); + fs.createNonRecursive(createFilePath, FsPermission + .getDefault(), false, 1024, (short) 1, 1024, null); - fs.mkdirs(createDirectoryPath); - fs.createNonRecursive(createFilePath, FsPermission - .getDefault(), false, 1024, (short) 1, 1024, null); - - metricMap = fs.getInstrumentation().toMap(); + metricMap = fs.getInstrumentation().toMap(); /* Test of statistic values after creating a directory and a file ; getFileStatus is called 1 time after creating file and 1 time at time of initialising. */ - assertEquals("Mismatch in op_create", 1, - (long) metricMap.get("op_create")); - assertEquals("Mismatch in op_create_non_recursive", 1, - (long) metricMap.get("op_create_non_recursive")); - assertEquals("Mismatch in files_created", 1, - (long) metricMap.get("files_created")); - assertEquals("Mismatch in directories_created", 1, - (long) metricMap.get("directories_created")); - assertEquals("Mismatch in op_mkdirs", 1, - (long) metricMap.get("op_mkdirs")); - assertEquals("Mismatch in op_get_file_status", 2, - (long) metricMap.get("op_get_file_status")); - - } finally { - fs.close(); - } + assertEquals("Mismatch in op_create", 1, + (long) metricMap.get("op_create")); + assertEquals("Mismatch in op_create_non_recursive", 1, + (long) metricMap.get("op_create_non_recursive")); + assertEquals("Mismatch in files_created", 1, + (long) metricMap.get("files_created")); + assertEquals("Mismatch in directories_created", 1, + (long) metricMap.get("directories_created")); + assertEquals("Mismatch in op_mkdirs", 1, + (long) metricMap.get("op_mkdirs")); + assertEquals("Mismatch in op_get_file_status", 2, + (long) metricMap.get("op_get_file_status")); //re-initialising Abfs to reset statistic values. fs.initialize(fs.getUri(), fs.getConf()); - try { /*Creating 10 directories and files; Directories and files can't be created with same name, hence + i to give unique names. */ - for (int i = 0; i < NUMBER_OF_OPS; i++) { - fs.mkdirs(path(getMethodName() + "Dir" + i)); - fs.createNonRecursive(path(getMethodName() + i), - FsPermission.getDefault(), false, 1024, (short) 1, - 1024, null); - } - - metricMap = fs.getInstrumentation().toMap(); + for (int i = 0; i < NUMBER_OF_OPS; i++) { + fs.mkdirs(path(getMethodName() + "Dir" + i)); + fs.createNonRecursive(path(getMethodName() + i), + FsPermission.getDefault(), false, 1024, (short) 1, + 1024, null); + } + + metricMap = fs.getInstrumentation().toMap(); /* Test of statistics values after creating 10 directories and files; getFileStatus is called 1 time at initialise() plus number of times file is created. */ - assertEquals("Mismatch in op_create", NUMBER_OF_OPS, - (long) metricMap.get("op_create")); - assertEquals("Mismatch in op_create_non_recursive", NUMBER_OF_OPS, - (long) metricMap.get("op_create_non_recursive")); - assertEquals("Mismatch in files_created", NUMBER_OF_OPS, - (long) metricMap.get("files_created")); - assertEquals("Mismatch in directories_created", NUMBER_OF_OPS, - (long) metricMap.get("directories_created")); - assertEquals("Mismatch in op_mkdirs", NUMBER_OF_OPS, - (long) metricMap.get("op_mkdirs")); - assertEquals("Mismatch in op_get_file_status", 1 + NUMBER_OF_OPS, - (long) metricMap.get("op_get_file_status")); - - } finally { - fs.close(); - } + assertEquals("Mismatch in op_create", NUMBER_OF_OPS, + (long) metricMap.get("op_create")); + assertEquals("Mismatch in op_create_non_recursive", NUMBER_OF_OPS, + (long) metricMap.get("op_create_non_recursive")); + assertEquals("Mismatch in files_created", NUMBER_OF_OPS, + (long) metricMap.get("files_created")); + assertEquals("Mismatch in directories_created", NUMBER_OF_OPS, + (long) metricMap.get("directories_created")); + assertEquals("Mismatch in op_mkdirs", NUMBER_OF_OPS, + (long) metricMap.get("op_mkdirs")); + assertEquals("Mismatch in op_get_file_status", 1 + NUMBER_OF_OPS, + (long) metricMap.get("op_get_file_status")); } @@ -161,48 +150,42 @@ public void testDeleteStatistics() throws IOException { assertEquals("Mismatch in op_list_status", 0, (long) metricMap.get("op_list_status")); - try { /* creating a directory and a file inside that directory. The directory is root. Hence, no parent. This allows us to invoke deleteRoot() method to see the population of directories_deleted and files_deleted counters. */ - fs.mkdirs(createDirectoryPath); - fs.create(path(createDirectoryPath + getMethodName())); - fs.delete(createDirectoryPath, true); + fs.mkdirs(createDirectoryPath); + fs.create(path(createDirectoryPath + getMethodName())); + fs.delete(createDirectoryPath, true); - metricMap = fs.getInstrumentation().toMap(); + metricMap = fs.getInstrumentation().toMap(); /* Test for op_delete, files_deleted, op_list_status. since directory is delete recursively op_delete is called 2 times. 1 file is deleted, 1 listStatus() call is made. */ - assertEquals("Mismatch in op_delete", 2, - (long) metricMap.get("op_delete")); - assertEquals("Mismatch in files_deleted", 1, - (long) metricMap.get("files_deleted")); - assertEquals("Mismatch in op_list_status", 1, - (long) metricMap.get("op_list_status")); + assertEquals("Mismatch in op_delete", 2, + (long) metricMap.get("op_delete")); + assertEquals("Mismatch in files_deleted", 1, + (long) metricMap.get("files_deleted")); + assertEquals("Mismatch in op_list_status", 1, + (long) metricMap.get("op_list_status")); /* creating a root directory and deleting it recursively to see if directories_deleted is called or not. */ - fs.mkdirs(createDirectoryPath); - fs.create(createFilePath); - fs.delete(createDirectoryPath, true); - metricMap = fs.getInstrumentation().toMap(); - - // Test for directories_deleted. - assertEquals("Mismatch in directories_deleted", 1, - (long) metricMap.get("directories_deleted")); - - } finally { - fs.close(); - } + fs.mkdirs(createDirectoryPath); + fs.create(createFilePath); + fs.delete(createDirectoryPath, true); + metricMap = fs.getInstrumentation().toMap(); + // Test for directories_deleted. + assertEquals("Mismatch in directories_deleted", 1, + (long) metricMap.get("directories_deleted")); } /** @@ -215,7 +198,7 @@ public void testOpenAppendRenameExists() throws IOException { AzureBlobFileSystem fs = getFileSystem(); Path createFilePath = path(getMethodName()); - Path destCreateFilePath = path(getMethodName()+"New"); + Path destCreateFilePath = path(getMethodName() + "New"); Map metricMap = fs.getInstrumentation().toMap(); @@ -229,81 +212,69 @@ public void testOpenAppendRenameExists() throws IOException { assertEquals("Mismatch in op_exists", 0, (long) metricMap.get("op_exists")); - try { - fs.create(createFilePath); - fs.open(createFilePath); - fs.append(createFilePath); - fs.rename(createFilePath, destCreateFilePath); - - metricMap = fs.getInstrumentation().toMap(); - //Testing single method calls to open, append and rename. - assertEquals("Mismatch in op_open", 1, - (long) metricMap.get("op_open")); - assertEquals("Mismatch in op_append", 1, - (long) metricMap.get("op_append")); - assertEquals("Mismatch in op_rename", 1, - (long) metricMap.get("op_rename")); + fs.create(createFilePath); + fs.open(createFilePath); + fs.append(createFilePath); + fs.rename(createFilePath, destCreateFilePath); - //Testing if file exists at path. - assertTrue("Mismatch in file Path existing", - fs.exists(destCreateFilePath)); - assertFalse("Mismatch in file Path existing", fs.exists(createFilePath)); + metricMap = fs.getInstrumentation().toMap(); + //Testing single method calls to open, append and rename. + assertEquals("Mismatch in op_open", 1, + (long) metricMap.get("op_open")); + assertEquals("Mismatch in op_append", 1, + (long) metricMap.get("op_append")); + assertEquals("Mismatch in op_rename", 1, + (long) metricMap.get("op_rename")); - metricMap = fs.getInstrumentation().toMap(); - //Testing exists() calls. - assertEquals("Mismatch in op_exists", 2, - (long) metricMap.get("op_exists")); + //Testing if file exists at path. + assertTrue("Mismatch in file Path existing", + fs.exists(destCreateFilePath)); + assertFalse("Mismatch in file Path existing", fs.exists(createFilePath)); - } finally { - fs.close(); - } + metricMap = fs.getInstrumentation().toMap(); + //Testing exists() calls. + assertEquals("Mismatch in op_exists", 2, + (long) metricMap.get("op_exists")); //re-initialising Abfs to reset statistic values. fs.initialize(fs.getUri(), fs.getConf()); - try { - - fs.create(destCreateFilePath); + fs.create(destCreateFilePath); - for (int i = 0; i < NUMBER_OF_OPS; i++) { - fs.open(destCreateFilePath); - fs.append(destCreateFilePath); - } + for (int i = 0; i < NUMBER_OF_OPS; i++) { + fs.open(destCreateFilePath); + fs.append(destCreateFilePath); + } - metricMap = fs.getInstrumentation().toMap(); + metricMap = fs.getInstrumentation().toMap(); - //Testing large number of method calls to open, append - assertEquals("Mismatch in op_open", NUMBER_OF_OPS, - (long) metricMap.get("op_open")); - assertEquals("Mismatch in op_append", NUMBER_OF_OPS, - (long) metricMap.get("op_append")); + //Testing large number of method calls to open, append. + assertEquals("Mismatch in op_open", NUMBER_OF_OPS, + (long) metricMap.get("op_open")); + assertEquals("Mismatch in op_append", NUMBER_OF_OPS, + (long) metricMap.get("op_append")); - for (int i = 0; i < NUMBER_OF_OPS; i++) { - // rename and then back to earlier name for no error while looping. - fs.rename(destCreateFilePath, createFilePath); - fs.rename(createFilePath, destCreateFilePath); + for (int i = 0; i < NUMBER_OF_OPS; i++) { + // rename and then back to earlier name for no error while looping. + fs.rename(destCreateFilePath, createFilePath); + fs.rename(createFilePath, destCreateFilePath); - //check if first name is existing and 2nd is not existing. - assertTrue("Mismatch in file Path existing", - fs.exists(destCreateFilePath)); - assertFalse("Mismatch in file Path existing", - fs.exists(createFilePath)); + //check if first name is existing and 2nd is not existing. + assertTrue("Mismatch in file Path existing", + fs.exists(destCreateFilePath)); + assertFalse("Mismatch in file Path existing", + fs.exists(createFilePath)); - } + } - metricMap = fs.getInstrumentation().toMap(); - System.out.println(metricMap); + metricMap = fs.getInstrumentation().toMap(); /*Testing exists() calls and rename calls. Since both were called 2 times in 1 loop. 2*numberOfOps is expectedValue. */ - assertEquals("Mismatch in op_rename", 2 * NUMBER_OF_OPS, - (long) metricMap.get("op_rename")); - assertEquals("Mismatch in op_exists", 2 * NUMBER_OF_OPS, - (long) metricMap.get("op_exists")); - - } finally { - fs.close(); - } + assertEquals("Mismatch in op_rename", 2 * NUMBER_OF_OPS, + (long) metricMap.get("op_rename")); + assertEquals("Mismatch in op_exists", 2 * NUMBER_OF_OPS, + (long) metricMap.get("op_exists")); } } From 38d05899acc7a19b84c65da429b751aa802d79d9 Mon Sep 17 00:00:00 2001 From: Mehakmeet Singh Date: Tue, 12 May 2020 16:49:10 +0530 Subject: [PATCH 3/6] HADOOP-17016. review comments fix --- .../fs/azurebfs/AbfsInstrumentation.java | 20 +- .../fs/azurebfs/AzureBlobFileSystem.java | 20 +- .../fs/azurebfs/ITestAbfsStatistics.java | 277 +++++++++--------- .../fs/azurebfs/TestAbfsStatistics.java | 28 +- 4 files changed, 194 insertions(+), 151 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java index 628ae7d668a07..21bab6ab7b5d0 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java @@ -36,7 +36,22 @@ import org.apache.hadoop.metrics2.lib.MutableCounterLong; import org.apache.hadoop.metrics2.lib.MutableMetric; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.*; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_APPEND; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_CREATE; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_CREATE_NON_RECURSIVE; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_DELETE; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_EXIST; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_DELEGATION_TOKEN; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_FILE_STATUS; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_LIST_STATUS; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_MKDIRS; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_OPEN; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_RENAME; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.DIRECTORIES_CREATED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.DIRECTORIES_DELETED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.ERROR_IGNORED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.FILES_CREATED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.FILES_DELETED; /** * Instrumentation of Abfs counters. @@ -101,8 +116,7 @@ public AbfsInstrumentation(URI uri) { * @return the metric or null. */ private MutableMetric lookupMetric(String name) { - MutableMetric metric = getRegistry().get(name); - return metric; + return getRegistry().get(name); } /** diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java index 8f705ac4f3545..b1883d40fa44a 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java @@ -78,7 +78,22 @@ import org.apache.hadoop.security.UserGroupInformation; import org.apache.hadoop.util.Progressable; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.*; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_APPEND; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_CREATE; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_CREATE_NON_RECURSIVE; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_DELETE; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_EXIST; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_DELEGATION_TOKEN; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_FILE_STATUS; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_LIST_STATUS; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_MKDIRS; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_OPEN; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_RENAME; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.DIRECTORIES_CREATED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.DIRECTORIES_DELETED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.ERROR_IGNORED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.FILES_CREATED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.FILES_DELETED; import static org.apache.hadoop.fs.impl.PathCapabilitiesSupport.validatePathCapabilityArgs; /** @@ -969,7 +984,8 @@ public void access(final Path path, final FsAction mode) throws IOException { * @return true if the path exists. * @throws IOException */ - @Override public boolean exists(Path f) throws IOException { + @Override + public boolean exists(Path f) throws IOException { statIncrement(CALL_EXIST); return super.exists(f); } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java index 571fee31a0a2d..64d06ac214e72 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java @@ -32,10 +32,54 @@ public class ITestAbfsStatistics extends AbstractAbfsIntegrationTest { private static final int NUMBER_OF_OPS = 10; + private String createOp = + AbfsStatistic.CALL_CREATE.getStatName(); + private String createNonRecursiveOp = + AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName(); + private String fileCreated = + AbfsStatistic.FILES_CREATED.getStatName(); + private String directoriesCreated = + AbfsStatistic.DIRECTORIES_CREATED.getStatName(); + private String mkdirOp = AbfsStatistic.CALL_MKDIRS.getStatName(); + private String getFileStatusOp = + AbfsStatistic.CALL_GET_FILE_STATUS.getStatName(); + private String deleteOp = + AbfsStatistic.CALL_DELETE.getStatName(); + private String fileDeleted = + AbfsStatistic.FILES_DELETED.getStatName(); + private String directoriesDeleted = + AbfsStatistic.DIRECTORIES_DELETED.getStatName(); + private String listStatusOp = + AbfsStatistic.CALL_LIST_STATUS.getStatName(); + private String openOp = AbfsStatistic.CALL_OPEN.getStatName(); + private String appendOp = + AbfsStatistic.CALL_APPEND.getStatName(); + private String renameOp = + AbfsStatistic.CALL_RENAME.getStatName(); + private String existsOp = AbfsStatistic.CALL_EXIST.getStatName(); public ITestAbfsStatistics() throws Exception { } + /** + * Testing the initial value of statistics. + */ + @Test + public void testInitialStatsValues() throws IOException { + describe("Testing the initial values of Abfs counters"); + AbfsInstrumentation abfsInstrumentation = + new AbfsInstrumentation(getFileSystem().getUri()); + Map metricMap = abfsInstrumentation.toMap(); + + for (Map.Entry entry : metricMap.entrySet()) { + String key = entry.getKey(); + Long value = entry.getValue(); + + //Verify if initial value of statistic is 0. + checkInitialValue(key, value); + } + } + /** * Testing statistics by creating files and directories. */ @@ -48,54 +92,36 @@ public void testCreateStatistics() throws IOException { Path createFilePath = path(getMethodName()); Path createDirectoryPath = path(getMethodName() + "Dir"); - Map metricMap = fs.getInstrumentation().toMap(); - - /* - Test for initial values of create statistics ; getFileStatus is called - 1 time after Abfs initialisation. - */ - assertEquals("Mismatch in op_create", 0, - (long) metricMap.get("op_create")); - assertEquals("Mismatch in op_create_non_recursive", 0, - (long) metricMap.get("op_create_non_recursive")); - assertEquals("Mismatch in files_created", 0, - (long) metricMap.get("files_created")); - assertEquals("Mismatch in directories_created", 0, - (long) metricMap.get("directories_created")); - assertEquals("Mismatch in op_mkdirs", 0, - (long) metricMap.get("op_mkdirs")); - assertEquals("Mismatch in op_get_file_status", 1, - (long) metricMap.get("op_get_file_status")); - fs.mkdirs(createDirectoryPath); fs.createNonRecursive(createFilePath, FsPermission .getDefault(), false, 1024, (short) 1, 1024, null); - metricMap = fs.getInstrumentation().toMap(); - /* - Test of statistic values after creating a directory and a file ; - getFileStatus is called 1 time after creating file and 1 time at - time of initialising. - */ - assertEquals("Mismatch in op_create", 1, - (long) metricMap.get("op_create")); - assertEquals("Mismatch in op_create_non_recursive", 1, - (long) metricMap.get("op_create_non_recursive")); - assertEquals("Mismatch in files_created", 1, - (long) metricMap.get("files_created")); - assertEquals("Mismatch in directories_created", 1, - (long) metricMap.get("directories_created")); - assertEquals("Mismatch in op_mkdirs", 1, - (long) metricMap.get("op_mkdirs")); - assertEquals("Mismatch in op_get_file_status", 2, - (long) metricMap.get("op_get_file_status")); + Map metricMap = fs.getInstrumentation().toMap(); + /* + Test of statistic values after creating a directory and a file ; + getFileStatus is called 1 time after creating file and 1 time at time of + initialising. + */ + assertEquals("Mismatch in " + createOp, 1, + (long) metricMap.get(createOp)); + assertEquals("Mismatch in " + createNonRecursiveOp, 1, + (long) metricMap.get(createNonRecursiveOp)); + assertEquals("Mismatch in " + fileCreated, 1, + (long) metricMap.get(fileCreated)); + assertEquals("Mismatch in " + directoriesCreated, 1, + (long) metricMap.get(directoriesCreated)); + assertEquals("Mismatch in " + mkdirOp, 1, + (long) metricMap.get(mkdirOp)); + assertEquals("Mismatch in " + getFileStatusOp, 2, + (long) metricMap.get(getFileStatusOp)); //re-initialising Abfs to reset statistic values. fs.initialize(fs.getUri(), fs.getConf()); - /*Creating 10 directories and files; Directories and files can't - be created with same name, hence + i to give unique names. - */ + /* + Creating 10 directories and files; Directories and files can't be created + with same name, hence + i to give unique names. + */ for (int i = 0; i < NUMBER_OF_OPS; i++) { fs.mkdirs(path(getMethodName() + "Dir" + i)); fs.createNonRecursive(path(getMethodName() + i), @@ -104,23 +130,23 @@ public void testCreateStatistics() throws IOException { } metricMap = fs.getInstrumentation().toMap(); - /* - Test of statistics values after creating 10 directories and files; - getFileStatus is called 1 time at initialise() plus number of - times file is created. - */ - assertEquals("Mismatch in op_create", NUMBER_OF_OPS, - (long) metricMap.get("op_create")); - assertEquals("Mismatch in op_create_non_recursive", NUMBER_OF_OPS, - (long) metricMap.get("op_create_non_recursive")); - assertEquals("Mismatch in files_created", NUMBER_OF_OPS, - (long) metricMap.get("files_created")); - assertEquals("Mismatch in directories_created", NUMBER_OF_OPS, - (long) metricMap.get("directories_created")); - assertEquals("Mismatch in op_mkdirs", NUMBER_OF_OPS, - (long) metricMap.get("op_mkdirs")); - assertEquals("Mismatch in op_get_file_status", 1 + NUMBER_OF_OPS, - (long) metricMap.get("op_get_file_status")); + /* + Test of statistics values after creating 10 directories and files; + getFileStatus is called 1 time at initialise() plus number of times file + is created. + */ + assertEquals("Mismatch in " + createOp, NUMBER_OF_OPS, + (long) metricMap.get(createOp)); + assertEquals("Mismatch in " + createNonRecursiveOp, NUMBER_OF_OPS, + (long) metricMap.get(createNonRecursiveOp)); + assertEquals("Mismatch in " + fileCreated, NUMBER_OF_OPS, + (long) metricMap.get(fileCreated)); + assertEquals("Mismatch in " + directoriesCreated, NUMBER_OF_OPS, + (long) metricMap.get(directoriesCreated)); + assertEquals("Mismatch in " + mkdirOp, NUMBER_OF_OPS, + (long) metricMap.get(mkdirOp)); + assertEquals("Mismatch in " + getFileStatusOp, 1 + NUMBER_OF_OPS, + (long) metricMap.get(getFileStatusOp)); } @@ -133,59 +159,48 @@ public void testDeleteStatistics() throws IOException { + "in Abfs"); AzureBlobFileSystem fs = getFileSystem(); /* - This directory path needs to be root for triggering the - directories_deleted counter. + This directory path needs to be root for triggering the + directories_deleted counter. */ Path createDirectoryPath = path("/"); Path createFilePath = path(getMethodName()); - Map metricMap = fs.getInstrumentation().toMap(); - //Test for initial statistic values before deleting any directory or files - assertEquals("Mismatch in op_delete", 0, - (long) metricMap.get("op_delete")); - assertEquals("Mismatch in files_deleted", 0, - (long) metricMap.get("files_deleted")); - assertEquals("Mismatch in directories_deleted", 0, - (long) metricMap.get("directories_deleted")); - assertEquals("Mismatch in op_list_status", 0, - (long) metricMap.get("op_list_status")); - - /* - creating a directory and a file inside that directory. - The directory is root. Hence, no parent. This allows us to invoke - deleteRoot() method to see the population of directories_deleted and - files_deleted counters. - */ + /* + creating a directory and a file inside that directory. + The directory is root. Hence, no parent. This allows us to invoke + deleteRoot() method to see the population of directories_deleted and + files_deleted counters. + */ fs.mkdirs(createDirectoryPath); fs.create(path(createDirectoryPath + getMethodName())); fs.delete(createDirectoryPath, true); - metricMap = fs.getInstrumentation().toMap(); + Map metricMap = fs.getInstrumentation().toMap(); + + /* + Test for op_delete, files_deleted, op_list_status. + since directory is delete recursively op_delete is called 2 times. + 1 file is deleted, 1 listStatus() call is made. + */ + assertEquals("Mismatch in " + deleteOp, 2, + (long) metricMap.get(deleteOp)); + assertEquals("Mismatch in " + fileDeleted, 1, + (long) metricMap.get(fileDeleted)); + assertEquals("Mismatch in " + listStatusOp, 1, + (long) metricMap.get(listStatusOp)); - /* - Test for op_delete, files_deleted, op_list_status. - since directory is delete recursively op_delete is called 2 times. - 1 file is deleted, 1 listStatus() call is made. - */ - assertEquals("Mismatch in op_delete", 2, - (long) metricMap.get("op_delete")); - assertEquals("Mismatch in files_deleted", 1, - (long) metricMap.get("files_deleted")); - assertEquals("Mismatch in op_list_status", 1, - (long) metricMap.get("op_list_status")); - - /* - creating a root directory and deleting it recursively to see if - directories_deleted is called or not. - */ + /* + creating a root directory and deleting it recursively to see if + directories_deleted is called or not. + */ fs.mkdirs(createDirectoryPath); fs.create(createFilePath); fs.delete(createDirectoryPath, true); metricMap = fs.getInstrumentation().toMap(); - // Test for directories_deleted. - assertEquals("Mismatch in directories_deleted", 1, - (long) metricMap.get("directories_deleted")); + //Test for directories_deleted. + assertEquals("Mismatch in " + directoriesDeleted, 1, + (long) metricMap.get(directoriesDeleted)); } /** @@ -200,41 +215,29 @@ public void testOpenAppendRenameExists() throws IOException { Path createFilePath = path(getMethodName()); Path destCreateFilePath = path(getMethodName() + "New"); - Map metricMap = fs.getInstrumentation().toMap(); - - //Tests for initial values of counters before any method is called. - assertEquals("Mismatch in op_open", 0, - (long) metricMap.get("op_open")); - assertEquals("Mismatch in op_append", 0, - (long) metricMap.get("op_append")); - assertEquals("Mismatch in op_rename", 0, - (long) metricMap.get("op_rename")); - assertEquals("Mismatch in op_exists", 0, - (long) metricMap.get("op_exists")); - fs.create(createFilePath); fs.open(createFilePath); fs.append(createFilePath); fs.rename(createFilePath, destCreateFilePath); - metricMap = fs.getInstrumentation().toMap(); + Map metricMap = fs.getInstrumentation().toMap(); //Testing single method calls to open, append and rename. - assertEquals("Mismatch in op_open", 1, - (long) metricMap.get("op_open")); - assertEquals("Mismatch in op_append", 1, - (long) metricMap.get("op_append")); - assertEquals("Mismatch in op_rename", 1, - (long) metricMap.get("op_rename")); + assertEquals("Mismatch in " + openOp, 1, + (long) metricMap.get(openOp)); + assertEquals("Mismatch in " + appendOp, 1, + (long) metricMap.get(appendOp)); + assertEquals("Mismatch in " + renameOp, 1, + (long) metricMap.get(renameOp)); //Testing if file exists at path. - assertTrue("Mismatch in file Path existing", + assertTrue("File should exist", fs.exists(destCreateFilePath)); - assertFalse("Mismatch in file Path existing", fs.exists(createFilePath)); + assertFalse("File should not exist", fs.exists(createFilePath)); metricMap = fs.getInstrumentation().toMap(); //Testing exists() calls. - assertEquals("Mismatch in op_exists", 2, - (long) metricMap.get("op_exists")); + assertEquals("Mismatch in " + existsOp, 2, + (long) metricMap.get(existsOp)); //re-initialising Abfs to reset statistic values. fs.initialize(fs.getUri(), fs.getConf()); @@ -249,10 +252,10 @@ public void testOpenAppendRenameExists() throws IOException { metricMap = fs.getInstrumentation().toMap(); //Testing large number of method calls to open, append. - assertEquals("Mismatch in op_open", NUMBER_OF_OPS, - (long) metricMap.get("op_open")); - assertEquals("Mismatch in op_append", NUMBER_OF_OPS, - (long) metricMap.get("op_append")); + assertEquals("Mismatch in " + openOp, NUMBER_OF_OPS, + (long) metricMap.get(openOp)); + assertEquals("Mismatch in " + appendOp, NUMBER_OF_OPS, + (long) metricMap.get(appendOp)); for (int i = 0; i < NUMBER_OF_OPS; i++) { // rename and then back to earlier name for no error while looping. @@ -260,21 +263,33 @@ public void testOpenAppendRenameExists() throws IOException { fs.rename(createFilePath, destCreateFilePath); //check if first name is existing and 2nd is not existing. - assertTrue("Mismatch in file Path existing", + assertTrue("File should exist", fs.exists(destCreateFilePath)); - assertFalse("Mismatch in file Path existing", + assertFalse("File should not exist", fs.exists(createFilePath)); } metricMap = fs.getInstrumentation().toMap(); - /*Testing exists() calls and rename calls. Since both were called 2 - times in 1 loop. 2*numberOfOps is expectedValue. */ - assertEquals("Mismatch in op_rename", 2 * NUMBER_OF_OPS, - (long) metricMap.get("op_rename")); - assertEquals("Mismatch in op_exists", 2 * NUMBER_OF_OPS, - (long) metricMap.get("op_exists")); + /* + Testing exists() calls and rename calls. Since both were called 2 + times in 1 loop. 2*numberOfOps is expectedValue. + */ + assertEquals("Mismatch in " + renameOp, 2 * NUMBER_OF_OPS, + (long) metricMap.get(renameOp)); + assertEquals("Mismatch in " + existsOp, 2 * NUMBER_OF_OPS, + (long) metricMap.get(existsOp)); } + + /** + * Method to check initial value of the statistics which should be 0. + * + * @param statName name of the statistic to be checked. + * @param statValue value of the statistic. + */ + private void checkInitialValue(String statName, long statValue) { + assertEquals("Mismatch in " + statName, 0, statValue); + } } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java index a39e378d867eb..22feda2e6799c 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java @@ -23,9 +23,16 @@ import org.junit.Test; +/** + * Unit tests for Abfs common counters. + */ public class TestAbfsStatistics extends AbstractAbfsIntegrationTest { private static final int LARGE_OPS = 100; + private String getDelegationTokenOp = + AbfsStatistic.CALL_GET_DELEGATION_TOKEN.getStatName(); + private String errorIgnored = + AbfsStatistic.ERROR_IGNORED.getStatName(); public TestAbfsStatistics() throws Exception { } @@ -34,20 +41,11 @@ public TestAbfsStatistics() throws Exception { * Tests for op_get_delegation_token and error_ignore counter values. */ @Test - public void testInitialiseStats() throws IOException { + public void testInitializeStats() throws IOException { describe("Testing the counter values after Abfs is initialised"); AbfsInstrumentation instrumentation = new AbfsInstrumentation(getFileSystem().getUri()); - Map metricMap = instrumentation.toMap(); - - System.out.println(metricMap); - - //Testing the statistics values at initial stage. - assertEquals("Mismatch in op_get_delegation_token", 0, - (long) metricMap.get("op_get_delegation_token")); - assertEquals("Mismatch in error_ignored", 0, - (long) metricMap.get("error_ignored")); //Testing summation of the counter values. for (int i = 0; i < LARGE_OPS; i++) { @@ -55,12 +53,12 @@ public void testInitialiseStats() throws IOException { instrumentation.incrementStat(AbfsStatistic.ERROR_IGNORED, 1); } - metricMap = instrumentation.toMap(); + Map metricMap = instrumentation.toMap(); - assertEquals("Mismatch in op_get_delegation_token", LARGE_OPS, - (long) metricMap.get("op_get_delegation_token")); - assertEquals("Mismatch in error_ignored", LARGE_OPS, - (long) metricMap.get("error_ignored")); + assertEquals("Mismatch in " + getDelegationTokenOp, LARGE_OPS, + (long) metricMap.get(getDelegationTokenOp)); + assertEquals("Mismatch in " + errorIgnored, LARGE_OPS, + (long) metricMap.get(errorIgnored)); } } From e1d76f30c9e31175023501972cee6e705a9e9d76 Mon Sep 17 00:00:00 2001 From: Mehakmeet Singh Date: Tue, 19 May 2020 15:31:16 +0530 Subject: [PATCH 4/6] HADOOP-17016. File error message fix. --- .../hadoop/fs/azurebfs/ITestAbfsStatistics.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java index 64d06ac214e72..0613600d50a74 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java @@ -230,9 +230,12 @@ public void testOpenAppendRenameExists() throws IOException { (long) metricMap.get(renameOp)); //Testing if file exists at path. - assertTrue("File should exist", + assertTrue(String.format("File with name %s should exist", + destCreateFilePath), fs.exists(destCreateFilePath)); - assertFalse("File should not exist", fs.exists(createFilePath)); + assertFalse(String.format("File with name %s should not exist", + createFilePath), + fs.exists(createFilePath)); metricMap = fs.getInstrumentation().toMap(); //Testing exists() calls. @@ -263,9 +266,11 @@ public void testOpenAppendRenameExists() throws IOException { fs.rename(createFilePath, destCreateFilePath); //check if first name is existing and 2nd is not existing. - assertTrue("File should exist", + assertTrue(String.format("File with name %s should exist", + destCreateFilePath), fs.exists(destCreateFilePath)); - assertFalse("File should not exist", + assertFalse(String.format("File with name %s should not exist", + createFilePath), fs.exists(createFilePath)); } From 14d1496928b2a9aafb4d05a7e31059333a801637 Mon Sep 17 00:00:00 2001 From: Mehakmeet Singh Date: Tue, 19 May 2020 16:51:19 +0530 Subject: [PATCH 5/6] HADOOP-17016. unnecessary variables removed. --- .../fs/azurebfs/ITestAbfsStatistics.java | 134 ++++++++---------- .../fs/azurebfs/TestAbfsStatistics.java | 14 +- 2 files changed, 67 insertions(+), 81 deletions(-) diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java index 0613600d50a74..c951aecf9e153 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java @@ -32,31 +32,6 @@ public class ITestAbfsStatistics extends AbstractAbfsIntegrationTest { private static final int NUMBER_OF_OPS = 10; - private String createOp = - AbfsStatistic.CALL_CREATE.getStatName(); - private String createNonRecursiveOp = - AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName(); - private String fileCreated = - AbfsStatistic.FILES_CREATED.getStatName(); - private String directoriesCreated = - AbfsStatistic.DIRECTORIES_CREATED.getStatName(); - private String mkdirOp = AbfsStatistic.CALL_MKDIRS.getStatName(); - private String getFileStatusOp = - AbfsStatistic.CALL_GET_FILE_STATUS.getStatName(); - private String deleteOp = - AbfsStatistic.CALL_DELETE.getStatName(); - private String fileDeleted = - AbfsStatistic.FILES_DELETED.getStatName(); - private String directoriesDeleted = - AbfsStatistic.DIRECTORIES_DELETED.getStatName(); - private String listStatusOp = - AbfsStatistic.CALL_LIST_STATUS.getStatName(); - private String openOp = AbfsStatistic.CALL_OPEN.getStatName(); - private String appendOp = - AbfsStatistic.CALL_APPEND.getStatName(); - private String renameOp = - AbfsStatistic.CALL_RENAME.getStatName(); - private String existsOp = AbfsStatistic.CALL_EXIST.getStatName(); public ITestAbfsStatistics() throws Exception { } @@ -102,18 +77,18 @@ public void testCreateStatistics() throws IOException { getFileStatus is called 1 time after creating file and 1 time at time of initialising. */ - assertEquals("Mismatch in " + createOp, 1, - (long) metricMap.get(createOp)); - assertEquals("Mismatch in " + createNonRecursiveOp, 1, - (long) metricMap.get(createNonRecursiveOp)); - assertEquals("Mismatch in " + fileCreated, 1, - (long) metricMap.get(fileCreated)); - assertEquals("Mismatch in " + directoriesCreated, 1, - (long) metricMap.get(directoriesCreated)); - assertEquals("Mismatch in " + mkdirOp, 1, - (long) metricMap.get(mkdirOp)); - assertEquals("Mismatch in " + getFileStatusOp, 2, - (long) metricMap.get(getFileStatusOp)); + assertEquals("Mismatch in " + AbfsStatistic.CALL_CREATE.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.CALL_CREATE.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.FILES_CREATED.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.FILES_CREATED.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.DIRECTORIES_CREATED.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.DIRECTORIES_CREATED.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_MKDIRS.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.CALL_MKDIRS.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_GET_FILE_STATUS.getStatName(), 2, + (long) metricMap.get(AbfsStatistic.CALL_GET_FILE_STATUS.getStatName())); //re-initialising Abfs to reset statistic values. fs.initialize(fs.getUri(), fs.getConf()); @@ -135,18 +110,26 @@ public void testCreateStatistics() throws IOException { getFileStatus is called 1 time at initialise() plus number of times file is created. */ - assertEquals("Mismatch in " + createOp, NUMBER_OF_OPS, - (long) metricMap.get(createOp)); - assertEquals("Mismatch in " + createNonRecursiveOp, NUMBER_OF_OPS, - (long) metricMap.get(createNonRecursiveOp)); - assertEquals("Mismatch in " + fileCreated, NUMBER_OF_OPS, - (long) metricMap.get(fileCreated)); - assertEquals("Mismatch in " + directoriesCreated, NUMBER_OF_OPS, - (long) metricMap.get(directoriesCreated)); - assertEquals("Mismatch in " + mkdirOp, NUMBER_OF_OPS, - (long) metricMap.get(mkdirOp)); - assertEquals("Mismatch in " + getFileStatusOp, 1 + NUMBER_OF_OPS, - (long) metricMap.get(getFileStatusOp)); + assertEquals("Mismatch in " + AbfsStatistic.CALL_CREATE.getStatName(), + NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.CALL_CREATE.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName(), + NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.FILES_CREATED.getStatName(), + NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.FILES_CREATED.getStatName())); + assertEquals( + "Mismatch in " + AbfsStatistic.DIRECTORIES_CREATED.getStatName(), + NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.DIRECTORIES_CREATED.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_MKDIRS.getStatName(), + NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.CALL_MKDIRS.getStatName())); + assertEquals( + "Mismatch in " + AbfsStatistic.CALL_GET_FILE_STATUS.getStatName(), + 1 + NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.CALL_GET_FILE_STATUS.getStatName())); } @@ -182,12 +165,12 @@ public void testDeleteStatistics() throws IOException { since directory is delete recursively op_delete is called 2 times. 1 file is deleted, 1 listStatus() call is made. */ - assertEquals("Mismatch in " + deleteOp, 2, - (long) metricMap.get(deleteOp)); - assertEquals("Mismatch in " + fileDeleted, 1, - (long) metricMap.get(fileDeleted)); - assertEquals("Mismatch in " + listStatusOp, 1, - (long) metricMap.get(listStatusOp)); + assertEquals("Mismatch in " + AbfsStatistic.CALL_DELETE.getStatName(), 2, + (long) metricMap.get(AbfsStatistic.CALL_DELETE.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.FILES_DELETED.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.FILES_DELETED.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_LIST_STATUS.getStatName(), + 1, (long) metricMap.get(AbfsStatistic.CALL_LIST_STATUS.getStatName())); /* creating a root directory and deleting it recursively to see if @@ -199,8 +182,9 @@ public void testDeleteStatistics() throws IOException { metricMap = fs.getInstrumentation().toMap(); //Test for directories_deleted. - assertEquals("Mismatch in " + directoriesDeleted, 1, - (long) metricMap.get(directoriesDeleted)); + assertEquals( + "Mismatch in " + AbfsStatistic.DIRECTORIES_DELETED.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.DIRECTORIES_DELETED.getStatName())); } /** @@ -222,12 +206,12 @@ public void testOpenAppendRenameExists() throws IOException { Map metricMap = fs.getInstrumentation().toMap(); //Testing single method calls to open, append and rename. - assertEquals("Mismatch in " + openOp, 1, - (long) metricMap.get(openOp)); - assertEquals("Mismatch in " + appendOp, 1, - (long) metricMap.get(appendOp)); - assertEquals("Mismatch in " + renameOp, 1, - (long) metricMap.get(renameOp)); + assertEquals("Mismatch in " + AbfsStatistic.CALL_OPEN.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.CALL_OPEN.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_APPEND.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.CALL_APPEND.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_RENAME.getStatName(), 1, + (long) metricMap.get(AbfsStatistic.CALL_RENAME.getStatName())); //Testing if file exists at path. assertTrue(String.format("File with name %s should exist", @@ -239,8 +223,8 @@ public void testOpenAppendRenameExists() throws IOException { metricMap = fs.getInstrumentation().toMap(); //Testing exists() calls. - assertEquals("Mismatch in " + existsOp, 2, - (long) metricMap.get(existsOp)); + assertEquals("Mismatch in " + AbfsStatistic.CALL_EXIST.getStatName(), 2, + (long) metricMap.get(AbfsStatistic.CALL_EXIST.getStatName())); //re-initialising Abfs to reset statistic values. fs.initialize(fs.getUri(), fs.getConf()); @@ -255,10 +239,12 @@ public void testOpenAppendRenameExists() throws IOException { metricMap = fs.getInstrumentation().toMap(); //Testing large number of method calls to open, append. - assertEquals("Mismatch in " + openOp, NUMBER_OF_OPS, - (long) metricMap.get(openOp)); - assertEquals("Mismatch in " + appendOp, NUMBER_OF_OPS, - (long) metricMap.get(appendOp)); + assertEquals("Mismatch in " + AbfsStatistic.CALL_OPEN.getStatName(), + NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.CALL_OPEN.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_APPEND.getStatName(), + NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.CALL_APPEND.getStatName())); for (int i = 0; i < NUMBER_OF_OPS; i++) { // rename and then back to earlier name for no error while looping. @@ -281,10 +267,12 @@ public void testOpenAppendRenameExists() throws IOException { Testing exists() calls and rename calls. Since both were called 2 times in 1 loop. 2*numberOfOps is expectedValue. */ - assertEquals("Mismatch in " + renameOp, 2 * NUMBER_OF_OPS, - (long) metricMap.get(renameOp)); - assertEquals("Mismatch in " + existsOp, 2 * NUMBER_OF_OPS, - (long) metricMap.get(existsOp)); + assertEquals("Mismatch in " + AbfsStatistic.CALL_RENAME.getStatName(), + 2 * NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.CALL_RENAME.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.CALL_EXIST.getStatName(), + 2 * NUMBER_OF_OPS, + (long) metricMap.get(AbfsStatistic.CALL_EXIST.getStatName())); } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java index 22feda2e6799c..9c3f97f5ea7b1 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java @@ -29,10 +29,6 @@ public class TestAbfsStatistics extends AbstractAbfsIntegrationTest { private static final int LARGE_OPS = 100; - private String getDelegationTokenOp = - AbfsStatistic.CALL_GET_DELEGATION_TOKEN.getStatName(); - private String errorIgnored = - AbfsStatistic.ERROR_IGNORED.getStatName(); public TestAbfsStatistics() throws Exception { } @@ -55,10 +51,12 @@ public void testInitializeStats() throws IOException { Map metricMap = instrumentation.toMap(); - assertEquals("Mismatch in " + getDelegationTokenOp, LARGE_OPS, - (long) metricMap.get(getDelegationTokenOp)); - assertEquals("Mismatch in " + errorIgnored, LARGE_OPS, - (long) metricMap.get(errorIgnored)); + assertEquals("Mismatch in " + AbfsStatistic.CALL_GET_DELEGATION_TOKEN.getStatName(), + LARGE_OPS, + (long) metricMap.get(AbfsStatistic.CALL_GET_DELEGATION_TOKEN.getStatName())); + assertEquals("Mismatch in " + AbfsStatistic.ERROR_IGNORED.getStatName(), + LARGE_OPS, + (long) metricMap.get(AbfsStatistic.ERROR_IGNORED.getStatName())); } } From 94401c1df44f2a9456e8afa8887b8a62af0891ef Mon Sep 17 00:00:00 2001 From: Mehakmeet Singh Date: Mon, 25 May 2020 12:03:41 +0530 Subject: [PATCH 6/6] HADOOP-17016. review comments fix. --- .../fs/azurebfs/AbfsInstrumentation.java | 37 +++--- .../fs/azurebfs/AzureBlobFileSystem.java | 32 ++--- .../fs/azurebfs/services/AbfsCounters.java | 66 ++++++++++ .../azurebfs/AbstractAbfsIntegrationTest.java | 15 +++ .../fs/azurebfs/ITestAbfsStatistics.java | 120 +++++++----------- .../fs/azurebfs/TestAbfsStatistics.java | 17 ++- 6 files changed, 158 insertions(+), 129 deletions(-) create mode 100644 hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsCounters.java diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java index 21bab6ab7b5d0..9094c4065de0c 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsInstrumentation.java @@ -25,7 +25,7 @@ import com.google.common.annotations.VisibleForTesting; -import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.fs.azurebfs.services.AbfsCounters; import org.apache.hadoop.metrics2.AbstractMetric; import org.apache.hadoop.metrics2.MetricStringBuilder; import org.apache.hadoop.metrics2.MetricsCollector; @@ -36,28 +36,12 @@ import org.apache.hadoop.metrics2.lib.MutableCounterLong; import org.apache.hadoop.metrics2.lib.MutableMetric; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_APPEND; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_CREATE; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_CREATE_NON_RECURSIVE; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_DELETE; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_EXIST; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_DELEGATION_TOKEN; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_FILE_STATUS; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_LIST_STATUS; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_MKDIRS; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_OPEN; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_RENAME; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.DIRECTORIES_CREATED; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.DIRECTORIES_DELETED; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.ERROR_IGNORED; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.FILES_CREATED; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.FILES_DELETED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.*; /** * Instrumentation of Abfs counters. */ -@InterfaceStability.Unstable -public class AbfsInstrumentation { +public class AbfsInstrumentation implements AbfsCounters { /** * Single context for all the Abfs counters to separate them from other @@ -149,12 +133,15 @@ private MutableCounterLong createCounter(AbfsStatistic stats) { } /** + * {@inheritDoc} + * * Increment a statistic with some value. * * @param statistic AbfsStatistic need to be incremented. * @param value long value to be incremented by. */ - protected void incrementStat(AbfsStatistic statistic, long value) { + @Override + public void incrementCounter(AbfsStatistic statistic, long value) { MutableCounterLong counter = lookupCounter(statistic.getStatName()); if (counter != null) { counter.incr(value); @@ -171,6 +158,8 @@ private MetricsRegistry getRegistry() { } /** + * {@inheritDoc} + * * Method to aggregate all the counters in the MetricRegistry and form a * string with prefix, separator and suffix. * @@ -180,7 +169,8 @@ private MetricsRegistry getRegistry() { * @param all gets all the values even if unchanged. * @return a String with all the metrics and their values. */ - protected String formString(String prefix, String separator, String suffix, + @Override + public String formString(String prefix, String separator, String suffix, boolean all) { MetricStringBuilder metricStringBuilder = new MetricStringBuilder(null, @@ -190,12 +180,15 @@ protected String formString(String prefix, String separator, String suffix, } /** + * {@inheritDoc} + * * Creating a map of all the counters for testing. * * @return a map of the metrics. */ @VisibleForTesting - protected Map toMap() { + @Override + public Map toMap() { MetricsToMap metricBuilder = new MetricsToMap(null); registry.snapshot(metricBuilder, true); return metricBuilder.getMap(); diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java index b1883d40fa44a..700d23a3275d9 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java @@ -30,6 +30,7 @@ import java.util.List; import java.util.ArrayList; import java.util.EnumSet; +import java.util.Map; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -68,6 +69,7 @@ import org.apache.hadoop.fs.azurebfs.contracts.exceptions.SASTokenProviderException; import org.apache.hadoop.fs.azurebfs.contracts.services.AzureServiceErrorCode; import org.apache.hadoop.fs.azurebfs.security.AbfsDelegationTokenManager; +import org.apache.hadoop.fs.azurebfs.services.AbfsCounters; import org.apache.hadoop.fs.permission.AclEntry; import org.apache.hadoop.fs.permission.AclStatus; import org.apache.hadoop.fs.permission.FsAction; @@ -78,22 +80,7 @@ import org.apache.hadoop.security.UserGroupInformation; import org.apache.hadoop.util.Progressable; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_APPEND; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_CREATE; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_CREATE_NON_RECURSIVE; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_DELETE; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_EXIST; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_DELEGATION_TOKEN; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_GET_FILE_STATUS; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_LIST_STATUS; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_MKDIRS; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_OPEN; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.CALL_RENAME; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.DIRECTORIES_CREATED; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.DIRECTORIES_DELETED; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.ERROR_IGNORED; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.FILES_CREATED; -import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.FILES_DELETED; +import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.*; import static org.apache.hadoop.fs.impl.PathCapabilitiesSupport.validatePathCapabilityArgs; /** @@ -110,7 +97,7 @@ public class AzureBlobFileSystem extends FileSystem { private boolean delegationTokenEnabled = false; private AbfsDelegationTokenManager delegationTokenManager; - private AbfsInstrumentation instrumentation; + private AbfsCounters instrumentation; @Override public void initialize(URI uri, Configuration configuration) @@ -396,17 +383,16 @@ public FileStatus[] listStatus(final Path f) throws IOException { * @param statistic AbfsStatistic that needs increment. */ private void statIncrement(AbfsStatistic statistic) { - incrementStatistic(statistic, 1); + incrementStatistic(statistic); } /** * Method for incrementing AbfsStatistic by a long value. * * @param statistic the Statistic to be incremented. - * @param value value to be incremented. */ - private void incrementStatistic(AbfsStatistic statistic, long value) { - instrumentation.incrementStat(statistic, value); + private void incrementStatistic(AbfsStatistic statistic) { + instrumentation.incrementCounter(statistic, 1); } /** @@ -1250,8 +1236,8 @@ boolean getIsNamespaceEnabled() throws AzureBlobFileSystemException { } @VisibleForTesting - AbfsInstrumentation getInstrumentation() { - return instrumentation; + Map getInstrumentationMap() { + return instrumentation.toMap(); } @Override diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsCounters.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsCounters.java new file mode 100644 index 0000000000000..87b1af4f0620c --- /dev/null +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsCounters.java @@ -0,0 +1,66 @@ +/** + * 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.fs.azurebfs.services; + +import java.util.Map; + +import com.google.common.annotations.VisibleForTesting; + +import org.apache.hadoop.classification.InterfaceAudience; +import org.apache.hadoop.classification.InterfaceStability; +import org.apache.hadoop.fs.azurebfs.AbfsStatistic; + +/** + * An interface for Abfs counters. + */ +@InterfaceAudience.Private +@InterfaceStability.Unstable +public interface AbfsCounters { + + /** + * Increment a AbfsStatistic by a long value. + * + * @param statistic AbfsStatistic to be incremented. + * @param value the value to increment the statistic by. + */ + void incrementCounter(AbfsStatistic statistic, long value); + + /** + * Form a String of the all the statistics and present in an organized manner. + * + * @param prefix the prefix to be set. + * @param separator the separator between the statistic name and value. + * @param suffix the suffix to be used. + * @param all enable all the statistics to be displayed or not. + * @return String of all the statistics and their values. + */ + String formString(String prefix, String separator, String suffix, + boolean all); + + /** + * Convert all the statistics into a key-value pair map to be used for + * testing. + * + * @return map with statistic name as key and statistic value as the map + * value. + */ + @VisibleForTesting + Map toMap(); + +} diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsIntegrationTest.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsIntegrationTest.java index 4d9fc5cae7331..87a3dcd881502 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsIntegrationTest.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/AbstractAbfsIntegrationTest.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.net.URI; import java.util.Hashtable; +import java.util.Map; import java.util.UUID; import java.util.concurrent.Callable; @@ -409,4 +410,18 @@ protected AbfsOutputStream createAbfsOutputStreamWithFlushEnabled( return (AbfsOutputStream) abfss.createFile(path, fs.getFsStatistics(), true, FsPermission.getDefault(), FsPermission.getUMask(fs.getConf())); } + + /** + * Custom assertion for AbfsStatistics which have statistics, expected + * value and map of statistics and value as its parameters. + * @param statistic the AbfsStatistics which needs to be asserted. + * @param expectedValue the expected value of the statistics. + * @param metricMap map of (String, Long) with statistics name as key and + * statistics value as map value. + */ + protected void assertAbfsStatistics(AbfsStatistic statistic, + long expectedValue, Map metricMap) { + assertEquals("Mismatch in " + statistic.getStatName(), expectedValue, + (long) metricMap.get(statistic.getStatName())); + } } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java index c951aecf9e153..c88dc847a3f9a 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAbfsStatistics.java @@ -24,6 +24,7 @@ import org.junit.Test; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.azurebfs.services.AbfsCounters; import org.apache.hadoop.fs.permission.FsPermission; /** @@ -42,9 +43,10 @@ public ITestAbfsStatistics() throws Exception { @Test public void testInitialStatsValues() throws IOException { describe("Testing the initial values of Abfs counters"); - AbfsInstrumentation abfsInstrumentation = + + AbfsCounters abfsCounters = new AbfsInstrumentation(getFileSystem().getUri()); - Map metricMap = abfsInstrumentation.toMap(); + Map metricMap = abfsCounters.toMap(); for (Map.Entry entry : metricMap.entrySet()) { String key = entry.getKey(); @@ -71,24 +73,18 @@ public void testCreateStatistics() throws IOException { fs.createNonRecursive(createFilePath, FsPermission .getDefault(), false, 1024, (short) 1, 1024, null); - Map metricMap = fs.getInstrumentation().toMap(); + Map metricMap = fs.getInstrumentationMap(); /* Test of statistic values after creating a directory and a file ; getFileStatus is called 1 time after creating file and 1 time at time of initialising. */ - assertEquals("Mismatch in " + AbfsStatistic.CALL_CREATE.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.CALL_CREATE.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.FILES_CREATED.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.FILES_CREATED.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.DIRECTORIES_CREATED.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.DIRECTORIES_CREATED.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_MKDIRS.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.CALL_MKDIRS.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_GET_FILE_STATUS.getStatName(), 2, - (long) metricMap.get(AbfsStatistic.CALL_GET_FILE_STATUS.getStatName())); + assertAbfsStatistics(AbfsStatistic.CALL_CREATE, 1, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_CREATE_NON_RECURSIVE, 1, metricMap); + assertAbfsStatistics(AbfsStatistic.FILES_CREATED, 1, metricMap); + assertAbfsStatistics(AbfsStatistic.DIRECTORIES_CREATED, 1, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_MKDIRS, 1, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_GET_FILE_STATUS, 2, metricMap); //re-initialising Abfs to reset statistic values. fs.initialize(fs.getUri(), fs.getConf()); @@ -104,33 +100,21 @@ public void testCreateStatistics() throws IOException { 1024, null); } - metricMap = fs.getInstrumentation().toMap(); + metricMap = fs.getInstrumentationMap(); /* Test of statistics values after creating 10 directories and files; getFileStatus is called 1 time at initialise() plus number of times file is created. */ - assertEquals("Mismatch in " + AbfsStatistic.CALL_CREATE.getStatName(), - NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.CALL_CREATE.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName(), - NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.CALL_CREATE_NON_RECURSIVE.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.FILES_CREATED.getStatName(), - NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.FILES_CREATED.getStatName())); - assertEquals( - "Mismatch in " + AbfsStatistic.DIRECTORIES_CREATED.getStatName(), - NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.DIRECTORIES_CREATED.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_MKDIRS.getStatName(), - NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.CALL_MKDIRS.getStatName())); - assertEquals( - "Mismatch in " + AbfsStatistic.CALL_GET_FILE_STATUS.getStatName(), - 1 + NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.CALL_GET_FILE_STATUS.getStatName())); - + assertAbfsStatistics(AbfsStatistic.CALL_CREATE, NUMBER_OF_OPS, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_CREATE_NON_RECURSIVE, NUMBER_OF_OPS, + metricMap); + assertAbfsStatistics(AbfsStatistic.FILES_CREATED, NUMBER_OF_OPS, metricMap); + assertAbfsStatistics(AbfsStatistic.DIRECTORIES_CREATED, NUMBER_OF_OPS, + metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_MKDIRS, NUMBER_OF_OPS, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_GET_FILE_STATUS, + 1 + NUMBER_OF_OPS, metricMap); } /** @@ -140,6 +124,7 @@ public void testCreateStatistics() throws IOException { public void testDeleteStatistics() throws IOException { describe("Testing counter values got by deleting directory and files " + "in Abfs"); + AzureBlobFileSystem fs = getFileSystem(); /* This directory path needs to be root for triggering the @@ -158,19 +143,16 @@ public void testDeleteStatistics() throws IOException { fs.create(path(createDirectoryPath + getMethodName())); fs.delete(createDirectoryPath, true); - Map metricMap = fs.getInstrumentation().toMap(); + Map metricMap = fs.getInstrumentationMap(); /* Test for op_delete, files_deleted, op_list_status. since directory is delete recursively op_delete is called 2 times. 1 file is deleted, 1 listStatus() call is made. */ - assertEquals("Mismatch in " + AbfsStatistic.CALL_DELETE.getStatName(), 2, - (long) metricMap.get(AbfsStatistic.CALL_DELETE.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.FILES_DELETED.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.FILES_DELETED.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_LIST_STATUS.getStatName(), - 1, (long) metricMap.get(AbfsStatistic.CALL_LIST_STATUS.getStatName())); + assertAbfsStatistics(AbfsStatistic.CALL_DELETE, 2, metricMap); + assertAbfsStatistics(AbfsStatistic.FILES_DELETED, 1, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_LIST_STATUS, 1, metricMap); /* creating a root directory and deleting it recursively to see if @@ -179,12 +161,10 @@ public void testDeleteStatistics() throws IOException { fs.mkdirs(createDirectoryPath); fs.create(createFilePath); fs.delete(createDirectoryPath, true); - metricMap = fs.getInstrumentation().toMap(); + metricMap = fs.getInstrumentationMap(); //Test for directories_deleted. - assertEquals( - "Mismatch in " + AbfsStatistic.DIRECTORIES_DELETED.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.DIRECTORIES_DELETED.getStatName())); + assertAbfsStatistics(AbfsStatistic.DIRECTORIES_DELETED, 1, metricMap); } /** @@ -202,16 +182,13 @@ public void testOpenAppendRenameExists() throws IOException { fs.create(createFilePath); fs.open(createFilePath); fs.append(createFilePath); - fs.rename(createFilePath, destCreateFilePath); + assertTrue(fs.rename(createFilePath, destCreateFilePath)); - Map metricMap = fs.getInstrumentation().toMap(); + Map metricMap = fs.getInstrumentationMap(); //Testing single method calls to open, append and rename. - assertEquals("Mismatch in " + AbfsStatistic.CALL_OPEN.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.CALL_OPEN.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_APPEND.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.CALL_APPEND.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_RENAME.getStatName(), 1, - (long) metricMap.get(AbfsStatistic.CALL_RENAME.getStatName())); + assertAbfsStatistics(AbfsStatistic.CALL_OPEN, 1, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_APPEND, 1, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_RENAME, 1, metricMap); //Testing if file exists at path. assertTrue(String.format("File with name %s should exist", @@ -221,10 +198,9 @@ public void testOpenAppendRenameExists() throws IOException { createFilePath), fs.exists(createFilePath)); - metricMap = fs.getInstrumentation().toMap(); + metricMap = fs.getInstrumentationMap(); //Testing exists() calls. - assertEquals("Mismatch in " + AbfsStatistic.CALL_EXIST.getStatName(), 2, - (long) metricMap.get(AbfsStatistic.CALL_EXIST.getStatName())); + assertAbfsStatistics(AbfsStatistic.CALL_EXIST, 2, metricMap); //re-initialising Abfs to reset statistic values. fs.initialize(fs.getUri(), fs.getConf()); @@ -236,20 +212,16 @@ public void testOpenAppendRenameExists() throws IOException { fs.append(destCreateFilePath); } - metricMap = fs.getInstrumentation().toMap(); + metricMap = fs.getInstrumentationMap(); //Testing large number of method calls to open, append. - assertEquals("Mismatch in " + AbfsStatistic.CALL_OPEN.getStatName(), - NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.CALL_OPEN.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_APPEND.getStatName(), - NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.CALL_APPEND.getStatName())); + assertAbfsStatistics(AbfsStatistic.CALL_OPEN, NUMBER_OF_OPS, metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_APPEND, NUMBER_OF_OPS, metricMap); for (int i = 0; i < NUMBER_OF_OPS; i++) { // rename and then back to earlier name for no error while looping. - fs.rename(destCreateFilePath, createFilePath); - fs.rename(createFilePath, destCreateFilePath); + assertTrue(fs.rename(destCreateFilePath, createFilePath)); + assertTrue(fs.rename(createFilePath, destCreateFilePath)); //check if first name is existing and 2nd is not existing. assertTrue(String.format("File with name %s should exist", @@ -261,18 +233,16 @@ public void testOpenAppendRenameExists() throws IOException { } - metricMap = fs.getInstrumentation().toMap(); + metricMap = fs.getInstrumentationMap(); /* Testing exists() calls and rename calls. Since both were called 2 times in 1 loop. 2*numberOfOps is expectedValue. */ - assertEquals("Mismatch in " + AbfsStatistic.CALL_RENAME.getStatName(), - 2 * NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.CALL_RENAME.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.CALL_EXIST.getStatName(), - 2 * NUMBER_OF_OPS, - (long) metricMap.get(AbfsStatistic.CALL_EXIST.getStatName())); + assertAbfsStatistics(AbfsStatistic.CALL_RENAME, 2 * NUMBER_OF_OPS, + metricMap); + assertAbfsStatistics(AbfsStatistic.CALL_EXIST, 2 * NUMBER_OF_OPS, + metricMap); } diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java index 9c3f97f5ea7b1..20d96fadef6e7 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/TestAbfsStatistics.java @@ -23,6 +23,8 @@ import org.junit.Test; +import org.apache.hadoop.fs.azurebfs.services.AbfsCounters; + /** * Unit tests for Abfs common counters. */ @@ -40,23 +42,20 @@ public TestAbfsStatistics() throws Exception { public void testInitializeStats() throws IOException { describe("Testing the counter values after Abfs is initialised"); - AbfsInstrumentation instrumentation = + AbfsCounters instrumentation = new AbfsInstrumentation(getFileSystem().getUri()); //Testing summation of the counter values. for (int i = 0; i < LARGE_OPS; i++) { - instrumentation.incrementStat(AbfsStatistic.CALL_GET_DELEGATION_TOKEN, 1); - instrumentation.incrementStat(AbfsStatistic.ERROR_IGNORED, 1); + instrumentation.incrementCounter(AbfsStatistic.CALL_GET_DELEGATION_TOKEN, 1); + instrumentation.incrementCounter(AbfsStatistic.ERROR_IGNORED, 1); } Map metricMap = instrumentation.toMap(); - assertEquals("Mismatch in " + AbfsStatistic.CALL_GET_DELEGATION_TOKEN.getStatName(), - LARGE_OPS, - (long) metricMap.get(AbfsStatistic.CALL_GET_DELEGATION_TOKEN.getStatName())); - assertEquals("Mismatch in " + AbfsStatistic.ERROR_IGNORED.getStatName(), - LARGE_OPS, - (long) metricMap.get(AbfsStatistic.ERROR_IGNORED.getStatName())); + assertAbfsStatistics(AbfsStatistic.CALL_GET_DELEGATION_TOKEN, LARGE_OPS, + metricMap); + assertAbfsStatistics(AbfsStatistic.ERROR_IGNORED, LARGE_OPS, metricMap); } }