sentry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ak...@apache.org
Subject sentry git commit: SENTRY-1963: Sentry JSON reporter should use regular implementation for local file system (Alex Kolbasov, reviewed by Na Li and Kalyan Kalvagadda)
Date Wed, 04 Oct 2017 00:19:49 GMT
Repository: sentry
Updated Branches:
  refs/heads/master 19efc0e44 -> 1749f7ebe


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/master
Commit: 1749f7ebe85e5d6a515ad264e69fad27b0624f06
Parents: 19efc0e
Author: Alexander Kolbasov <akolb@cloudera.com>
Authored: Tue Oct 3 17:19:31 2017 -0700
Committer: Alexander Kolbasov <akolb@cloudera.com>
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);
+      }
     }
   }
 }


Mime
View raw message