[GitHub] twill issue #48: (TWILL-189) Allows secure store update with different UGI
Github user albertshau commented on the issue: https://github.com/apache/twill/pull/48 just a couple comments --- 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 #48: (TWILL-189) Allows secure store update with differen...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/48#discussion_r109223872
--- Diff:
twill-api/src/main/java/org/apache/twill/api/security/SecureStoreRenewer.java
---
@@ -0,0 +1,40 @@
+/*
+ * 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.twill.api.security;
+
+import org.apache.twill.api.RunId;
+import org.apache.twill.api.SecureStore;
+import org.apache.twill.filesystem.Location;
+
+import java.io.IOException;
+
+/**
+ * This class is responsible for renewing the secure store used by
application.
+ */
+public abstract class SecureStoreRenewer {
+
+ /**
+ * Renew the secure store for an application run. It must use the {@link
Location} provided by the
--- End diff --
incomplete javadoc
---
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 #48: (TWILL-189) Allows secure store update with differen...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/48#discussion_r109225715
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java ---
@@ -265,6 +254,44 @@ public void cancel() {
}
@Override
+ public Cancellable setSecureStoreRenewer(SecureStoreRenewer renewer,
long initialDelay,
+ long delay, long retryDelay,
TimeUnit unit) {
+synchronized (this) {
+ if (secureStoreScheduler != null) {
+// Shutdown and block until the schedule is stopped
+stopScheduler(secureStoreScheduler);
+
+Futures.getUnchecked(secureStoreScheduler.submit(new Runnable() {
--- End diff --
what is this doing? isn't it shutdown by stopScheduler()?
---
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 #47: (TWILL-223) Make FileContextLocationFactory UGI awar...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/47#discussion_r108811273
--- Diff:
twill-yarn/src/main/java/org/apache/twill/filesystem/FileContextLocationFactory.java
---
@@ -103,16 +138,32 @@ public Location create(URI uri) {
@Override
public Location getHomeLocation() {
+FileContext fc = getFileContext();
// Fix for TWILL-163. FileContext.getHomeDirectory() uses
System.getProperty("user.name") instead of UGI
return new FileContextLocation(this, fc,
new
Path(fc.getHomeDirectory().getParent(), fc.getUgi().getShortUserName()));
}
/**
- * Returns the {@link FileContext} used by this {@link LocationFactory}.
+ * Returns the {@link FileContext} for the current user based on {@link
UserGroupInformation#getCurrentUser()}.
+ *
+ * @throws IllegalStateException if failed to determine the current user
or fail to create the FileContext.
+ * @throws RuntimeException if failed to get the {@link FileContext}
object for the current user due to exception
*/
public FileContext getFileContext() {
-return fc;
+try {
+ return
fileContextCache.getUnchecked(UserGroupInformation.getCurrentUser());
+} catch (IOException e) {
+ throw new IllegalStateException("Failed to get current user
information", e);
+} catch (UncheckedExecutionException e) {
+ Throwable cause = e.getCause();
+ if (cause instanceof UnsupportedFileSystemException) {
+String defaultURI =
configuration.get(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY,
+
CommonConfigurationKeysPublic.FS_DEFAULT_NAME_DEFAULT);
+throw new IllegalStateException("File system with URI '" +
defaultURI + "' is not supported", cause);
+ }
+ throw new RuntimeException(cause);
--- End diff --
at this point the cause should be a RuntimeException right? in which case
we should just propagate it directly rather than wrapping 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 pull request #26: (TWILL-208) add Location.mkdirs(String permissions)
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/26#discussion_r97681685
--- Diff:
twill-common/src/main/java/org/apache/twill/filesystem/Location.java ---
@@ -207,6 +207,21 @@
boolean mkdirs() throws IOException;
/**
+ * Creates the directory named by this abstract pathname, including any
necessary
+ * but nonexistent parent directories.
+ *
+ * @param permission A permission string. It has to be either a three
digit or a nine character string.
+ * For the three digit string, it is similar to the
UNIX permission numeric representation.
+ * The first digit is the permission for owner, second
digit is the permission for group and
+ * the third digit is the permission for all.
+ * For the nine character string, it uses the format
as specified by the
+ * {@link PosixFilePermissions#fromString(String)}
method.
+ *
+ * @return true if and only if the renaming succeeded; false otherwise
--- End diff --
update javadoc
---
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 #21: (TWILL-63) Speed up application launch time
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/21#discussion_r95048004
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/ApplicationBundler.java ---
@@ -97,10 +76,10 @@ public ApplicationBundler(Iterable
excludePackages) {
}
/**
- * Constructs a ApplicationBundler.
+ * Constructs an ApplicationBundler.
*
* @param excludePackages Class packages to exclude
- * @param includePackages Class packages that should be included.
Anything in this list will override the
+ * @param includePackages Class packages that should be includwwed.
Anything in this list will override the
--- End diff --
typo
---
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 #21: (TWILL-63) Speed up application launch time
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/21#discussion_r95050388
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/LocationCacheCleaner.java ---
@@ -0,0 +1,206 @@
+/*
+ * 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.twill.yarn;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Predicate;
+import com.google.common.util.concurrent.AbstractIdleService;
+import com.google.common.util.concurrent.Futures;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.twill.api.Configs;
+import org.apache.twill.common.Threads;
+import org.apache.twill.filesystem.Location;
+import org.apache.twill.internal.io.LocationCache;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Responsible for cleanup of {@link LocationCache}.
+ */
+final class LocationCacheCleaner extends AbstractIdleService {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(LocationCacheCleaner.class);
+
+ private final Location cacheBaseLocation;
+ private final String sessionId;
+ private final long expiry;
+ private final long antiqueExpiry;
+ private final Predicate cleanupPredicate;
+ private final Set pendingCleanups;
+ private ScheduledExecutorService scheduler;
+
+ LocationCacheCleaner(Configuration config, Location cacheBaseLocation,
+ String sessionId, Predicate
cleanupPredicate) {
+this.cacheBaseLocation = cacheBaseLocation;
+this.sessionId = sessionId;
+this.expiry = config.getLong(Configs.Keys.LOCATION_CACHE_EXPIRY_MS,
+
Configs.Defaults.LOCATION_CACHE_EXPIRY_MS);
+this.antiqueExpiry =
config.getLong(Configs.Keys.LOCATION_CACHE_ANTIQUE_EXPIRY_MS,
+
Configs.Defaults.LOCATION_CACHE_ANTIQUE_EXPIRY_MS);
+this.cleanupPredicate = cleanupPredicate;
+this.pendingCleanups = new HashSet<>();
+ }
+
+ @Override
+ protected void startUp() throws Exception {
+scheduler =
Executors.newSingleThreadScheduledExecutor(Threads.createDaemonThreadFactory("location-cache-cleanup"));
+scheduler.execute(new Runnable() {
+ @Override
+ public void run() {
+long currentTime = System.currentTimeMillis();
+cleanup(currentTime);
+
+// By default, run the cleanup at half of the expiry
+long scheduleDelay = expiry / 2;
+for (PendingCleanup pendingCleanup : pendingCleanups) {
+ // If there is any pending cleanup that needs to be cleanup
early, schedule the run earlier.
+ if (pendingCleanup.getExpireTime() - currentTime <
scheduleDelay) {
+scheduleDelay = pendingCleanup.getExpireTime() - currentTime;
+ }
+}
+scheduler.schedule(this, scheduleDelay, TimeUnit.MILLISECONDS);
+ }
+});
+ }
+
+ @Override
+ protected void shutDown() throws Exception {
+scheduler.shutdownNow();
+ }
+
+ @VisibleForTesting
+ void forceCleanup(final long currentTime) {
+Futures.getUnchecked(scheduler.submit(new Runnable() {
+ @Override
+ public void run() {
+cleanup(currentTime);
+ }
+}));
+ }
+
+ /**
+ * Performs cleanup based on the given time.
+ */
+ private void cleanup(long currentTime) {
+// First go through the pending cleanup list and remove those that can
be removed
+Iterator iterator = pendingCleanups.itera
[GitHub] twill pull request #21: (TWILL-63) Speed up application launch time
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/21#discussion_r95050605
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java ---
@@ -277,9 +281,16 @@ public TwillPreparer prepare(TwillApplication
application) {
Preconditions.checkState(serviceDelegate.isRunning(), "Service not
start. Please call start() first.");
final TwillSpecification twillSpec = application.configure();
final String appName = twillSpec.getName();
+RunId runId = RunIds.generate();
+Location appLocation = locationFactory.create(String.format("/%s/%s",
twillSpec.getName(), runId.getId()));
+LocationCache locationCache = this.locationCache;
+if (locationCache == null) {
+ locationCache = new NoCachingLocationCache(appLocation);
--- End diff --
nit: extra space
---
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 #16: [TWILL-199] Handle offset error and return next offs...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/16#discussion_r90151909
--- Diff:
twill-core/src/main/java/org/apache/twill/kafka/client/KafkaConsumer.java ---
@@ -35,8 +35,10 @@
* Invoked when new messages is available.
* @param messages Iterator of new messages. The {@link
FetchedMessage} instance maybe reused in the Iterator
* and across different invocation.
+ * @return A long larger than zero as the offset to restart fetching
messages when offset error is caught,
+ * {@code -1} if the error cannot be resolved. Returns {@code
0} if no need to restart fetching.
*/
-void onReceived(Iterator messages);
+long onReceived(Iterator messages);
--- End diff --
-1 for latest and -2 for earliest seems to make sense. 0 is a valid kafka
offset, though I suppose practically speaking, any use of 0 could be replaced
by -2.
It is unclear what an 'error' means. Does it mean the consumer should stop
consuming? I think we can just leave that out.
---
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 #16: [TWILL-199] Handle offset error and return next offs...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/16#discussion_r90152039
--- Diff:
twill-core/src/main/java/org/apache/twill/kafka/client/KafkaConsumer.java ---
@@ -35,8 +35,10 @@
* Invoked when new messages is available.
* @param messages Iterator of new messages. The {@link
FetchedMessage} instance maybe reused in the Iterator
* and across different invocation.
+ * @return A long larger than zero as the offset to restart fetching
messages when offset error is caught,
--- End diff --
what happens if the offset returned is out of bounds? Should document the
expected behavior in such circumstances. Looking at the implementation, it
seems like we read from the earliest offset.
---
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 #16: [TWILL-199] Handle offset error and return next offs...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/16#discussion_r90149679
--- Diff:
twill-core/src/main/java/org/apache/twill/kafka/client/KafkaOffsetProvider.java
---
@@ -0,0 +1,36 @@
+/*
+ * 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.twill.kafka.client;
+
+/**
+ * Define interface that could provide a method to check whether a message
meets a given condition. If the condition is
+ * not met, the method will return the next offset to continue searching
for the message meeting this condition.
+ */
+public interface KafkaOffsetProvider {
--- End diff --
doesn't look like this interface belongs in Twill
---
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 #8: (TWILL-177) Make ZKDiscoveryService AutoCloseable
Github user albertshau commented on the issue: https://github.com/apache/twill/pull/8 one minor comment, otherwise 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 #8: (TWILL-177) Make ZKDiscoveryService AutoCloseable
Github user albertshau commented on a diff in the pull request: https://github.com/apache/twill/pull/8#discussion_r78623437 --- Diff: twill-yarn/src/test/java/org/apache/twill/yarn/YarnTestSuite.java --- @@ -17,8 +17,15 @@ */ package org.apache.twill.yarn; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.rules.ExternalResource; +import org.junit.rules.TestName; --- End diff -- unused 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] twill issue #9: (TWILL-173) Have EmbeddedKafkaServer restart multiple times ...
Github user albertshau commented on the issue: https://github.com/apache/twill/pull/9 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 #9: (TWILL-173) Have EmbeddedKafkaServer restart multiple...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/9#discussion_r77083133
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/kafka/EmbeddedKafkaServer.java
---
@@ -65,9 +72,19 @@ protected void startUp() throws Exception {
if (rootCause instanceof ZkTimeoutException) {
// Potentially caused by race condition bug described in
TWILL-139.
LOG.warn("Timeout when connecting to ZooKeeper from KafkaServer.
Attempt number {}.", tries, rootCause);
+} else if (rootCause instanceof BindException) {
+ LOG.warn("Kafka failed to bind to port {}. Attempt number {}.",
kafkaConfig.port(), tries, rootCause);
} else {
throw e;
}
+
+// Do a random sleep of < 200ms
+TimeUnit.MILLISECONDS.sleep(new Random().nextInt(200) + 1L);
+
+// Generate a new port for the Kafka
+int port = Networks.getRandomPort();
+Preconditions.checkState(port > 0, "Failed to get random port.");
+properties.setProperty("port", Integer.toString(port));
--- End diff --
Should we only do this if its originally set to 0 or left empty? Without
reading the code I would expect it to connect to the port I set, or not connect
at all.
Or is this not a concern because the port is not used to connect?
---
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 #9: (TWILL-173) Have EmbeddedKafkaServer restart multiple...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/9#discussion_r77081138
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/kafka/EmbeddedKafkaServer.java
---
@@ -65,9 +72,19 @@ protected void startUp() throws Exception {
if (rootCause instanceof ZkTimeoutException) {
// Potentially caused by race condition bug described in
TWILL-139.
LOG.warn("Timeout when connecting to ZooKeeper from KafkaServer.
Attempt number {}.", tries, rootCause);
+} else if (rootCause instanceof BindException) {
+ LOG.warn("Kafka failed to bind to port {}. Attempt number {}.",
kafkaConfig.port(), tries, rootCause);
} else {
throw e;
}
+
+// Do a random sleep of < 200ms
+TimeUnit.MILLISECONDS.sleep(new Random().nextInt(200) + 1L);
+
+// Generate a new port for the Kafka
+int port = Networks.getRandomPort();
+Preconditions.checkState(port > 0, "Failed to get random port.");
+properties.setProperty("port", Integer.toString(port));
--- End diff --
so "port" is required to be in the Properties passed to KafkaConfig? Is
there any place where a port is being set in the Properties passed to the
constructor, and then used somewhere else?
---
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 #3: (TWILL-188) Added methods to Location for manipulation of pe...
Github user albertshau commented on the issue: https://github.com/apache/twill/pull/3 only minor comments about javadocs, otherwise 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 #3: (TWILL-188) Added methods to Location for manipulatio...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/3#discussion_r76164629
--- Diff:
twill-common/src/main/java/org/apache/twill/filesystem/Location.java ---
@@ -53,11 +54,42 @@
* Atomically creates a new, empty file named by this abstract pathname
if and only if a file with this name
* does not yet exist.
* @return {@code true} if the file is successfully create, {@code
false} otherwise.
- * @throws IOException
+ * @throws IOException if error encountered during creationg of the file
*/
boolean createNew() throws IOException;
/**
+ * Atomically creates a new, empty file named by this abstract pathname
if and only if a file with this name
+ * does not yet exist. The newly created file will have permission set
based on the given permission settings.
+ *
+ * @param permission A permission string. It has to be either a three
digits or a nine characters string.
+ * For the three digits string, it is similar to the
UNIX permission numeric representation.
+ * The first digit is the permission for owner, second
digit is the permission for group and
+ * the third digit is the permission for all.
+ * For the nine characters string, it uses the format
as specified by the
+ * {@link PosixFilePermissions#fromString(String)}
method.
+ * @return {@code true} if the file is successfully create, {@code
false} otherwise.
+ * @throws IOException if error encountered during creationg of the file
+ */
+ boolean createNew(String permission) throws IOException;
+
+ /**
+ * Returns the permissions of this {@link Location}. The permission
string is a nine characters string as the format
--- End diff --
nine characters -> nine character
---
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 #3: (TWILL-188) Added methods to Location for manipulatio...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/3#discussion_r76164602
--- Diff:
twill-common/src/main/java/org/apache/twill/filesystem/Location.java ---
@@ -53,11 +54,42 @@
* Atomically creates a new, empty file named by this abstract pathname
if and only if a file with this name
* does not yet exist.
* @return {@code true} if the file is successfully create, {@code
false} otherwise.
- * @throws IOException
+ * @throws IOException if error encountered during creationg of the file
*/
boolean createNew() throws IOException;
/**
+ * Atomically creates a new, empty file named by this abstract pathname
if and only if a file with this name
+ * does not yet exist. The newly created file will have permission set
based on the given permission settings.
+ *
+ * @param permission A permission string. It has to be either a three
digits or a nine characters string.
+ * For the three digits string, it is similar to the
UNIX permission numeric representation.
--- End diff --
three digits -> three digit
---
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 #3: (TWILL-188) Added methods to Location for manipulatio...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/3#discussion_r76164643
--- Diff:
twill-common/src/main/java/org/apache/twill/filesystem/Location.java ---
@@ -73,7 +105,12 @@
* Creates an {@link OutputStream} for this location with the given
permission. The actual permission supported
* depends on implementation.
*
- * @param permission A POSIX permission string.
+ * @param permission A permission string. It has to be either a three
digits or a nine characters string.
+ * For the three digits string, it is similar to the
UNIX permission numeric representation.
--- End diff --
three digit, nine character
---
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 #3: (TWILL-188) Added methods to Location for manipulatio...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/3#discussion_r76164566
--- Diff:
twill-common/src/main/java/org/apache/twill/filesystem/Location.java ---
@@ -53,11 +54,42 @@
* Atomically creates a new, empty file named by this abstract pathname
if and only if a file with this name
* does not yet exist.
* @return {@code true} if the file is successfully create, {@code
false} otherwise.
- * @throws IOException
+ * @throws IOException if error encountered during creationg of the file
--- End diff --
typo: creation
---
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 #3: (TWILL-188) Added methods to Location for manipulatio...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/3#discussion_r76164518
--- Diff:
twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java ---
@@ -255,4 +288,53 @@ public int hashCode() {
public String toString() {
return file.toString();
}
+
+ /**
+ * Ensures the given {@link File} is a directory. If it doesn't exist,
it will be created.
+ */
+ private void ensureDirectory(File dir) throws IOException {
+// Check, create, check to resolve race conditions if there are
concurrent creation of the directory.
+if (!dir.isDirectory() && !dir.mkdirs() && !dir.isDirectory()) {
+ throw new IOException("Failed to create directory " + dir);
+}
+ }
+
+ /**
+ * Parses the given permission to a set of {@link PosixFilePermission}.
+ *
+ * @param permission the permission as passed to the {@link
#createNew(String)} or {@link #getOutputStream(String)}
+ * methods.
+ * @return a new set of {@link PosixFilePermission}.
+ */
+ private Set parsePermissions(String permission) {
+Set permissions;
+if (permission.length() == 3) {
+ permissions = parseNumericPermissions(permission);
+} else if (permission.length() == 9) {
+ permissions = PosixFilePermissions.fromString(permission);
+} else {
+ throw new IllegalArgumentException("Invalid permission " +
permission +
+ ". Permission should either be
a three digits or nine characters string.");
+}
+
+return permissions;
+ }
+
+ /**
+ * Parses a three digits UNIX numeric permission representation to a set
of {@link PosixFilePermission}.
+ */
+ private Set parseNumericPermissions(String
permission) {
+String posixPermission = "";
+for (int i = 0; i < 3; i++) {
+ int digit = permission.charAt(i) - '0';
+ if (digit < 0 || digit > 7) {
+throw new IllegalArgumentException("Invalid permission " +
permission +
+ ". Only digits between 0-7 is
allowed.");
--- End diff --
is -> are
---
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 #3: (TWILL-188) Added methods to Location for manipulatio...
Github user albertshau commented on a diff in the pull request:
https://github.com/apache/twill/pull/3#discussion_r76164526
--- Diff:
twill-common/src/main/java/org/apache/twill/filesystem/LocalLocation.java ---
@@ -255,4 +288,53 @@ public int hashCode() {
public String toString() {
return file.toString();
}
+
+ /**
+ * Ensures the given {@link File} is a directory. If it doesn't exist,
it will be created.
+ */
+ private void ensureDirectory(File dir) throws IOException {
+// Check, create, check to resolve race conditions if there are
concurrent creation of the directory.
+if (!dir.isDirectory() && !dir.mkdirs() && !dir.isDirectory()) {
+ throw new IOException("Failed to create directory " + dir);
+}
+ }
+
+ /**
+ * Parses the given permission to a set of {@link PosixFilePermission}.
+ *
+ * @param permission the permission as passed to the {@link
#createNew(String)} or {@link #getOutputStream(String)}
+ * methods.
+ * @return a new set of {@link PosixFilePermission}.
+ */
+ private Set parsePermissions(String permission) {
+Set permissions;
+if (permission.length() == 3) {
+ permissions = parseNumericPermissions(permission);
+} else if (permission.length() == 9) {
+ permissions = PosixFilePermissions.fromString(permission);
+} else {
+ throw new IllegalArgumentException("Invalid permission " +
permission +
+ ". Permission should either be
a three digits or nine characters string.");
--- End diff --
digits -> digit
---
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.
---
