[GitHub] twill pull request #80: (TWILL-264) Fix Discoverable.hashCode implementation
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/80 (TWILL-264) Fix Discoverable.hashCode implementation You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill TWILL-264-fix-discoverable Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/80.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 #80 commit b4feee26f827fe4c8f9f667f9af05edd83305a47 Author: Terence Yim Date: 2019-01-25T18:00:00Z (TWILL-264) Fix Discoverable.hashCode implementation ---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221661663
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -345,6 +388,28 @@ private static FileSystem
getFileSystem(LocationFactory locationFactory) throws
return null;
}
+ private static void handleLogAction(Exception e) {
--- End diff --
This method is unnecessary, better log it in place. E.g. in the static
initializer above:
```java
static {
try {
Class dfsUtilsClientClazz =
Class.forName("org.apache.hadoop.hdfs.DFSUtilClient");
getHaNnRpcAddressesMethod =
dfsUtilsClientClazz.getMethod("getHaNnRpcAddresses",
Configuration.class);
hasDFSUtilClient = true;
} catch (ClassNotFoundException e) {
// Expected for Hadoop version < 2.8, hence log it as debug only to no
polluting the logs
LOG.debug("No DFSUtilClient found", e);
} catch (NoSuchMethodException e) {
// This is unexpected for not founding the getHaNnRpcAddresses method
if the DFSUtilClient class exists
LOG.warn("No DFSUtilClient.getHaNnRpcAddresses method found. Getting HA
NameNode address might fail.", e);
}
}
```
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221661290
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration
config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method
getHaNnRpcAddresses(Configuration config)
+ * @param config
+ * @return
+ */
+ private static Set>>
getEntries(Configuration config) {
+return iDFSUtilClientExists ? invoke(config) :
+DFSUtil.getHaNnRpcAddresses(config).entrySet();
+ }
+
+ private static Set>>
invoke(Configuration config) {
+try {
+ return ((Map) getHaNnRpcAddressesMethod.invoke(null,
config)).entrySet();
--- End diff --
Why casting to `Map` while the return type of this method is `Set`??
---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/71#discussion_r221661448 --- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java --- @@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException } } + /*** + * When hadoop_version > 2.8.0, class DFSUtils has no method getHaNnRpcAddresses(Configuration config) --- End diff -- This is not a very clear javadoc that describe what this method does (it should have the same javadoc as the `DFSUtil.getHaNnRpcAddresses` method). ---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/71#discussion_r221660462 --- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java --- @@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration config) throws IOException } } + /*** --- End diff -- Use two asterisks instead of three `/**` ---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/71#discussion_r221663573 --- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java --- @@ -72,7 +77,23 @@ HADOOP_26 } - private static final Logger LOG = LoggerFactory.getLogger(YarnUtils.class); + private static boolean iDFSUtilClientExists = false; // use this to judge if the hadoop version is above 2.8 --- End diff -- What does the `i` prefix mean? Better just call this `hasDFSUtilClient`. ---
[GitHub] twill pull request #71: TWILL-262 YarnUtils#cloneHaNnCredentials uses DFSUti...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/71#discussion_r221660130
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -202,6 +223,28 @@ public static void cloneHaNnCredentials(Configuration
config) throws IOException
}
}
+ /***
+ * When hadoop_version > 2.8.0, class DFSUtils has no method
getHaNnRpcAddresses(Configuration config)
+ * @param config
+ * @return
+ */
+ private static Set>>
getEntries(Configuration config) {
+return iDFSUtilClientExists ? invoke(config) :
+DFSUtil.getHaNnRpcAddresses(config).entrySet();
+ }
+
+ private static Set>>
invoke(Configuration config) {
--- End diff --
Please name this method with a more appropriate name, rather than a generic
name `invoke`.
---
[GitHub] twill pull request #61: (TWILL-244) Make resource report available to TwillR...
Github user chtyim closed the pull request at: https://github.com/apache/twill/pull/61 ---
[GitHub] twill pull request #68: (TWILL-258) Use loopback address for ZK server
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/68 (TWILL-258) Use loopback address for ZK server - Fix a race condition in the LocationCacheTest - Fix race condition in LogLevelChangeTestRun You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-258 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/68.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 #68 commit 0172a5d1f549ea2485fdce35034d1dec61a26ba3 Author: Terence Yim <chtyim@...> Date: 2018-03-19T07:28:20Z (TWILL-258) Use loopback address for ZK server - Fix a race condition in the LocationCacheTest - Fix race condition in LogLevelChangeTestRun ---
[GitHub] twill pull request #67: (TWILL-61) Fix to allow higher attempts to relaunch ...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/67#discussion_r174644467
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterMain.java
---
@@ -170,45 +167,7 @@ private ApplicationKafkaService(ZKClient zkClient,
String kafkaZKConnect) {
protected void startUp() throws Exception {
// Create the ZK node for Kafka to use. If the node already exists,
delete it to make sure there is
// no left over content from previous AM attempt.
- final SettableOperationFuture completion =
SettableOperationFuture.create(kafkaZKPath,
-
Threads.SAME_THREAD_EXECUTOR);
- LOG.info("Preparing Kafka ZK path {}{}",
zkClient.getConnectString(), kafkaZKPath);
--- End diff --
oh yeah, accidentally removed.
---
[GitHub] twill issue #67: (TWILL-61) Fix to allow higher attempts to relaunch the app...
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/67 I've addressed the comments and fixed one more issue (one deletion failure, if the node not exist, we can just go ahead and recreate the node instead of failing). Also refactored the callback code a bit to try to make it cleaner. ---
[GitHub] twill pull request #67: (TWILL-61) Fix to allow higher attempts to relaunch ...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/67#discussion_r174626043
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/kafka/client/SimpleKafkaPublisher.java
---
@@ -159,30 +159,37 @@ public void changed(BrokerService brokerService) {
}
String newBrokerList = brokerService.getBrokerList();
- if (newBrokerList.isEmpty()) {
-LOG.warn("Broker list is empty. No Kafka producer is created.");
-return;
- }
+ // If there is no change, whether it is empty or not, just return
if (Objects.equal(brokerList, newBrokerList)) {
return;
}
- Properties props = new Properties();
- props.put("metadata.broker.list", newBrokerList);
- props.put("serializer.class", ByteBufferEncoder.class.getName());
- props.put("key.serializer.class", IntegerEncoder.class.getName());
- props.put("partitioner.class", IntegerPartitioner.class.getName());
- props.put("request.required.acks", Integer.toString(ack.getAck()));
- props.put("compression.codec", compression.getCodec());
+ Producer<Integer, ByteBuffer> newProducer = null;
+ if (!newBrokerList.isEmpty()) {
+Properties props = new Properties();
+props.put("metadata.broker.list", newBrokerList);
+props.put("serializer.class", ByteBufferEncoder.class.getName());
+props.put("key.serializer.class", IntegerEncoder.class.getName());
+props.put("partitioner.class", IntegerPartitioner.class.getName());
+props.put("request.required.acks", Integer.toString(ack.getAck()));
+props.put("compression.codec", compression.getCodec());
+
+ProducerConfig config = new ProducerConfig(props);
+newProducer = new Producer<>(config);
+ }
- ProducerConfig config = new ProducerConfig(props);
- Producer<Integer, ByteBuffer> oldProducer = producer.getAndSet(new
Producer<Integer, ByteBuffer>(config));
+ // If the broker list is empty, the producer will be set to null
+ Producer<Integer, ByteBuffer> oldProducer =
producer.getAndSet(newProducer);
if (oldProducer != null) {
oldProducer.close();
}
- LOG.info("Update Kafka producer broker list: {}", newBrokerList);
+ if (newBrokerList.isEmpty()) {
+LOG.warn("Empty Kafka producer broker list, publish will fail.");
--- End diff --
Yes
---
[GitHub] twill pull request #67: (TWILL-61) Fix to allow higher attempts to relaunch ...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/67#discussion_r174618874
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillService.java ---
@@ -216,11 +217,52 @@ protected final void shutDown() throws Exception {
return zkClient.setData(liveNodePath, serializeLiveNode());
}
+ /**
+ * Creates the live node for the service. If the node already exists, it
will be deleted before creation.
+ *
+ * @return A {@link OperationFuture} that will be completed when the
creation is done.
+ */
private OperationFuture createLiveNode() {
-String liveNodePath = getLiveNodePath();
+final String liveNodePath = getLiveNodePath();
LOG.info("Create live node {}{}", zkClient.getConnectString(),
liveNodePath);
-return ZKOperations.ignoreError(zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL),
-
KeeperException.NodeExistsException.class, liveNodePath);
+final SettableOperationFuture resultFuture =
SettableOperationFuture.create(liveNodePath,
+
Threads.SAME_THREAD_EXECUTOR);
+OperationFuture createFuture = zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL);
+Futures.addCallback(createFuture, new FutureCallback() {
+ final FutureCallback thisCallback = this;
+
+ @Override
+ public void onSuccess(String result) {
+LOG.info("Live node created {}{}", zkClient.getConnectString(),
liveNodePath);
+resultFuture.set(result);
+ }
+
+ @Override
+ public void onFailure(final Throwable createFailure) {
+if (!(createFailure instanceof
KeeperException.NodeExistsException)) {
+ resultFuture.setException(createFailure);
+}
--- End diff --
That's right. Missed it.
---
[GitHub] twill pull request #67: (TWILL-61) Fix to allow higher attempts to relaunch ...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/67#discussion_r174618395
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillService.java ---
@@ -216,11 +217,52 @@ protected final void shutDown() throws Exception {
return zkClient.setData(liveNodePath, serializeLiveNode());
}
+ /**
+ * Creates the live node for the service. If the node already exists, it
will be deleted before creation.
+ *
+ * @return A {@link OperationFuture} that will be completed when the
creation is done.
+ */
private OperationFuture createLiveNode() {
-String liveNodePath = getLiveNodePath();
+final String liveNodePath = getLiveNodePath();
LOG.info("Create live node {}{}", zkClient.getConnectString(),
liveNodePath);
-return ZKOperations.ignoreError(zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL),
-
KeeperException.NodeExistsException.class, liveNodePath);
+final SettableOperationFuture resultFuture =
SettableOperationFuture.create(liveNodePath,
+
Threads.SAME_THREAD_EXECUTOR);
+OperationFuture createFuture = zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL);
+Futures.addCallback(createFuture, new FutureCallback() {
+ final FutureCallback thisCallback = this;
+
+ @Override
+ public void onSuccess(String result) {
+LOG.info("Live node created {}{}", zkClient.getConnectString(),
liveNodePath);
+resultFuture.set(result);
+ }
+
+ @Override
+ public void onFailure(final Throwable createFailure) {
+if (!(createFailure instanceof
KeeperException.NodeExistsException)) {
+ resultFuture.setException(createFailure);
+}
+
+// If the node already exists, it is due to previous run attempt
that left an ephemeral node.
+// Try to delete the node and recreate it
+LOG.info("Live node already exist. Deleting node {}{}",
zkClient.getConnectString(), liveNodePath);
+Futures.addCallback(zkClient.delete(liveNodePath), new
FutureCallback() {
+ @Override
+ public void onSuccess(String result) {
+Futures.addCallback(zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL),
+thisCallback,
Threads.SAME_THREAD_EXECUTOR);
+ }
+
+ @Override
+ public void onFailure(Throwable t) {
+createFailure.addSuppressed(t);
+resultFuture.setException(createFailure);
+ }
+}, Threads.SAME_THREAD_EXECUTOR);
+ }
+}, Threads.SAME_THREAD_EXECUTOR);
+
+return resultFuture;
--- End diff --
I can pull the common code between this and the ApplicationMasterMain class
into a util function. But still, inside the util function, there would be three
callbacks.
---
[GitHub] twill pull request #67: (TWILL-61) Fix to allow higher attempts to relaunch ...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/67#discussion_r174618087
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillService.java ---
@@ -216,11 +217,52 @@ protected final void shutDown() throws Exception {
return zkClient.setData(liveNodePath, serializeLiveNode());
}
+ /**
+ * Creates the live node for the service. If the node already exists, it
will be deleted before creation.
+ *
+ * @return A {@link OperationFuture} that will be completed when the
creation is done.
+ */
private OperationFuture createLiveNode() {
-String liveNodePath = getLiveNodePath();
+final String liveNodePath = getLiveNodePath();
LOG.info("Create live node {}{}", zkClient.getConnectString(),
liveNodePath);
-return ZKOperations.ignoreError(zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL),
-
KeeperException.NodeExistsException.class, liveNodePath);
+final SettableOperationFuture resultFuture =
SettableOperationFuture.create(liveNodePath,
+
Threads.SAME_THREAD_EXECUTOR);
+OperationFuture createFuture = zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL);
+Futures.addCallback(createFuture, new FutureCallback() {
+ final FutureCallback thisCallback = this;
+
+ @Override
+ public void onSuccess(String result) {
+LOG.info("Live node created {}{}", zkClient.getConnectString(),
liveNodePath);
+resultFuture.set(result);
+ }
+
+ @Override
+ public void onFailure(final Throwable createFailure) {
+if (!(createFailure instanceof
KeeperException.NodeExistsException)) {
+ resultFuture.setException(createFailure);
+}
+
+// If the node already exists, it is due to previous run attempt
that left an ephemeral node.
+// Try to delete the node and recreate it
+LOG.info("Live node already exist. Deleting node {}{}",
zkClient.getConnectString(), liveNodePath);
+Futures.addCallback(zkClient.delete(liveNodePath), new
FutureCallback() {
+ @Override
+ public void onSuccess(String result) {
+Futures.addCallback(zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL),
+thisCallback,
Threads.SAME_THREAD_EXECUTOR);
+ }
+
+ @Override
+ public void onFailure(Throwable t) {
+createFailure.addSuppressed(t);
+resultFuture.setException(createFailure);
+ }
+}, Threads.SAME_THREAD_EXECUTOR);
+ }
+}, Threads.SAME_THREAD_EXECUTOR);
+
+return resultFuture;
--- End diff --
Well, we do need that many levels of callback (create -> delete -> create)
for the operation. Any suggestions on how to simplify it?
---
[GitHub] twill pull request #67: (TWILL-61) Fix to allow higher attempts to relaunch ...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/67#discussion_r174617783
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillService.java ---
@@ -216,11 +217,52 @@ protected final void shutDown() throws Exception {
return zkClient.setData(liveNodePath, serializeLiveNode());
}
+ /**
+ * Creates the live node for the service. If the node already exists, it
will be deleted before creation.
+ *
+ * @return A {@link OperationFuture} that will be completed when the
creation is done.
+ */
private OperationFuture createLiveNode() {
-String liveNodePath = getLiveNodePath();
+final String liveNodePath = getLiveNodePath();
LOG.info("Create live node {}{}", zkClient.getConnectString(),
liveNodePath);
-return ZKOperations.ignoreError(zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL),
-
KeeperException.NodeExistsException.class, liveNodePath);
+final SettableOperationFuture resultFuture =
SettableOperationFuture.create(liveNodePath,
+
Threads.SAME_THREAD_EXECUTOR);
+OperationFuture createFuture = zkClient.create(liveNodePath,
serializeLiveNode(), CreateMode.EPHEMERAL);
+Futures.addCallback(createFuture, new FutureCallback() {
+ final FutureCallback thisCallback = this;
+
+ @Override
+ public void onSuccess(String result) {
+LOG.info("Live node created {}{}", zkClient.getConnectString(),
liveNodePath);
+resultFuture.set(result);
+ }
+
+ @Override
+ public void onFailure(final Throwable createFailure) {
+if (!(createFailure instanceof
KeeperException.NodeExistsException)) {
+ resultFuture.setException(createFailure);
+}
+
+// If the node already exists, it is due to previous run attempt
that left an ephemeral node.
--- End diff --
Ephemeral node won't go away immediate if the process die. It will stay
there till ZK session timeout, which is typically multiple seconds. In the
meantime, the next AM process may already be started by YARN, hence the new AM
process will see the ephemeral node.
---
[GitHub] twill pull request #67: (TWILL-61) Fix to allow higher attempts to relaunch ...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/67 (TWILL-61) Fix to allow higher attempts to relaunch the app after the first attempt failed - Delete the Kafka root zk node for the application if already exist - Delete the AM instance zk node if already exist - For runnables parent zk node, it is not an error if it already exist - Enhance KafkaClient publisher / consumer to deal with Kafka cluster changes - When AM killed and restarted, the embedded Kafka will be running in different host and port You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-61 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/67.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 #67 commit 9decca071a9e067b30be2150a6097463c939b6af Author: Terence Yim <chtyim@...> Date: 2018-03-09T20:21:26Z (TWILL-61) Fix to allow higher attempts to relaunch the app after the first attempt failed - Delete the Kafka root zk node for the application if already exist - Delete the AM instance zk node if already exist - For runnables parent zk node, it is not an error if it already exist - Enhance KafkaClient publisher / consumer to deal with Kafka cluster changes - When AM killed and restarted, the embedded Kafka will be running in different host and port ---
[GitHub] twill issue #65: (TWILL-254) Update to use ContainerId.fromString
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/65 Changes LGTM. ---
[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString
Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/65#discussion_r166160088 --- Diff: pom.xml --- @@ -680,9 +680,9 @@ -hadoop-2.5 +hadoop-2.6 -2.5.1 +2.6.5 --- End diff -- Yes, that's correct. ---
[GitHub] twill issue #65: (TWILL-254) Update to use ContainerId.fromString
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/65 The change looks mostly ok to me, except for the change in the pom.xml profile. ---
[GitHub] twill pull request #65: (TWILL-254) Update to use ContainerId.fromString
Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/65#discussion_r166078366 --- Diff: pom.xml --- @@ -680,9 +680,9 @@ -hadoop-2.5 +hadoop-2.6 -2.5.1 +2.6.5 --- End diff -- The profile for different versions of Hadoop is both for version specific code as well as for running unit-tests. It is better to keep it and add the hadoop-2.6 ---
[GitHub] twill pull request #64: (TWILL-251) Reduce log level of YarnNMClient
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/64 (TWILL-251) Reduce log level of YarnNMClient - Also reduce the polling frequency You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-251-reduce-log Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/64.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 #64 commit 86a769db37650ff199ea5183f27c32dc0aa4cee3 Author: Terence Yim <cht...@apache.org> Date: 2017-12-01T19:21:07Z (TWILL-251) Reduce log level of YarnNMClient - Also reduce the polling frequency ---
[GitHub] twill pull request #63: (TWILL-248) Speedup shutdown of tracker service
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/63 (TWILL-248) Speedup shutdown of tracker service You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-248-speedup-shutdown Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/63.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 #63 commit ef87c6210f01b5d4c24193b4e808220973e89cdc Author: Terence Yim <cht...@apache.org> Date: 2017-10-30T22:10:34Z (TWILL-248) Speedup shutdown of tracker service ---
[GitHub] twill pull request #62: (TWILL-248) Upgrade to use Netty-4.1
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/62#discussion_r143854276
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/ResourceReportClient.java ---
@@ -52,12 +54,16 @@
public ResourceReport get() {
for (URL url : resourceUrls) {
try {
-Reader reader = new BufferedReader(new
InputStreamReader(url.openStream(), Charsets.UTF_8));
-try {
+HttpURLConnection urlConn = (HttpURLConnection)
url.openConnection();
--- End diff --
Not entirely. This is to enable compression when fetching the report.
---
[GitHub] twill pull request #62: (TWILL-248) Upgrade to use Netty-4.1
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/62 (TWILL-248) Upgrade to use Netty-4.1 - Also enable ResourceReportClient to use HTTP compression You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-248-upgrade-netty-4.1 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/62.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 #62 commit 321e7ba0116ccb7ba1c9b814ff60ad0dfd4ac3e5 Author: Terence Yim <cht...@apache.org> Date: 2017-10-10T20:26:11Z (TWILL-248) Upgrade to use Netty-4.1 - Also enable ResourceReportClient to use HTTP compression ---
[GitHub] twill pull request #61: (TWILL-244) Make resource report available to TwillR...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/61#discussion_r137130751
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/ResourceReportClient.java ---
@@ -0,0 +1,116 @@
+/*
+ * 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.internal;
+
+import com.google.common.base.Charsets;
+import org.apache.twill.api.ResourceReport;
+import org.apache.twill.api.ResourceReporter;
+import org.apache.twill.api.TwillRunResources;
+import org.apache.twill.internal.json.ResourceReportAdapter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.lang.reflect.Type;
+import java.net.URL;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import javax.annotation.Nullable;
+
+/**
+ * A {@link ResourceReporter} that fetches reports from the given set of
URLs.
+ */
+public final class ResourceReportClient implements ResourceReporter {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ResourceReportClient.class);
+
+ private final List resourceUrls;
+
+ public ResourceReportClient(List resourceUrls) {
+this.resourceUrls = resourceUrls;
+ }
+
+ @Nullable
+ @Override
+ public ResourceReport getResourceReport() {
+for (URL url : resourceUrls) {
+ try {
+return fetchURL(url, "", ResourceReport.class);
+ } catch (IOException e) {
+// Just log a trace as it's ok to not able to fetch resource report
+LOG.trace("Exception raised when getting resource report from
{}.", url, e);
+ }
+}
--- End diff --
That's the contract of the `ResourceReporter`. If the report is not
available for whatever reason, it'll just return `null` / empty collection.
---
[GitHub] twill pull request #61: (TWILL-244) Make resource report available to TwillR...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/61 (TWILL-244) Make resource report available to TwillRunnable - Make resource report available to TwillContext - Enhance tracker service to provide more resource related endpoints - Provide endpoints to fetch specific part of the report to reduce bandwidth - Added a ResourceReporter API to provide programmatic access - Make resource report available via TwillContext - Fixed a concurrent modification bug in ZKDiscoveryService.close You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-244-resource-report-runnable Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/61.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 #61 commit 53312962b33cc20c2f272b217d076730c498644d Author: Terence Yim <cht...@apache.org> Date: 2017-08-28T23:47:45Z (TWILL-244) Make resource report available to TwillContext - Enhance tracker service to provide more resource related endpoints - Provide endpoints to fetch specific part of the report to reduce bandwidth - Added a ResourceReporter API to provide programmatic access - Make resource report available via TwillContext commit 31facdf2f7f93f06c8fd131960be6d4c52fd646e Author: Terence Yim <cht...@apache.org> Date: 2017-08-29T01:33:53Z Fix a concurrent modification bug in ZKDiscoveryService.close --- 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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/58 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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132322898
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -239,14 +359,8 @@ protected void doStop() throws Exception {
LOG.info("Stop application master with spec: {}",
TwillRuntimeSpecificationAdapter.create().toJson(twillRuntimeSpec));
-if (eventHandler != null) {
- try {
-// call event handler destroy. If there is error, only log and not
affected stop sequence.
-eventHandler.destroy();
- } catch (Throwable t) {
-LOG.warn("Exception when calling {}.destroy()",
eventHandler.getClass().getName(), t);
- }
-}
+// call event handler destroy
+eventHandler.destroy();
--- End diff --
I think so.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132322316
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -239,14 +359,8 @@ protected void doStop() throws Exception {
LOG.info("Stop application master with spec: {}",
TwillRuntimeSpecificationAdapter.create().toJson(twillRuntimeSpec));
-if (eventHandler != null) {
- try {
-// call event handler destroy. If there is error, only log and not
affected stop sequence.
-eventHandler.destroy();
- } catch (Throwable t) {
-LOG.warn("Exception when calling {}.destroy()",
eventHandler.getClass().getName(), t);
- }
-}
+// call event handler destroy
+eventHandler.destroy();
--- End diff --
Shouldn't call `destroy()` here anymore. It should be called after call
other calls to the event handler, as it is acted as a cleanup call.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132322087
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -181,19 +190,130 @@ public Reader getInput() throws IOException {
}
@SuppressWarnings("unchecked")
- @Nullable
private EventHandler createEventHandler(TwillSpecification twillSpec)
throws ClassNotFoundException {
// Should be able to load by this class ClassLoader, as they packaged
in the same jar.
EventHandlerSpecification handlerSpec = twillSpec.getEventHandler();
if (handlerSpec == null) {
- return null;
+ // if no handler is specified, return an EventHandler with no-op
+ return new EventHandler() {};
}
Class handlerClass =
getClass().getClassLoader().loadClass(handlerSpec.getClassName());
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
"Class {} does not implements {}",
handlerClass,
EventHandler.class.getName());
-return Instances.newInstance((Class)
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+// wrap all calls to the delegate EventHandler methods except
initialize so that all errors will be caught
+return new EventHandler() {
+ private boolean initialized;
+
+ @Override
+ public void initialize(EventHandlerContext context) {
+delegate.initialize(context);
+initialized = true;
--- End diff --
Since any exception thrown from `initialize` will terminate the app, I
think we don't need this, right?
---
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 #60: (TWILL-242) Fixed the dropping of extra jvm opts
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/60 (TWILL-242) Fixed the dropping of extra jvm opts You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-242-fix-extra-jvm-opts Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/60.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 #60 commit 7187ed966139fd05ce4a6a8d79b65feac34545ae Author: Terence Yim <cht...@apache.org> Date: 2017-08-09T21:56:33Z (TWILL-242) Fixed the dropping of extra jvm opts --- 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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132298929
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -193,7 +200,91 @@ private EventHandler
createEventHandler(TwillSpecification twillSpec) throws Cla
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
"Class {} does not implements {}",
handlerClass,
EventHandler.class.getName());
-return Instances.newInstance((Class)
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+if (delegate == null) {
+ // if no handler is specified, return an EventHandler with no-op
+ return new EventHandler() {};
+}
+// wrap the delegate EventHandler so that all errors will be caught
+return new EventHandler() {
--- End diff --
hum.. that's true.. maybe we should keep 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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132297954
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
---
@@ -401,6 +409,24 @@ void stopAll() {
// When we acquire this lock, all stopped runnables should have been
cleaned up by handleCompleted() method
containerLock.lock();
try {
+ for (Map.Entry<String, Map<String, TwillContainerController>> entry
: containers.rowMap().entrySet()) {
+String runnableName = entry.getKey();
+Collection containerInfos =
containerStats.get(runnableName);
+for (Map.Entry<String, TwillContainerController>
containerControllerEntry : entry.getValue().entrySet()) {
+ boolean containerExist = false;
+ for (ContainerInfo containerInfo : containerInfos) {
+if
(containerInfo.getId().equals(containerControllerEntry.getKey())) {
+ containerExist = true;
+ break;
+}
+ }
+ // Only call eventHandler.containerStopped if container is not
removed by handleCompleted
+ if (containerExist) {
+eventHandler.containerStopped(runnableName,
containerControllerEntry.getValue().getInstanceId(),
--- End diff --
you can move this inside the `for` loop and no need to use `containerExist`.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132297305
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -193,7 +200,91 @@ private EventHandler
createEventHandler(TwillSpecification twillSpec) throws Cla
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
"Class {} does not implements {}",
handlerClass,
EventHandler.class.getName());
-return Instances.newInstance((Class)
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+if (delegate == null) {
+ // if no handler is specified, return an EventHandler with no-op
+ return new EventHandler() {};
+}
+// wrap the delegate EventHandler so that all errors will be caught
+return new EventHandler() {
--- End diff --
You'll need to wrap the `initialize` as well. Also, I'd suggest if
`initialize` throw exception, you'll just ignore all calls to other lifecycle
methods to avoid excessive logging.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132296875
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -193,7 +200,91 @@ private EventHandler
createEventHandler(TwillSpecification twillSpec) throws Cla
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
"Class {} does not implements {}",
handlerClass,
EventHandler.class.getName());
-return Instances.newInstance((Class)
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+if (delegate == null) {
+ // if no handler is specified, return an EventHandler with no-op
+ return new EventHandler() {};
+}
+// wrap the delegate EventHandler so that all errors will be caught
+return new EventHandler() {
+
+ @Override
+ public void started() {
+try {
+ delegate.started();
+} catch (Throwable t) {
+ LOG.warn("Exception when calling {}.started()",
eventHandler.getClass().getName(), t);
--- End diff --
`Exception raised when...`
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132296480
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -193,7 +200,91 @@ private EventHandler
createEventHandler(TwillSpecification twillSpec) throws Cla
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
"Class {} does not implements {}",
handlerClass,
EventHandler.class.getName());
-return Instances.newInstance((Class)
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+if (delegate == null) {
+ // if no handler is specified, return an EventHandler with no-op
+ return new EventHandler() {};
+}
+// wrap the delegate EventHandler so that all errors will be caught
+return new EventHandler() {
+
+ @Override
+ public void started() {
+try {
+ delegate.started();
+} catch (Throwable t) {
+ LOG.warn("Exception when calling {}.started()",
eventHandler.getClass().getName(), t);
--- End diff --
Shouldn't it be `delegate.getClass().getName()`??
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132296198
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -193,7 +200,91 @@ private EventHandler
createEventHandler(TwillSpecification twillSpec) throws Cla
Preconditions.checkArgument(EventHandler.class.isAssignableFrom(handlerClass),
"Class {} does not implements {}",
handlerClass,
EventHandler.class.getName());
-return Instances.newInstance((Class)
handlerClass);
+final EventHandler delegate = Instances.newInstance((Class) handlerClass);
+if (delegate == null) {
--- End diff --
`Instances.newInstance` never return `null`.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132295847
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -181,7 +189,6 @@ public Reader getInput() throws IOException {
}
@SuppressWarnings("unchecked")
- @Nullable
private EventHandler createEventHandler(TwillSpecification twillSpec)
throws ClassNotFoundException {
// Should be able to load by this class ClassLoader, as they packaged
in the same jar.
EventHandlerSpecification handlerSpec = twillSpec.getEventHandler();
--- End diff --
if `handlerSpec` is `null`, you still want to return a no-op handler, right?
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132037568
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -221,7 +229,8 @@ protected void doStart() throws Exception {
// initialize the event handler, if it fails, it will fail the
application.
if (eventHandler != null) {
- eventHandler.initialize(new
BasicEventHandlerContext(twillSpec.getEventHandler()));
+ eventHandler.initialize(new
BasicEventHandlerContext(twillRuntimeSpec, twillSpec.getEventHandler()));
--- End diff --
The `twillRuntimeSpec` already contains `twillSpec`. Seems unnecessary to
pass in `twillSpec.getEventHandler()` as a separate parameter.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132038521
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
---
@@ -267,6 +280,14 @@ void stopByIdAndWait(String runnableName, int
instanceId) {
resourceReport.removeRunnableResources(runnableName, containerId);
containerChange.signalAll();
+if (eventHandler != null) {
+ Integer exitStatus = containerExitStatus.get(containerId);
--- End diff --
Should remove it from the exit status map, otherwise the map can keep
growing in size.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132038164
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
---
@@ -173,6 +183,9 @@ void start(String runnableName, ContainerInfo
containerInfo, TwillContainerLaunc
startSequence.addLast(runnableName);
}
containerChange.signalAll();
+ if (eventHandler != null) {
+eventHandler.containerLaunched(runnableName, instanceId,
containerInfo.getId());
--- End diff --
How do we handle exception raised from event handler methods?
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132040768
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/EventHandlerTest.java ---
@@ -0,0 +1,351 @@
+/*
+ * 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.base.Joiner;
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableMap;
+import org.apache.twill.api.AbstractTwillRunnable;
+import org.apache.twill.api.EventHandler;
+import org.apache.twill.api.EventHandlerContext;
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillController;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.PrinterLogHandler;
+import org.junit.Assert;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Tests {@link EventHandler} methods
+ */
+public final class EventHandlerTest extends BaseYarnTest {
+ private static final Logger LOG =
LoggerFactory.getLogger(EventHandlerTest.class);
+
+ @ClassRule
+ public static final TemporaryFolder TMP_FOLDER = new TemporaryFolder();
+ public static final String STARTED_FILE = "started_file";
+ public static final String RUN_FILE = "run_file";
+ public static final String CONTAINER_LAUNCHED_FOLDER = "launched_folder";
+ public static final String CONTAINER_STOPPED_FOLDER = "stopped_folder";
+ public static final String COMPLETED_FILE = "completed_file";
+ public static final String KILLED_FILE = "killed_file";
+ public static final String ABORTED_FILE = "aborted_file";
+
+ @Test
+ public void testComplete() throws InterruptedException,
ExecutionException, TimeoutException, IOException {
+// Create a parent folder to be written by EventHandler
+File parentFolder = TMP_FOLDER.newFolder();
+parentFolder.setWritable(true, false);
+TwillController controller = getTwillRunner().prepare(new
CompleteApplication(parentFolder.getAbsolutePath()))
+ .addLogHandler(new PrinterLogHandler(new PrintWriter(System.out,
true)))
+ .start();
+
+// Wait for the app to complete within 120 seconds.
+try {
+ controller.awaitTerminated(120, TimeUnit.SECONDS);
+ Set handlerFiles = new
HashSet<>(Arrays.asList(parentFolder.list()));
+ Assert.assertEquals(5, handlerFiles.size());
+ // EventHandler#started() method should be called to create a file
+ Assert.assertTrue(handlerFiles.contains(STARTED_FILE));
+ // CompleteRunnable#run() method should be called to create a file
after EventHandler#started() method is called
+ Assert.assertTrue(handlerFiles.contains(RUN_FILE));
+ // EventHandler#containerLaunched(String, int, String) method should
be called to create a folder
+ Assert.assertTrue(handlerFiles.contains(CONTAINER_LAUNCHED_FOLDER));
+ // EventHandler#containerStopped(String, int, String, int) method
should be called to create a folder
+ Assert.assertTrue(handlerFiles.contains(CONTAINER_STOPPED_FOLDER));
+ // Assert that containerLaunched and containerStopped are called for
the same containers
+ // for the same number of times
+ String[] containerLaunchedFiles = new
File(parentFolder.getAbsolute
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132040093
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
---
@@ -267,6 +280,14 @@ void stopByIdAndWait(String runnableName, int
instanceId) {
resourceReport.removeRunnableResources(runnableName, containerId);
containerChange.signalAll();
+if (eventHandler != null) {
+ Integer exitStatus = containerExitStatus.get(containerId);
+ if (exitStatus == null) {
--- End diff --
So this is for handling the case when `handleCompleted` was not called
(e.g. the runnable container is not stopping after receiving the "stop"
command), right? If that's the case, isn't that the exit status is always not
there?
Also, do we need to do the similar logic to call event handler in the
`stopAll` method?
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132029508
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/EventHandlerTest.java ---
@@ -0,0 +1,351 @@
+/*
+ * 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.base.Joiner;
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableMap;
+import org.apache.twill.api.AbstractTwillRunnable;
+import org.apache.twill.api.EventHandler;
+import org.apache.twill.api.EventHandlerContext;
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillController;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.PrinterLogHandler;
+import org.junit.Assert;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Tests {@link EventHandler} methods
+ */
+public final class EventHandlerTest extends BaseYarnTest {
+ private static final Logger LOG =
LoggerFactory.getLogger(EventHandlerTest.class);
+
+ @ClassRule
+ public static final TemporaryFolder TMP_FOLDER = new TemporaryFolder();
+ public static final String STARTED_FILE = "started_file";
+ public static final String RUN_FILE = "run_file";
+ public static final String CONTAINER_LAUNCHED_FOLDER = "launched_folder";
+ public static final String CONTAINER_STOPPED_FOLDER = "stopped_folder";
+ public static final String COMPLETED_FILE = "completed_file";
+ public static final String KILLED_FILE = "killed_file";
+ public static final String ABORTED_FILE = "aborted_file";
+
+ @Test
+ public void testComplete() throws InterruptedException,
ExecutionException, TimeoutException, IOException {
+// Create a parent folder to be written by EventHandler
+File parentFolder = TMP_FOLDER.newFolder();
+parentFolder.setWritable(true, false);
+TwillController controller = getTwillRunner().prepare(new
CompleteApplication(parentFolder.getAbsolutePath()))
+ .addLogHandler(new PrinterLogHandler(new PrintWriter(System.out,
true)))
+ .start();
+
+// Wait for the app to complete within 120 seconds.
+try {
+ controller.awaitTerminated(120, TimeUnit.SECONDS);
+ Set handlerFiles = new
HashSet<>(Arrays.asList(parentFolder.list()));
+ Assert.assertEquals(5, handlerFiles.size());
+ // EventHandler#started() method should be called to create a file
+ Assert.assertTrue(handlerFiles.contains(STARTED_FILE));
+ // CompleteRunnable#run() method should be called to create a file
after EventHandler#started() method is called
+ Assert.assertTrue(handlerFiles.contains(RUN_FILE));
+ // EventHandler#containerLaunched(String, int, String) method should
be called to create a folder
+ Assert.assertTrue(handlerFiles.contains(CONTAINER_LAUNCHED_FOLDER));
+ // EventHandler#containerStopped(String, int, String, int) method
should be called to create a folder
+ Assert.assertTrue(handlerFiles.contains(CONTAINER_STOPPED_FOLDER));
+ // Assert that containerLaunched and containerStopped are called for
the same containers
+ // for the same number of times
+ String[] containerLaunchedFiles = new
File(parentFolder.getAbsolute
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132029465
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/EventHandlerTest.java ---
@@ -0,0 +1,351 @@
+/*
+ * 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.base.Joiner;
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableMap;
+import org.apache.twill.api.AbstractTwillRunnable;
+import org.apache.twill.api.EventHandler;
+import org.apache.twill.api.EventHandlerContext;
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillController;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.PrinterLogHandler;
+import org.junit.Assert;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Tests {@link EventHandler} methods
+ */
+public final class EventHandlerTest extends BaseYarnTest {
+ private static final Logger LOG =
LoggerFactory.getLogger(EventHandlerTest.class);
+
+ @ClassRule
+ public static final TemporaryFolder TMP_FOLDER = new TemporaryFolder();
+ public static final String STARTED_FILE = "started_file";
+ public static final String RUN_FILE = "run_file";
+ public static final String CONTAINER_LAUNCHED_FOLDER = "launched_folder";
+ public static final String CONTAINER_STOPPED_FOLDER = "stopped_folder";
+ public static final String COMPLETED_FILE = "completed_file";
+ public static final String KILLED_FILE = "killed_file";
+ public static final String ABORTED_FILE = "aborted_file";
+
+ @Test
+ public void testComplete() throws InterruptedException,
ExecutionException, TimeoutException, IOException {
+// Create a parent folder to be written by EventHandler
+File parentFolder = TMP_FOLDER.newFolder();
+parentFolder.setWritable(true, false);
+TwillController controller = getTwillRunner().prepare(new
CompleteApplication(parentFolder.getAbsolutePath()))
+ .addLogHandler(new PrinterLogHandler(new PrintWriter(System.out,
true)))
+ .start();
+
+// Wait for the app to complete within 120 seconds.
+try {
+ controller.awaitTerminated(120, TimeUnit.SECONDS);
+ Set handlerFiles = new
HashSet<>(Arrays.asList(parentFolder.list()));
+ Assert.assertEquals(5, handlerFiles.size());
+ // EventHandler#started() method should be called to create a file
+ Assert.assertTrue(handlerFiles.contains(STARTED_FILE));
+ // CompleteRunnable#run() method should be called to create a file
after EventHandler#started() method is called
+ Assert.assertTrue(handlerFiles.contains(RUN_FILE));
+ // EventHandler#containerLaunched(String, int, String) method should
be called to create a folder
+ Assert.assertTrue(handlerFiles.contains(CONTAINER_LAUNCHED_FOLDER));
+ // EventHandler#containerStopped(String, int, String, int) method
should be called to create a folder
+ Assert.assertTrue(handlerFiles.contains(CONTAINER_STOPPED_FOLDER));
+ // Assert that containerLaunched and containerStopped are called for
the same containers
+ // for the same number of times
+ String[] containerLaunchedFiles = new
File(parentFolder.getAbsolute
[GitHub] twill pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r132029365
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/EventHandlerTest.java ---
@@ -0,0 +1,351 @@
+/*
+ * 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.base.Joiner;
+import com.google.common.base.Stopwatch;
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableMap;
+import org.apache.twill.api.AbstractTwillRunnable;
+import org.apache.twill.api.EventHandler;
+import org.apache.twill.api.EventHandlerContext;
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillController;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.PrinterLogHandler;
+import org.junit.Assert;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Tests {@link EventHandler} methods
+ */
+public final class EventHandlerTest extends BaseYarnTest {
--- End diff --
Rename the test class to `EventHandlerTestRun` and add it to
`YarnTestSuite`.
---
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 #59: (TWILL-241) Added per runnable configuration and jvm option...
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/59 @anew Thanks for the review. --- 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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762189
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -291,6 +300,24 @@ public void run() {
// Since all the runnables are now stopped, it is okay to stop the
poller.
stopPoller.shutdownNow();
cleanupDir();
+if (eventHandler != null) {
+ if (stopStatus == null) {
+// if finalStatus is not set, the application must be stopped by a
SystemMessages#STOP_COMMAND
+eventHandler.killed(twillRuntimeSpec.getTwillAppName(),
twillRuntimeSpec.getTwillAppRunId());
+return;
+ }
+ switch (stopStatus) {
+case COMPLETED:
+ eventHandler.completed();
+ break;
+case ABORTED:
+eventHandler.aborted();
--- End diff --
Misalignment.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131761843
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
--- End diff --
Also please add 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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762593
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -85,13 +105,29 @@ public TimeoutAction
launchTimeout(Iterable timeoutEvents) {
return TimeoutAction.recheck(10, TimeUnit.SECONDS);
}
}
+
+@Override
+public void aborted() {
+ try {
+new
File(context.getSpecification().getConfigs().get("parentFolderPath") +
File.separator
--- End diff --
When constructing file, usually not to use `File.separator`, but instead
use the constructor `File(File parent, String path)`.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762365
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
+
+ String getTwillAppRunId();
--- End diff --
The run id should be of type `RunId`.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131762894
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -85,13 +105,29 @@ public TimeoutAction
launchTimeout(Iterable timeoutEvents) {
return TimeoutAction.recheck(10, TimeUnit.SECONDS);
}
}
+
+@Override
+public void aborted() {
+ try {
+new
File(context.getSpecification().getConfigs().get("parentFolderPath") +
File.separator
--- End diff --
Also, we are only testing abort? How about other lifecycle methods?
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131761787
--- Diff:
twill-api/src/main/java/org/apache/twill/api/EventHandlerContext.java ---
@@ -22,5 +22,9 @@
*/
public interface EventHandlerContext {
+ String getTwillAppName();
+
+ String getTwillAppRunId();
--- End diff --
Just call it `getRunId()`.
---
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 #59: (TWILL-241) Added per runnable configuration and jvm...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131511384
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -183,6 +184,13 @@ public TwillPreparer withConfiguration(Map<String,
String> config) {
}
@Override
+ public TwillPreparer withConfiguration(String runnableName, Map<String,
String> config) {
--- End diff --
Should default to the app setting first, then to the constants default
---
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 #59: (TWILL-241) Added per runnable configuration and jvm...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/59#discussion_r131511349
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -87,19 +86,46 @@ public String getTwillAppName() {
return twillAppName;
}
- public int getReservedMemory() {
-return reservedMemory;
+ /**
+ * Returns the minimum heap ratio for the application master.
+ */
+ public double getAMMinHeapRatio() {
+return getMinHeapRatio(config);
+ }
+
+ /**
+ * Returns the minimum heap ratio for the given runnable.
+ */
+ public double getMinHeapRatio(String runnableName) {
+return getMinHeapRatio(runnableConfigs.containsKey(runnableName) ?
runnableConfigs.get(runnableName) : config);
--- End diff --
Agree. Get it wrong after some refactoring
---
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 #59: (TWILL-241) Added per runnable configuration and jvm...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/59 (TWILL-241) Added per runnable configuration and jvm options support This PR has two commits, one for adding per runnable configuration, the other for adding per runnable jvm options. You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-241-per-runnable-opts Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/59.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 #59 commit f1931deb128dda8b3c4da76486ab1b443318496e Author: Terence Yim <cht...@apache.org> Date: 2017-08-04T22:29:05Z (TWILL-241) Added support for per Runnable configuration commit 29a7999f45859996595287fb1c28b225a2564ed9 Author: Terence Yim <cht...@apache.org> Date: 2017-08-04T23:19:32Z (TWILL-241) Added support for per runnable JVM options - Also removed JvmOptionsCodec since JvmOptions only uses simple types --- 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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131211956
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/ProvisionTimeoutTestRun.java ---
@@ -79,12 +83,22 @@ public void initialize(EventHandlerContext context) {
@Override
public TimeoutAction launchTimeout(Iterable
timeoutEvents) {
+ for (TimeoutEvent event : timeoutEvents) {
+LOG.info("Requested {} containers for runnable {}, only got {}
after {} ms.",
--- End diff --
What's the point of this logging?
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131210382
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -291,6 +301,23 @@ public void run() {
// Since all the runnables are now stopped, it is okay to stop the
poller.
stopPoller.shutdownNow();
cleanupDir();
+if (eventHandler != null) {
+ if (finalStatus == null) {
+// if finalStatus is not set, the application must be stopped by a
SystemMessages#STOP_COMMAND
+eventHandler.killed(twillRuntimeSpec.getTwillAppName(),
twillRuntimeSpec.getTwillAppRunId());
+ }
+ switch (finalStatus) {
--- End diff --
This may have NPE. Should have a `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 pull request #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131209163
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -135,6 +135,15 @@
private final Map<String, Map<String, String>> environments;
private final TwillRuntimeSpecification twillRuntimeSpec;
+ /**
+ * Final status of this service when it stops.
+ */
+ public enum FinalStatus {
--- End diff --
Sounds more like `StopStatus` or `CompletionStatus`. Also it should be
`private` since it is only used in this class.
Also, please move it before all fields declaration instead of having it in
between fields.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131208560
--- Diff: twill-api/src/main/java/org/apache/twill/api/EventHandler.java ---
@@ -124,6 +124,74 @@ public void initialize(EventHandlerContext context) {
}
/**
+ * Invoked by the application when it starts.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ */
+ public void started(String twillAppName, RunId runId) {
+// No-op
+ }
+
+ /**
+ * Invoked by the application when new container is launched for {@link
TwillRunnable}.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ * @param runnableName name of the runnable to be run in the new
container
+ * @param instanceId the instance ID of the runnable instance to be run
in the new container
+ * @param containerId the ID of the newly launched container
+ */
+ public void containerLaunched(String twillAppName, RunId runId, String
runnableName,
+int instanceId, String containerId) {
+// No-op
+ }
+
+ /**
+ * Invoked by the application when a container is stopped.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ * @param runnableName name of the runnable in the stopped container
+ * @param instanceId the instance ID of the runnable instance run in the
stopped container
+ * @param containerId the ID of the stopped container
+ */
+ public void containerStopped(String twillAppName, RunId runId, String
runnableName,
+ int instanceId, String containerId) {
+// No-op
+ }
+
+ /**
+ * Invoked by the application when all containers complete.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ */
+ public void completed(String twillAppName, RunId runId) {
+// No-op
+ }
+
+ /**
+ * Invoked by the application when stop command is received to kill the
current application.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ */
+ public void killed(String twillAppName, RunId runId) {
+// No-op
+ }
+
+ /**
+ * Invoked by the application when the application is aborted because of
timeout.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ */
+ public void aborted(String twillAppName, RunId runId) {
+// No-op
+ }
+
+ /**
* Invoked by the application when shutting down.
*/
public void destroy() {
--- End diff --
continue, with the recheck time to some hardcode constant (say 60 seconds).
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r131210237
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
---
@@ -291,6 +301,23 @@ public void run() {
// Since all the runnables are now stopped, it is okay to stop the
poller.
stopPoller.shutdownNow();
cleanupDir();
+if (eventHandler != null) {
+ if (finalStatus == null) {
+// if finalStatus is not set, the application must be stopped by a
SystemMessages#STOP_COMMAND
+eventHandler.killed(twillRuntimeSpec.getTwillAppName(),
twillRuntimeSpec.getTwillAppRunId());
+ }
+ switch (finalStatus) {
+case COMPLETED:
+ eventHandler.completed(twillRuntimeSpec.getTwillAppName(),
twillRuntimeSpec.getTwillAppRunId());
+ break;
+case ABORTED:
+eventHandler.aborted(twillRuntimeSpec.getTwillAppName(),
twillRuntimeSpec.getTwillAppRunId());
+ break;
+default:
+ // should never reach here
+ LOG.error("Unsupported FinalStatus '%s'", finalStatus.name());
--- End diff --
That's not the right syntax. slf4j logger uses `{}`. Should be
`LOG.error("Unsupported status {}", finalStatus)`.
---
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 #55: (TWILL-237) Twill is using hdfs HAUtil api that is n...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/55#discussion_r131207866
--- Diff:
twill-yarn/src/main/java/org/apache/twill/filesystem/FileContextLocation.java
---
@@ -162,7 +163,8 @@ public URI toURI() {
// append "port" to the path URI, while the DistributedFileSystem
always use the cluster logical
// name, which doesn't allow having port in it.
URI uri = path.toUri();
-if (HAUtil.isLogicalUri(locationFactory.getConfiguration(), uri)) {
+
+if
(FileContextLocationUtil.useLogicalUri(locationFactory.getConfiguration(),
uri)) {
--- End diff --
That make sense.
---
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 #55: (TWILL-237) Twill is using hdfs HAUtil api that is n...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/55#discussion_r131208032
--- Diff:
twill-yarn/src/main/java/org/apache/twill/filesystem/FileContextLocationUtil.java
---
@@ -0,0 +1,77 @@
+/*
+ * 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.filesystem;
+
+import com.google.common.base.Throwables;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hdfs.HAUtil;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
+import java.net.URI;
+
+/**
+ * Utility class.
+ */
+class FileContextLocationUtil {
--- End diff --
Util class should be `final`.
---
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 #58: [TWILL-240] EventHandler Improvement
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/58#discussion_r130406951
--- Diff: twill-api/src/main/java/org/apache/twill/api/EventHandler.java ---
@@ -124,6 +124,74 @@ public void initialize(EventHandlerContext context) {
}
/**
+ * Invoked by the application when it starts.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ */
+ public void started(String twillAppName, RunId runId) {
+// No-op
+ }
+
+ /**
+ * Invoked by the application when new container is launched for {@link
TwillRunnable}.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ * @param runnableName name of the runnable to be run in the new
container
+ * @param instanceId the instance ID of the runnable instance to be run
in the new container
+ * @param containerId the ID of the newly launched container
+ */
+ public void containerLaunched(String twillAppName, RunId runId, String
runnableName,
+int instanceId, String containerId) {
+// No-op
+ }
+
+ /**
+ * Invoked by the application when a container is stopped.
+ *
+ * @param twillAppName name of the current application
+ * @param runId run ID of current application run
+ * @param runnableName name of the runnable in the stopped container
+ * @param instanceId the instance ID of the runnable instance run in the
stopped container
+ * @param containerId the ID of the stopped container
+ */
+ public void containerStopped(String twillAppName, RunId runId, String
runnableName,
--- End diff --
Should provide the exit code as well.
---
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 #56: (TWILL-238) Change TwillController restartInstances ...
Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/56#discussion_r126296669 --- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java --- @@ -17,15 +17,14 @@ */ package org.apache.twill.api; -import org.apache.twill.api.logging.LogEntry; -import org.apache.twill.api.logging.LogHandler; -import org.apache.twill.discovery.Discoverable; -import org.apache.twill.discovery.ServiceDiscovered; - import java.util.Map; import java.util.Set; import java.util.concurrent.Future; import javax.annotation.Nullable; +import org.apache.twill.api.logging.LogEntry; --- End diff -- Please don't rearrange the imports. You can use the ide profiles available in http://twill.apache.org/HowToContribute.html --- 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 #55: (TWILL-237) Twill is using hdfs HAUtil api that is n...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/55#discussion_r124058716
--- Diff:
twill-yarn/src/main/java/org/apache/twill/filesystem/FileContextLocation.java
---
@@ -162,7 +163,8 @@ public URI toURI() {
// append "port" to the path URI, while the DistributedFileSystem
always use the cluster logical
// name, which doesn't allow having port in it.
URI uri = path.toUri();
-if (HAUtil.isLogicalUri(locationFactory.getConfiguration(), uri)) {
+
+if
(FileContextLocationUtil.useLogicalUri(locationFactory.getConfiguration(),
uri)) {
--- End diff --
It seems like we actually don't need to check, but rather just always strip
off the port from the `URI`, since the `FileContext` or `FileSystem` in Hadoop
will always determine it internally based on the `Configuration`.
In fact that would make the returning `URI` more predictable.
---
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 #54: (TWILL-180) Reflects YARN application completion sta...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/54 (TWILL-180) Reflects YARN application completion status via TwillController You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-180 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/54.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 #54 commit 0e6fd06e4659049dfc335ded9b12a87654de2bda Author: Terence Yim <cht...@apache.org> Date: 2017-04-04T06:38:58Z (TWILL-180) Reflects YARN application completion status via TwillController --- 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 #53: (TWILL-230) Get resource report based on the caller ...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/53#discussion_r109553180
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillController.java ---
@@ -322,7 +314,46 @@ private boolean hasRun(YarnApplicationState state) {
@Override
public ResourceReport getResourceReport() {
-// in case the user calls this before starting, return null
+// Only has resource report if the app is running.
+if (state() != State.RUNNING) {
+ return null;
+}
+ResourceReportClient resourcesClient = getResourcesClient();
return (resourcesClient == null) ? null : resourcesClient.get();
}
+
+ /**
+ * Returns the {@link ResourceReportClient} for fetching resource report
from the AM.
+ * It first consults the RM for the tracking URL and get the resource
report from there.
+ */
+ @Nullable
+ private ResourceReportClient getResourcesClient() {
+YarnApplicationReport report = processController.getReport();
+List urls = new ArrayList<>(2);
--- End diff --
Because we store the tracking URL and the original tracking URL.
---
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 #52: (TWILL-176) Redirect all requests to tracker URL to ...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/52 (TWILL-176) Redirect all requests to tracker URL to /resources path - "/resources" is the only path supported for the tracker service for now You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-176 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/52.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 #52 commit b07fd47e3e78ad80097e26a944baceebefd0559c Author: Terence Yim <cht...@apache.org> Date: 2017-04-03T21:42:44Z (TWILL-176) Redirect all requests to tracker URL to /resources path - "/resources" is the only path supported for the tracker service for now --- 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 #51: (TWILL-179) Added support for custom ClassLoader for...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/51 (TWILL-179) Added support for custom ClassLoader for containers - Added method TwillPreparer.setClassLoader - Use system property "twill.custom.class.loader" to pass the class name of the custom ClassLoader You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-179 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/51.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 #51 commit 955156ee8b1cfb8e4072f77c9f83ee4ebce2e03b Author: Terence Yim <cht...@apache.org> Date: 2017-04-03T19:34:14Z (TWILL-179) Added support for custom ClassLoader for containers - Added method TwillPreparer.setClassLoader - Use system property "twill.custom.class.loader" to pass the class name of the custom ClassLoader --- 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 #48: (TWILL-189) Allows secure store update with different UGI
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/48 I tried to create a unit test using MiniKdc, but seems like I hit this bug https://issues.apache.org/jira/browse/HDFS-9213 --- 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 #49: (TWILL-229) Default to use logback_template.xml to configur...
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/49 For AM, currently it is not settable. For Runnable containers, user can add as resources / localize a `logback.xml` file and specify it in the classpath. --- 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 #46: (TWILL-227) Disabling caching of FileSystem instance...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/46#discussion_r108542463
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/yarn/YarnUtils.java ---
@@ -159,19 +159,19 @@ public static ApplicationId createApplicationId(long
timestamp, int id) {
return ImmutableList.of();
}
-FileSystem fileSystem = getFileSystem(locationFactory, config);
-
-if (fileSystem == null) {
- LOG.warn("Unexpected: LocationFactory is not backed by
FileContextLocationFactory");
- return ImmutableList.of();
-}
+try (FileSystem fileSystem = getFileSystem(locationFactory)) {
+ if (fileSystem == null) {
+LOG.warn("Unexpected: LocationFactory is not backed by
FileContextLocationFactory");
--- End diff --
Updated
---
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 #46: (TWILL-227) Disabling caching of FileSystem instance when g...
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/46 This replaces #45 due to accidentally deleting the branch. --- 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 #45: (TWILL-227) Disabling caching of FileSystem instance...
Github user chtyim closed the pull request at: https://github.com/apache/twill/pull/45 --- 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 #45: (TWILL-227) Disabling caching of FileSystem instance...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/45 (TWILL-227) Disabling caching of FileSystem instance when getting delegation token You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-227 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/45.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 #45 commit b2ce61f9023770aab9efd180b8e662212784d137 Author: Terence Yim <cht...@apache.org> Date: 2017-03-27T23:37:07Z (TWILL-227) Disabling caching of FileSystem instance when getting delegation token --- 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 #44: (TWILL-226) Remove the deprecated HDFSLocationFactor...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/44 (TWILL-226) Remove the deprecated HDFSLocationFactory You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-226 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/44.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 #44 commit dcb9fb9fcb853dbee65682fbc15b214521a89434 Author: Terence Yim <cht...@apache.org> Date: 2017-03-27T23:14:17Z (TWILL-226) Remove the deprecated HDFSLocationFactory --- 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 #42: (TWILL-171) Clone the HDFS delegation in HA mode.
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/42 (TWILL-171) Clone the HDFS delegation in HA mode. - This is for working around HDFS-9276 You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-171 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/42.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 #42 commit 2813b85f3cfa5ad0e37fd68cf06b1c4d577db76f Author: Terence Yim <cht...@apache.org> Date: 2017-03-27T17:51:26Z (TWILL-171) Clone the HDFS delegation in HA mode. - This is for working around HDFS-9276 --- 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 #40: (TWILL-122) Allow disabling log collection
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107773444
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map<String, Integer> getMaxRetries() {
return maxRetries;
}
+
+ /**
+ * Returns the ZK connection string for the Kafka used for log
collections,
+ * or {@code null} if log collection is disabled.
+ */
+ @Nullable
+ public String getKafkaZKConnect() {
+if (!isLogCollectionEnabled()) {
--- End diff --
Do you mean this javadoc and the `@Nullable` annotation not enough?
```java
/**
* Returns the ZK connection string for the Kafka used for log collections,
* or {@code null} if log collection is disabled.
*/
@Nullable
public String getKafkaZKConnect()
```
---
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 #40: (TWILL-122) Allow disabling log collection
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107717653
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map<String, Integer> getMaxRetries() {
return maxRetries;
}
+
+ /**
+ * Returns the ZK connection string for the Kafka used for log
collections,
+ * or {@code null} if log collection is disabled.
+ */
+ @Nullable
+ public String getKafkaZKConnect() {
+if (!isLogCollectionEnabled()) {
--- End diff --
How to avoid returning null? You mean by throwing exception so that the
caller has to call isLogCollectionEnabled before calling this method? I am not
a friend of method coupling like this.
---
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 #40: (TWILL-122) Allow disabling log collection
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107717203
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillController.java ---
@@ -77,7 +77,7 @@
*/
YarnTwillController(String appName, RunId runId, ZKClient zkClient,
final ApplicationMasterLiveNodeData amLiveNodeData,
final YarnAppClient yarnAppClient) {
-super(runId, zkClient, Collections.emptyList());
+super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() !=
null, Collections.emptyList());
--- End diff --
The contract of the getKafkaZKConnect won't change though, since it has to
return something (or throw exception). From the caller point of view, there is
almost no differences.
---
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 #40: (TWILL-122) Allow disabling log collection
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/40 @hsaputra Thanks for the review. Please see my replies. --- 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 #40: (TWILL-122) Allow disabling log collection
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107347828
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillController.java ---
@@ -77,7 +77,7 @@
*/
YarnTwillController(String appName, RunId runId, ZKClient zkClient,
final ApplicationMasterLiveNodeData amLiveNodeData,
final YarnAppClient yarnAppClient) {
-super(runId, zkClient, Collections.emptyList());
+super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() !=
null, Collections.emptyList());
--- End diff --
Or are you suggesting adding a new method `boolean isLogCollectionEnabled`
to the `ApplicationMasterLiveNode` class?
---
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 #40: (TWILL-122) Allow disabling log collection
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107347590
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map<String, Integer> getMaxRetries() {
return maxRetries;
}
+
+ /**
+ * Returns the ZK connection string for the Kafka used for log
collections,
+ * or {@code null} if log collection is disabled.
+ */
+ @Nullable
+ public String getKafkaZKConnect() {
+if (!isLogCollectionEnabled()) {
--- End diff --
What do mean by side effect check? The contract of this method is to return
`null` if log collection is disabled.
---
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 #40: (TWILL-122) Allow disabling log collection
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107344859
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java
---
@@ -71,21 +71,37 @@
private static final Logger LOG =
LoggerFactory.getLogger(AbstractTwillController.class);
private static final Gson GSON = new Gson();
+ private final String appName;
+ private final RunId runId;
private final Queue logHandlers;
private final KafkaClientService kafkaClient;
private ZKDiscoveryService discoveryServiceClient;
private Cancellable logCancellable;
- public AbstractTwillController(RunId runId, ZKClient zkClient,
Iterable logHandlers) {
+ public AbstractTwillController(String appName, RunId runId, ZKClient
zkClient, boolean logCollectionEnabled,
+ Iterable logHandlers) {
super(runId, zkClient);
+this.appName = appName;
+this.runId = runId;
this.logHandlers = new ConcurrentLinkedQueue<>();
-this.kafkaClient = new
ZKKafkaClientService(ZKClients.namespace(zkClient, "/" + runId.getId() +
"/kafka"));
-Iterables.addAll(this.logHandlers, logHandlers);
+
+// When addressing TWILL-147, need to check if the given ZKClient is
+// actually used by the Kafka used for log collection
+if (logCollectionEnabled) {
+ this.kafkaClient = new
ZKKafkaClientService(ZKClients.namespace(zkClient, "/" + runId.getId() +
"/kafka"));
+ Iterables.addAll(this.logHandlers, logHandlers);
+} else {
+ this.kafkaClient = null;
+ if (!Iterables.isEmpty(logHandlers)) {
+LOG.warn("Log collection is disabled for application {} with runId
{}. " +
+ "Adding log handler won't get any logs.", appName,
runId);
+ }
+}
}
@Override
protected synchronized void doStartUp() {
-if (!logHandlers.isEmpty()) {
+if (kafkaClient != null && !logHandlers.isEmpty()) {
--- End diff --
`logCollectionEnabled` is not a field. I used `kafkaClient == null` to
indicate disabling of log collection.
Also, the `logHandlers` collection can be empty during startup of the
controller, even log collection is enabled, hence the check.
---
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 #40: (TWILL-122) Allow disabling log collection
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/40 (TWILL-122) Allow disabling log collection - Introduced a new configuration twill.log.collection.enabled for turning off log collection - Refactor YarnTwillController and related class hierarchy to not starting Kafka client when log collection is disabled - Added Kafka zk connection string information in AM live node data - Refactor KafkaAppender and ServiceMain configureLogger - Log to StatusManager instead of Logger to avoid recursive logging - Instead of resetting logback configuration, directly instantiate and add the Kafka log appender to the logging context. - Refactor ServiceMain, ApplicationMasterMain and TwillContainerMain to simplify ZK Connection string construction You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-122 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/40.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 #40 commit 617280ee72b2e7fc7a3cc473d6dfe35576cf5e57 Author: Terence Yim <cht...@apache.org> Date: 2017-03-20T21:42:46Z (TWILL-122) Allow disabling log collection - Introduced a new configuration twill.log.collection.enabled for turning off log collection - Refactor YarnTwillController and related class hierarchy to not starting Kafka client when log collection is disabled - Added Kafka zk connection string information in AM live node data - Refactor KafkaAppender and ServiceMain configureLogger - Log to StatusManager instead of Logger to avoid recursive logging - Instead of resetting logback configuration, directly instantiate and add the Kafka log appender to the logging context. - Refactor ServiceMain, ApplicationMasterMain and TwillContainerMain to simplify ZK Connection string construction --- 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 #39: (TWILL-225) Refactor TwillPreprer to allow configura...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/39#discussion_r107264780
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -455,21 +475,40 @@ private void saveLogLevels(String runnableName,
Map<String, LogEntry.Level> logL
this.logLevels.put(runnableName, newLevels);
}
+ /**
+ * Creates an {@link Credentials} by copying the {@link Credentials} of
the current user.
+ */
private Credentials createCredentials() {
Credentials credentials = new Credentials();
try {
credentials.addAll(UserGroupInformation.getCurrentUser().getCredentials());
+} catch (IOException e) {
--- End diff --
I don't think so. Even if this call fail, if the caller is able to acquire
required credentials in his own way and add them via the `addSecureStore`
method, then the app can still be submitted and executed correctly.
---
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 #39: (TWILL-225) Refactor TwillPreprer to allow configura...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/39#discussion_r10700
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -155,25 +153,16 @@ public String apply(Class cls) {
private ClassAcceptor classAcceptor;
private final Map<String, Integer> maxRetries = Maps.newHashMap();
- YarnTwillPreparer(YarnConfiguration yarnConfig, TwillSpecification
twillSpec, RunId runId,
-YarnAppClient yarnAppClient, String zkConnectString,
Location appLocation, Set twillClassPaths,
+ YarnTwillPreparer(Configuration config, TwillSpecification twillSpec,
RunId runId,
--- End diff --
It's more generic. We actually not using any specific method from
`YarnConfiguration` in this class.
---
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 #36: (TWILL-90) Add new configuration options to set AM memory s...
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/36 Thanks for the review. --- 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 #36: (TWILL-90) Add new configuration options to set AM m...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/36 (TWILL-90) Add new configuration options to set AM memory size You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/TWILL-90 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/36.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 #36 commit c085671661fce24c120cd9677ca07b9914ef0a0a Author: Terence Yim <cht...@apache.org> Date: 2017-03-16T21:27:25Z (TWILL-90) Add new configuration options to set AM memory size --- 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 #35: (TWILL-207) Only use list of class names as the cache name
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/35 As said in the description, you will see the important changes when ignoring white space changes --- 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 #35: (TWILL-207) Only use list of class names as the cache name
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/35 Force push a change to trigger travis build 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 issue #34: (TWILL-186) Fix NPE and container size mismatch
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/34 Seems like the github sync is lagging. I merged this change about 6 hours ago --- 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 #34: (TWILL-186) Fix NPE and container size mismatch
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/34 I squashed them after getting LGTM to prepare for the merge --- 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 #35: (TWILL-207) Only use list of class names as the cach...
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/35 (TWILL-207) Only use list of class names as the cache name - Also some indentation changes. You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/twill-207 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/35.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 #35 commit 542850875e0aecbe34d16bca962186b3d32bfb19 Author: Terence Yim <cht...@apache.org> Date: 2017-03-01T02:03:45Z (TWILL-207) Only use list of class names as the cache name - Also some indentation changes. --- 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 #34: (TWILL-186) Fix NPE and container size mismatch
Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/34#discussion_r103596214 --- Diff: twill-yarn/src/main/java/org/apache/twill/internal/yarn/AbstractYarnAMClient.java --- @@ -50,12 +51,11 @@ private static final Logger LOG = LoggerFactory.getLogger(AbstractYarnAMClient.class); // Map from a unique ID to inflight requests - private final Multimap<String, T> containerRequests; - - // List of requests pending to send through allocate call - private final List requests; + private final Multimap<String, T> inflightRequests; + // Map from a unique ID to pending requests. It is for recording --- End diff -- Oh. It is for recording the container requests that has yet to be sent to RM. Will update the comment. --- 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 #34: Feature/twill 186
GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/34 Feature/twill 186 You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/twill-186 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/34.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 #34 commit b64c90b618f065ca01a8c5d36df2535b1344564e Author: Terence Yim <cht...@apache.org> Date: 2017-02-28T05:22:32Z (TWILL-186) Code cleanup for ApplicationMasterService and related classes - Get rid of the inner loop in the doRun method - The inner loop can block the heartbeat thread for too long if there are a lot of runnable instances to stop - Remove unnecessary throwables.propagate - Remove unnecessary intermediate method - Better logging - Request multiple instances in the same request - Refactory/simiply placement policy related code - Expose container instanceId instead of parsing it from runId commit fcfa5becd61ddc2513f29a2be700b3be166d4b0b Author: Terence Yim <cht...@apache.org> Date: 2017-02-28T22:43:37Z (TWILL-186) Guard against YARN returning mismatch container size case. - Also make sure we don't remove container request without adding it 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] twill pull request #33: TWILL-216 Make ratio between total memory and on-hea...
Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/33#discussion_r101789920 --- Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java --- @@ -38,7 +38,6 @@ import com.google.common.util.concurrent.SettableFuture; import com.google.gson.Gson; import com.google.gson.GsonBuilder; --- End diff -- Please don't update import spaces. If you use IntelliJ or eclipse, you can download the style from the twill site. --- 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 #33: TWILL-216 Make ratio between total memory and on-hea...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/33#discussion_r101699381
--- Diff: twill-api/src/main/java/org/apache/twill/api/Configs.java ---
@@ -75,6 +82,7 @@ private Keys() {
*/
public static final int JAVA_RESERVED_MEMORY_MB = 200;
+public static final double HEAP_RESERVED_MIN_RATIO_DEFAULT =
Constants.HEAP_MIN_RATIO;
--- End diff --
You can remove the `Constants.HEAP_MIN_RATIO` and just define the default
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
