[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15938499#comment-15938499
]
ASF GitHub Bot commented on TWILL-122:
--
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107688594
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
Thats actually what I meant. I am wondering if we could avoid returning
null here because the extra check of if for `isLogCollectionEnabled`. The idea
is the code that need to get KafkaZkconnect can get it when it is needed and
dont call it when there is no need to do Kafka connect bc the caller can call
`isLogCollectionEnabled` to determine whether to get the connect info or not.
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] twill pull request #40: (TWILL-122) Allow disabling log collection
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107688594
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
Thats actually what I meant. I am wondering if we could avoid returning
null here because the extra check of if for `isLogCollectionEnabled`. The idea
is the code that need to get KafkaZkconnect can get it when it is needed and
dont call it when there is no need to do Kafka connect bc the caller can call
`isLogCollectionEnabled` to determine whether to get the connect info or not.
---
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 hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107689017
--- 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 --
Yeah, thinking about having
`ApplicationMasterLiveNode.isLogCollectionEnabled` to make the contract more
clear. Otherwise later we will forget why checking for getKafkaZKConnect !=
null 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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15938501#comment-15938501
]
ASF GitHub Bot commented on TWILL-122:
--
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107689017
--- 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 --
Yeah, thinking about having
`ApplicationMasterLiveNode.isLogCollectionEnabled` to make the contract more
clear. Otherwise later we will forget why checking for getKafkaZKConnect !=
null here
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] twill pull request #40: (TWILL-122) Allow disabling log collection
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107690293
--- 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 --
You are right here, there is a chance that even log is enabled but caller
can pass empty `logHandlers`.
+1 for current 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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15938510#comment-15938510
]
ASF GitHub Bot commented on TWILL-122:
--
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107690293
--- 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 --
You are right here, there is a chance that even log is enabled but caller
can pass empty `logHandlers`.
+1 for current check
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15938711#comment-15938711
]
ASF GitHub Bot commented on TWILL-122:
--
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.
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[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 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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15938713#comment-15938713
]
ASF GitHub Bot commented on TWILL-122:
--
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 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.
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] twill pull request #40: (TWILL-122) Allow disabling log collection
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107739693
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
The call to `getZkConnectStr` and `getTwillAppName` and `getTwillAppRunId`
will never return null based on the contract (no @Nullable) , hence call to
`getKafkaZKConnect` will never be null.
I was saying that either way, someone need to check whether call to
getKafkaZKConnect will return null if log collection is disabled, so why not
ask to isLogCollectionEnabled to check 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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15938902#comment-15938902
]
ASF GitHub Bot commented on TWILL-122:
--
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107739693
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
The call to `getZkConnectStr` and `getTwillAppName` and `getTwillAppRunId`
will never return null based on the contract (no @Nullable) , hence call to
`getKafkaZKConnect` will never be null.
I was saying that either way, someone need to check whether call to
getKafkaZKConnect will return null if log collection is disabled, so why not
ask to isLogCollectionEnabled to check first.
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
question regarding BundleRunnable behavior in case of failure
Hello there, I am using BundleRunnable and so far my experience was that in case of failure of my runnable container process never exits. And my impression it pretty much all the time stuck in executing Shutdown hooks (not even my application specific) Here is ThreadDump for that thread: "TwillContainerService" #32 prio=5 os_prio=0 tid=0x7f1590297800 nid=0x4de2 in Object.wait() [0x7f15805d9000] java.lang.Thread.State: WAITING (on object monitor) at java.lang.Object.wait(Native Method) at java.lang.Thread.join(Thread.java:1249) - locked <0xff9c41d0> (a org.apache.twill.internal.ServiceMain$1) at java.lang.Thread.join(Thread.java:1323) at java.lang.ApplicationShutdownHooks.runHooks(ApplicationShutdownHooks.java:106) at java.lang.ApplicationShutdownHooks$1.run(ApplicationShutdownHooks.java:46) at java.lang.Shutdown.runHooks(Shutdown.java:123) at java.lang.Shutdown.sequence(Shutdown.java:167) at java.lang.Shutdown.exit(Shutdown.java:212) - locked <0xff6363e8> (a java.lang.Class for java.lang.Shutdown) at java.lang.Runtime.exit(Runtime.java:109) at java.lang.System.exit(System.java:971) at org.apache.twill.ext.BundledJarRunnable.run(BundledJarRunnable.java:59) at org.apache.twill.internal.container.TwillContainerService.doRun(TwillContainerService.java:130) at org.apache.twill.internal.AbstractTwillService.run(AbstractTwillService.java:181) at twill.com.google.common.util.concurrent.AbstractExecutionThreadService$1$1.run(AbstractExecutionThreadService.java:52) at java.lang.Thread.run(Thread.java:745) Just wonder if anybody else experienced similar. Thanks
[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_r107767933
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
This is because there is no way to enforce someone have to call
`isLogCollectionEnabled` first before calling the `getKafkaZKConnect` method,
meaning the `getKafkaZKConnect` method needs to handle the case when log
collection disabled anyway (either by returning `null` or returning `Optional`
or throwing exception). From the caller perspective, it seems more straight
forward to call the method and validate against the contract then calling two
methods in the right order.
However, having `getKafkaZKConnect` returning `null` (I prefer `Optional`,
but since we are on Java 7, and we are on the road on reducing reliance on
guava, so that's not really an option), can make cases like this simpler in
future when adding support for TWILL-147. E.g.
```java
YarnTwillController(@Nullable String kafkaZKConnect, ...) {
this.kafkaClient = createKafkaClient(kafkaZKConnect);
}
@Nullable
private KafkaClient createKafkaClient(@Nnullable String kafkaZKConnect) {
if (kafkaZKConnect == null) {
return null;
}
return new KafkaClient();
}
...
// For app launch with the current twill runner
new YarnTwillController(spec.getKafkaZKConnect(), ...);
// For running app recovered from ZK.
new YarnTwillController(amLiveNode.getKafkaZKConnect(), ...);
```
Without the `nullable` support, there would be more overloaded constructors
as well as code branches to achieve the same.
---
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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15939116#comment-15939116
]
ASF GitHub Bot commented on TWILL-122:
--
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107767933
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
This is because there is no way to enforce someone have to call
`isLogCollectionEnabled` first before calling the `getKafkaZKConnect` method,
meaning the `getKafkaZKConnect` method needs to handle the case when log
collection disabled anyway (either by returning `null` or returning `Optional`
or throwing exception). From the caller perspective, it seems more straight
forward to call the method and validate against the contract then calling two
methods in the right order.
However, having `getKafkaZKConnect` returning `null` (I prefer `Optional`,
but since we are on Java 7, and we are on the road on reducing reliance on
guava, so that's not really an option), can make cases like this simpler in
future when adding support for TWILL-147. E.g.
```java
YarnTwillController(@Nullable String kafkaZKConnect, ...) {
this.kafkaClient = createKafkaClient(kafkaZKConnect);
}
@Nullable
private KafkaClient createKafkaClient(@Nnullable String kafkaZKConnect) {
if (kafkaZKConnect == null) {
return null;
}
return new KafkaClient();
}
...
// For app launch with the current twill runner
new YarnTwillController(spec.getKafkaZKConnect(), ...);
// For running app recovered from ZK.
new YarnTwillController(amLiveNode.getKafkaZKConnect(), ...);
```
Without the `nullable` support, there would be more overloaded constructors
as well as code branches to achieve the same.
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] twill pull request #40: (TWILL-122) Allow disabling log collection
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107771740
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
Same thing of no way that caller could have guessed without looking at the
code that there is a check of isLogCollectionEnabled inside getKafkaZKConnect
that have inferred meaning when it is returning null.
The compromise I think at least added JavaDoc header for getKafkaZKConnect
to explain why it could 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 issue #40: (TWILL-122) Allow disabling log collection
Github user hsaputra commented on the issue: https://github.com/apache/twill/pull/40 +1 for correctness I just added suggestion of style of return null vs declarative check by caller to determine if logging 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. ---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15939150#comment-15939150
]
ASF GitHub Bot commented on TWILL-122:
--
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107771740
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
Same thing of no way that caller could have guessed without looking at the
code that there is a check of isLogCollectionEnabled inside getKafkaZKConnect
that have inferred meaning when it is returning null.
The compromise I think at least added JavaDoc header for getKafkaZKConnect
to explain why it could return null.
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[ https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15939152#comment-15939152 ] ASF GitHub Bot commented on TWILL-122: -- Github user hsaputra commented on the issue: https://github.com/apache/twill/pull/40 +1 for correctness I just added suggestion of style of return null vs declarative check by caller to determine if logging collection is disabled. > Allow disabling the log transport > - > > Key: TWILL-122 > URL: https://issues.apache.org/jira/browse/TWILL-122 > Project: Apache Twill > Issue Type: Improvement > Components: core >Reporter: Terence Yim >Assignee: Terence Yim > Fix For: 0.11.0 > > > Currently transporting logs to Kafka is mandatory. It should be optionally so > that application can have its own way of log collection. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[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 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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15939190#comment-15939190
]
ASF GitHub Bot commented on TWILL-122:
--
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 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()
```
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] twill pull request #40: (TWILL-122) Allow disabling log collection
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107781058
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
Ah sorry, that will work, I was looking at snippet of change. Yeah, should
work
---
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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15939213#comment-15939213
]
ASF GitHub Bot commented on TWILL-122:
--
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107781058
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr() {
public Map 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 --
Ah sorry, that will work, I was looking at snippet of change. Yeah, should
work
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] twill pull request #40: (TWILL-122) Allow disabling log collection
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107788346
--- 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 --
At this point is more about style than correctness. I personally prefer to
not make the logic of determining to enable or disable logging in
YarnTwillController but rather on object that have current state of the union.
---
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.
---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[
https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15939256#comment-15939256
]
ASF GitHub Bot commented on TWILL-122:
--
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107788346
--- 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 --
At this point is more about style than correctness. I personally prefer to
not make the logic of determining to enable or disable logging in
YarnTwillController but rather on object that have current state of the union.
> Allow disabling the log transport
> -
>
> Key: TWILL-122
> URL: https://issues.apache.org/jira/browse/TWILL-122
> Project: Apache Twill
> Issue Type: Improvement
> Components: core
>Reporter: Terence Yim
>Assignee: Terence Yim
> Fix For: 0.11.0
>
>
> Currently transporting logs to Kafka is mandatory. It should be optionally so
> that application can have its own way of log collection.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[GitHub] twill issue #40: (TWILL-122) Allow disabling log collection
Github user chtyim commented on the issue: https://github.com/apache/twill/pull/40 Thank @hsaputra for the review. Merging the change. --- 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. ---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[ https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15939308#comment-15939308 ] ASF GitHub Bot commented on TWILL-122: -- Github user chtyim commented on the issue: https://github.com/apache/twill/pull/40 Thank @hsaputra for the review. Merging the change. > Allow disabling the log transport > - > > Key: TWILL-122 > URL: https://issues.apache.org/jira/browse/TWILL-122 > Project: Apache Twill > Issue Type: Improvement > Components: core >Reporter: Terence Yim >Assignee: Terence Yim > Fix For: 0.11.0 > > > Currently transporting logs to Kafka is mandatory. It should be optionally so > that application can have its own way of log collection. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[GitHub] twill pull request #40: (TWILL-122) Allow disabling log collection
Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/40 --- 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. ---
[jira] [Commented] (TWILL-122) Allow disabling the log transport
[ https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15939310#comment-15939310 ] ASF GitHub Bot commented on TWILL-122: -- Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/40 > Allow disabling the log transport > - > > Key: TWILL-122 > URL: https://issues.apache.org/jira/browse/TWILL-122 > Project: Apache Twill > Issue Type: Improvement > Components: core >Reporter: Terence Yim >Assignee: Terence Yim > Fix For: 0.11.0 > > > Currently transporting logs to Kafka is mandatory. It should be optionally so > that application can have its own way of log collection. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Resolved] (TWILL-122) Allow disabling the log transport
[ https://issues.apache.org/jira/browse/TWILL-122?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Terence Yim resolved TWILL-122. --- Resolution: Fixed > Allow disabling the log transport > - > > Key: TWILL-122 > URL: https://issues.apache.org/jira/browse/TWILL-122 > Project: Apache Twill > Issue Type: Improvement > Components: core >Reporter: Terence Yim >Assignee: Terence Yim > Fix For: 0.11.0 > > > Currently transporting logs to Kafka is mandatory. It should be optionally so > that application can have its own way of log collection. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
