You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by tgravescs <gi...@git.apache.org> on 2014/04/24 22:20:20 UTC
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
GitHub user tgravescs opened a pull request:
https://github.com/apache/spark/pull/538
SPARK-1557 Set permissions on event log files/directories
This adds minimal setting of event log directory/files permissions. To have a secure environment the user must manually create the top level event log directory and set permissions up. We can add logic to do that automatically later if we want.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tgravescs/spark SPARK-1557
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/538.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #538
----
commit 3150ed61a40d0c5fdd1f324ee7e557e47d3b7fd0
Author: Thomas Graves <tg...@apache.org>
Date: 2014-04-24T18:55:31Z
SPARK-1557 Set permissions on event log files/directories
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11982783
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -51,10 +52,11 @@ private[spark] class EventLoggingListener(
private val logBaseDir = conf.get("spark.eventLog.dir", "/tmp/spark-events").stripSuffix("/")
private val name = appName.replaceAll("[ :/]", "-").toLowerCase + "-" + System.currentTimeMillis
val logDir = logBaseDir + "/" + name
+ val LOG_FILE_PERMISSIONS: FsPermission = FsPermission.createImmutable(0770: Short)
--- End diff --
Do we plan to make this configurable? @pwendell
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41391542
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11982714
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -79,16 +81,23 @@ private[spark] class FileLogger(
if (!fileSystem.mkdirs(path)) {
throw new IOException("Error in creating log directory: %s".format(logDir))
}
+ if (dirPerms.isDefined) {
+ val fsStatus = fileSystem.getFileStatus(path)
+ if (fsStatus.getPermission().toShort() != dirPerms.get.toShort()) {
+ fileSystem.setPermission(path, dirPerms.get);
+ }
+ }
}
/**
* Create a new writer for the file identified by the given path.
*/
- private def createWriter(fileName: String): PrintWriter = {
+ private def createWriter(fileName: String, perms: Option[FsPermission] = None): PrintWriter = {
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41439902
Thanks again @tgraves. Pending the small changes this LGTM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12016242
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -51,10 +52,12 @@ private[spark] class EventLoggingListener(
private val logBaseDir = conf.get("spark.eventLog.dir", "/tmp/spark-events").stripSuffix("/")
private val name = appName.replaceAll("[ :/]", "-").toLowerCase + "-" + System.currentTimeMillis
val logDir = logBaseDir + "/" + name
+ val LOG_FILE_PERMISSIONS: FsPermission =
+ FsPermission.createImmutable(Integer.parseInt("770", 8).toShort: Short)
--- End diff --
Also I think it's better if we move this to `object EventLoggingListener`, alongside with other scary all caps variables.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11994917
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -150,15 +159,16 @@ private[spark] class FileLogger(
/**
* Start a writer for a new file, closing the existing one if it exists.
* @param fileName Name of the new file, defaulting to the file index if not provided.
+ * @param perms Permissions to put on the new file.
*/
- def newFile(fileName: String = "") {
+ def newFile(fileName: String = "", perms: Option[FsPermission] = None) {
--- End diff --
Yep intent was to allow permissions to be set at file level if someone wanted.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by techaddict <gi...@git.apache.org>.
Github user techaddict commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12007808
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -51,10 +52,11 @@ private[spark] class EventLoggingListener(
private val logBaseDir = conf.get("spark.eventLog.dir", "/tmp/spark-events").stripSuffix("/")
private val name = appName.replaceAll("[ :/]", "-").toLowerCase + "-" + System.currentTimeMillis
val logDir = logBaseDir + "/" + name
+ val LOG_FILE_PERMISSIONS: FsPermission = FsPermission.createImmutable(0770: Short)
--- End diff --
IMO `0770: Short` should be written as `Integer.parseInt("700", 8): Short`
For more details check @6e101f1183f92769779bc8ac14813c063bf1ff3f
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11982722
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -97,11 +106,11 @@ private[spark] class FileLogger(
// Second parameter is whether to append
new FileOutputStream(uri.getPath, !overwrite)
} else {
- val path = new Path(logPath)
hadoopDataStream = Some(fileSystem.create(path, overwrite))
hadoopDataStream.get
}
+ if (perms.isDefined) fileSystem.setPermission(path, perms.get)
--- End diff --
How about `dirPermissions.foreach { perms => fileSystem.setPermission(path, perms }`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11982774
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -27,6 +27,7 @@ import org.apache.hadoop.fs.{FileSystem, FSDataOutputStream, Path}
import org.apache.spark.{Logging, SparkConf}
import org.apache.spark.io.CompressionCodec
+import org.apache.hadoop.fs.permission.FsPermission
--- End diff --
nit: put this with other hadoop imports
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41570226
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11982727
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -150,15 +159,16 @@ private[spark] class FileLogger(
/**
* Start a writer for a new file, closing the existing one if it exists.
* @param fileName Name of the new file, defaulting to the file index if not provided.
+ * @param perms Permissions to put on the new file.
*/
- def newFile(fileName: String = "") {
+ def newFile(fileName: String = "", perms: Option[FsPermission] = None) {
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12015408
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -51,10 +52,12 @@ private[spark] class EventLoggingListener(
private val logBaseDir = conf.get("spark.eventLog.dir", "/tmp/spark-events").stripSuffix("/")
private val name = appName.replaceAll("[ :/]", "-").toLowerCase + "-" + System.currentTimeMillis
val logDir = logBaseDir + "/" + name
+ val LOG_FILE_PERMISSIONS: FsPermission =
+ FsPermission.createImmutable(Integer.parseInt("770", 8).toShort: Short)
--- End diff --
No need to specify `: Short`; it's pretty clear what it is with `'toShort`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41327716
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14447/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41337802
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14449/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41431487
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12010013
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -51,10 +52,11 @@ private[spark] class EventLoggingListener(
private val logBaseDir = conf.get("spark.eventLog.dir", "/tmp/spark-events").stripSuffix("/")
private val name = appName.replaceAll("[ :/]", "-").toLowerCase + "-" + System.currentTimeMillis
val logDir = logBaseDir + "/" + name
+ val LOG_FILE_PERMISSIONS: FsPermission = FsPermission.createImmutable(0770: Short)
--- End diff --
his code doesn't compile for me:
[error] /Users/tgraves/Documents/workspace/tgravescs-spark/yarn/stable/../common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala:63: type mismatch;
[error] found : Int
[error] required: Short
[error] FsPermission.createImmutable(Integer.parseInt("700", 8): Short)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41565133
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41438786
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41395697
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14483/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12016182
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -97,11 +106,11 @@ private[spark] class FileLogger(
// Second parameter is whether to append
new FileOutputStream(uri.getPath, !overwrite)
} else {
- val path = new Path(logPath)
hadoopDataStream = Some(fileSystem.create(path, overwrite))
hadoopDataStream.get
}
+ perms.foreach {p => fileSystem.setPermission(path, p)}
--- End diff --
Here I would do `perms.orElse(dirPermissions).foreach { ... }`, so we fall back on the logger's permissions if they exist. That way if we intend for all our files to have one set of permissions (e.g. in event logging) we don't have to provide `Some(permissions)` every time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11982712
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -60,12 +62,12 @@ private[spark] class FileLogger(
private var writer: Option[PrintWriter] = None
- createLogDir()
+ createLogDir(dirPermissions)
--- End diff --
no need to pass this in. It's already accessible
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41327449
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11995083
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -51,10 +52,11 @@ private[spark] class EventLoggingListener(
private val logBaseDir = conf.get("spark.eventLog.dir", "/tmp/spark-events").stripSuffix("/")
private val name = appName.replaceAll("[ :/]", "-").toLowerCase + "-" + System.currentTimeMillis
val logDir = logBaseDir + "/" + name
+ val LOG_FILE_PERMISSIONS: FsPermission = FsPermission.createImmutable(0770: Short)
--- End diff --
We can make it configurable if you think its useful - let me know.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41395695
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41333543
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41565109
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/538
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r11994901
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -79,16 +81,23 @@ private[spark] class FileLogger(
if (!fileSystem.mkdirs(path)) {
throw new IOException("Error in creating log directory: %s".format(logDir))
}
+ if (dirPerms.isDefined) {
+ val fsStatus = fileSystem.getFileStatus(path)
+ if (fsStatus.getPermission().toShort() != dirPerms.get.toShort()) {
+ fileSystem.setPermission(path, dirPerms.get);
+ }
+ }
}
/**
* Create a new writer for the file identified by the given path.
*/
- private def createWriter(fileName: String): PrintWriter = {
+ private def createWriter(fileName: String, perms: Option[FsPermission] = None): PrintWriter = {
--- End diff --
The class says its a generic logging class. Left this to allow file permissions to be different per file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41438791
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14490/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41327715
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41327465
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41391530
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41570228
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14530/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12016028
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -79,16 +81,23 @@ private[spark] class FileLogger(
if (!fileSystem.mkdirs(path)) {
throw new IOException("Error in creating log directory: %s".format(logDir))
}
+ if (dirPermissions.isDefined) {
+ val fsStatus = fileSystem.getFileStatus(path)
+ if (fsStatus.getPermission().toShort() != dirPermissions.get.toShort()) {
+ fileSystem.setPermission(path, dirPermissions.get);
--- End diff --
nit: semicolon, also in the line above, do `toShort` without parentheses
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12016093
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -114,7 +118,7 @@ private[spark] class EventLoggingListener(
* In addition, create an empty special file to indicate application completion.
*/
def stop() = {
- logger.newFile(APPLICATION_COMPLETE)
+ logger.newFile(APPLICATION_COMPLETE, Some(LOG_FILE_PERMISSIONS))
--- End diff --
It's a little awkward that every time we create a new file we pass in `Some(LOG_FILE_PERMISSIONS)`. I think in `FileLogger` we should check if the logger has a permissions and default on it if it does.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12009367
--- Diff: core/src/main/scala/org/apache/spark/util/FileLogger.scala ---
@@ -79,16 +81,23 @@ private[spark] class FileLogger(
if (!fileSystem.mkdirs(path)) {
throw new IOException("Error in creating log directory: %s".format(logDir))
}
+ if (dirPerms.isDefined) {
+ val fsStatus = fileSystem.getFileStatus(path)
+ if (fsStatus.getPermission().toShort() != dirPerms.get.toShort()) {
+ fileSystem.setPermission(path, dirPerms.get);
+ }
+ }
}
/**
* Create a new writer for the file identified by the given path.
*/
- private def createWriter(fileName: String): PrintWriter = {
+ private def createWriter(fileName: String, perms: Option[FsPermission] = None): PrintWriter = {
--- End diff --
Sure
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41333555
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41337801
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/538#issuecomment-41431475
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: SPARK-1557 Set permissions on event log files/...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/538#discussion_r12011802
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -51,10 +52,11 @@ private[spark] class EventLoggingListener(
private val logBaseDir = conf.get("spark.eventLog.dir", "/tmp/spark-events").stripSuffix("/")
private val name = appName.replaceAll("[ :/]", "-").toLowerCase + "-" + System.currentTimeMillis
val logDir = logBaseDir + "/" + name
+ val LOG_FILE_PERMISSIONS: FsPermission = FsPermission.createImmutable(0770: Short)
--- End diff --
I'll update to use something similar, just need to make it a short first.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---