[GitHub] twill issue #26: (TWILL-208) add Location.mkdirs(String permissions)
Github user gokulavasan commented on the issue: https://github.com/apache/twill/pull/26 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] twill pull request #26: (TWILL-208) add Location.mkdirs(String permissions)
Github user gokulavasan commented on a diff in the pull request:
https://github.com/apache/twill/pull/26#discussion_r97885713
--- Diff:
twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java ---
@@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String
pathBase) throws Exceptio
// TODO (TWILL-209): figure out how to make MiniDFSCluster enforce
permissions
@Ignore
- @Test(expected = Exception.class)
public void testPermissions() throws IOException {
// create a directory that does not permit anything
dfsCluster.getFileSystem().mkdir(new Path("/a"),
FsPermission.valueOf("--"));
-// creating a subdir should fail because even the owner has no write
permission
-dfsCluster.getFileSystem().mkdir(new Path("/a/b"),
FsPermission.valueOf("--"));
+boolean succeeded = false;
+try {
+ // creating a subdir should fail because even the owner has no write
permission
+ dfsCluster.getFileSystem().mkdir(new Path("/a/b"),
FsPermission.valueOf("--"));
+ succeeded = true;
+} catch (Exception e) {
+ // expected; TODO: figure out which exception should the expected
here, it's not documented
+}
+if (succeeded) {
--- End diff --
No it won't I think. Since Assert.fail throws AssertionError which doesn't
extend Exception?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #26: (TWILL-208) add Location.mkdirs(String permissions)
Github user gokulavasan commented on a diff in the pull request:
https://github.com/apache/twill/pull/26#discussion_r97880069
--- Diff:
twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java ---
@@ -55,11 +55,19 @@ protected LocationFactory createLocationFactory(String
pathBase) throws Exceptio
// TODO (TWILL-209): figure out how to make MiniDFSCluster enforce
permissions
@Ignore
- @Test(expected = Exception.class)
public void testPermissions() throws IOException {
// create a directory that does not permit anything
dfsCluster.getFileSystem().mkdir(new Path("/a"),
FsPermission.valueOf("--"));
-// creating a subdir should fail because even the owner has no write
permission
-dfsCluster.getFileSystem().mkdir(new Path("/a/b"),
FsPermission.valueOf("--"));
+boolean succeeded = false;
+try {
+ // creating a subdir should fail because even the owner has no write
permission
+ dfsCluster.getFileSystem().mkdir(new Path("/a/b"),
FsPermission.valueOf("--"));
+ succeeded = true;
+} catch (Exception e) {
+ // expected; TODO: figure out which exception should the expected
here, it's not documented
+}
+if (succeeded) {
--- End diff --
nit: you can move the Assert.fail to line 65 and remove the boolean
variable.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #26: (TWILL-208) add Location.mkdirs(String permissions)
Github user gokulavasan commented on a diff in the pull request:
https://github.com/apache/twill/pull/26#discussion_r97855245
--- Diff:
twill-yarn/src/test/java/org/apache/twill/filesystem/HDFSLocationTest.java ---
@@ -47,4 +52,14 @@ public static void finish() {
protected LocationFactory createLocationFactory(String pathBase) throws
Exception {
return new HDFSLocationFactory(dfsCluster.getFileSystem(), pathBase);
}
+
+ // TODO (TWILL-209): figure out how to make MiniDFSCluster enforce
permissions
+ @Ignore
+ @Test(expected = Exception.class)
+ public void testPermissions() throws IOException {
+// create a directory that does not permit anything
+dfsCluster.getFileSystem().mkdir(new Path("/a"),
FsPermission.valueOf("--"));
--- End diff --
How do we know for sure that the exception was not thrown by this operation?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #22: (TWILL-187) Added max start time
Github user gokulavasan commented on a diff in the pull request:
https://github.com/apache/twill/pull/22#discussion_r95049555
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillPreparer.java
---
@@ -257,8 +258,19 @@
TwillPreparer setLogLevels(String runnableName, Map<String,
LogEntry.Level> logLevelsForRunnable);
/**
- * Starts the application.
+ * Starts the application. It's the same as calling {@link #start(long,
TimeUnit)} with timeout of 60 seconds.
--- End diff --
Is there any constant that we can refer to for '60' ?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill issue #10: TWILL-107 Add payload support for Discoverable
Github user gokulavasan commented on the issue: https://github.com/apache/twill/pull/10 @hsaputra I have added some description to the PR. Sorry. Thank you for catching it. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] twill issue #10: TWILL-107 Add payload support for Discoverable
Github user gokulavasan commented on the issue: https://github.com/apache/twill/pull/10 Thanks for the review @chtyim. Addressed comments. Please take a look again. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] twill pull request #10: TWILL-107 Add payload support for Discoverable
GitHub user gokulavasan opened a pull request: https://github.com/apache/twill/pull/10 TWILL-107 Add payload support for Discoverable JIRA : https://issues.apache.org/jira/browse/TWILL-107 You can merge this pull request into a Git repository by running: $ git pull https://github.com/gokulavasan/twill feature/twill-107 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/10.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 #10 commit 5f82fe12e12a6cf42ad7ac43cde4e9b3fb0fec53 Author: Gokul Gunasekaran <go...@cask.co> Date: 2016-09-09T01:12:43Z TWILL-107 Add payload support for Discoverable Signed-off-by: Gokul Gunasekaran <go...@cask.co> --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
