SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system (Alex Kolbasov, reviewed by Na Li and Kalyan Kalvagadda)
Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/1749f7eb Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/1749f7eb Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/1749f7eb Branch: refs/heads/akolb-cli Commit: 1749f7ebe85e5d6a515ad264e69fad27b0624f06 Parents: 19efc0e Author: Alexander Kolbasov <[email protected]> Authored: Tue Oct 3 17:19:31 2017 -0700 Committer: Alexander Kolbasov <[email protected]> Committed: Tue Oct 3 17:19:31 2017 -0700 ---------------------------------------------------------------------- .../db/service/thrift/SentryMetrics.java | 144 +++++++++++++------ 1 file changed, 103 insertions(+), 41 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/1749f7eb/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java index 4063a66..86cae64 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java @@ -17,10 +17,16 @@ */ package org.apache.sentry.provider.db.service.thrift; -import com.codahale.metrics.*; - -import static com.codahale.metrics.MetricRegistry.name; - +import com.codahale.metrics.ConsoleReporter; +import com.codahale.metrics.Counter; +import com.codahale.metrics.Gauge; +import com.codahale.metrics.Histogram; +import com.codahale.metrics.JmxReporter; +import com.codahale.metrics.Metric; +import com.codahale.metrics.MetricRegistry; +import com.codahale.metrics.MetricSet; +import com.codahale.metrics.Slf4jReporter; +import com.codahale.metrics.Timer; import com.codahale.metrics.json.MetricsModule; import com.codahale.metrics.jvm.BufferPoolMetricSet; import com.codahale.metrics.jvm.GarbageCollectorMetricSet; @@ -30,25 +36,23 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.util.concurrent.ThreadFactoryBuilder; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.fs.FileSystem; -import org.apache.hadoop.fs.LocalFileSystem; -import org.apache.hadoop.fs.Path; -import org.apache.hadoop.fs.permission.FsPermission; import org.apache.sentry.provider.db.service.persistent.SentryStore; import org.apache.sentry.service.thrift.SentryService; import org.apache.sentry.service.thrift.SentryServiceUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static java.io.File.createTempFile; -import static org.apache.sentry.provider.db.service.thrift.SentryMetricsServletContextListener.METRIC_REGISTRY; -import static org.apache.sentry.service.thrift.ServiceConstants.ServerConfig; - import java.io.BufferedWriter; -import java.io.File; import java.io.FileWriter; import java.io.IOException; import java.lang.management.ManagementFactory; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.StandardCopyOption; +import java.nio.file.attribute.FileAttribute; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -57,6 +61,10 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import static com.codahale.metrics.MetricRegistry.name; +import static org.apache.sentry.provider.db.service.thrift.SentryMetricsServletContextListener.METRIC_REGISTRY; +import static org.apache.sentry.service.thrift.ServiceConstants.ServerConfig; + /** * A singleton class which holds metrics related utility functions as well as the list of metrics. */ @@ -276,8 +284,30 @@ public final class SentryMetrics { * This class originated from Apache Hive JSON reporter. */ private static class JsonFileReporter implements AutoCloseable, Runnable { - /** File permissions: -rw-r--r-- */ - private static final short PERMISSIONS = 0644; + // + // Implementation notes. + // + // 1. Since only local file systems are supported, there is no need to use Hadoop + // version of Path class. + // 2. java.nio package provides modern implementation of file and directory operations + // which is better then the traditional java.io, so we are using it here. + // In particular, it supports atomic creation of temporary files with specified + // permissions in the specified directory. This also avoids various attacks possible + // when temp file name is generated first, followed by file creation. + // See http://www.oracle.com/technetwork/articles/javase/nio-139333.html for + // the description of NIO API and + // http://docs.oracle.com/javase/tutorial/essential/io/legacy.html for the + // description of interoperability between legacy IO api vs NIO API. + // 3. To avoid race conditions with readers of the metrics file, the implementation + // dumps metrics to a temporary file in the same directory as the actual metrics + // file and then renames it to the destination. Since both are located on the same + // filesystem, this rename is likely to be atomic (as long as the underlying OS + // support atomic renames. + // + + // Permissions for the metrics file + private static final FileAttribute<Set<PosixFilePermission>> FILE_ATTRS = + PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-r--r--")); private static final String JSON_REPORTER_THREAD_NAME = "json-reporter"; private ScheduledExecutorService executor = null; @@ -287,14 +317,22 @@ public final class SentryMetrics { false)); private final Configuration conf; /** Destination file name. */ - private final String pathString; + // Location of JSON file + private final Path path; + // tmpdir is the dirname(path) + private final Path tmpDir; private final long interval; private final TimeUnit unit; JsonFileReporter(Configuration conf, long interval, TimeUnit unit) { this.conf = conf; - pathString = conf.get(ServerConfig.SENTRY_JSON_REPORTER_FILE, + String pathString = conf.get(ServerConfig.SENTRY_JSON_REPORTER_FILE, ServerConfig.SENTRY_JSON_REPORTER_FILE_DEFAULT); + path = Paths.get(pathString).toAbsolutePath(); + LOGGER.info("Reporting metrics to {}", path); + // We want to use tmpDir i the same directory as the destination file to support atomic + // move of temp file to the destination metrics file + tmpDir = path.getParent(); this.interval = interval; this.unit = unit; } @@ -307,31 +345,50 @@ public final class SentryMetrics { @Override public void run() { - String json = null; + Path tmpFile = null; try { - json = jsonMapper.writerWithDefaultPrettyPrinter().writeValueAsString(METRIC_REGISTRY); - } catch (JsonProcessingException e) { - LOGGER.error("Error converting metrics to JSON", e); - return; - } - File tmpFile = null; - try { - tmpFile = createTempFile("sentry-json", null); - } catch (IOException e) { - LOGGER.error("failed to create temp file for JSON metrics", e); - } - - assert tmpFile != null; - try (LocalFileSystem fs = FileSystem.getLocal(conf); - BufferedWriter bw = new BufferedWriter(new FileWriter(tmpFile))) { - bw.write(json); - Path tmpPath = new Path(tmpFile.getAbsolutePath()); - fs.setPermission(tmpPath, FsPermission.createImmutable(PERMISSIONS)); - Path path = new Path(pathString); - fs.rename(tmpPath, path); - fs.setPermission(path, FsPermission.createImmutable(PERMISSIONS)); - } catch (IOException e) { - LOGGER.warn("Error writing JSON metrics", e); + String json = null; + try { + json = jsonMapper.writerWithDefaultPrettyPrinter().writeValueAsString(METRIC_REGISTRY); + } catch (JsonProcessingException e) { + LOGGER.error("Error converting metrics to JSON", e); + return; + } + // Metrics are first dumped to a temp file which is then renamed to the destination + try { + tmpFile = Files.createTempFile(tmpDir, "smetrics", "json", FILE_ATTRS); + } catch (IOException e) { + LOGGER.error("failed to create temp file for JSON metrics", e); + return; + } catch (SecurityException e) { + // This shouldn't ever happen + LOGGER.error("failed to create temp file for JSON metrics: no permissions", e); + return; + } catch (UnsupportedOperationException e) { + // This shouldn't ever happen + LOGGER.error("failed to create temp file for JSON metrics: operartion not supported", e); + return; + } + + try (BufferedWriter bw = new BufferedWriter(new FileWriter(tmpFile.toFile()))) { + bw.write(json); + } + + // Move temp file to the destination file + Files.move(tmpFile, path, StandardCopyOption.REPLACE_EXISTING); + } catch (Throwable t) { + // catch all errors (throwable and execptions to prevent subsequent tasks from being suppressed) + LOGGER.error("Error executing scheduled task ", t); + } finally { + // If something happened and we were not able to rename the temp file, attempt to remove it + if (tmpFile != null && tmpFile.toFile().exists()) { + // Attempt to delete temp file, if this fails, not much can be done about it. + try { + Files.delete(tmpFile); + } catch (Exception e) { + LOGGER.error("failed to delete yemporary metrics file {}", tmpFile, e); + } + } } } @@ -342,6 +399,11 @@ public final class SentryMetrics { JSON_REPORTER_THREAD_NAME, 1, TimeUnit.MINUTES, LOGGER); executor = null; } + try { + Files.delete(path); + } catch (IOException e) { + LOGGER.error("Unable to delete {}", path, e); + } } } }
