Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.11.0
    • Component/s: core
    • Labels:
      None

      Description

      Currently transporting logs to Kafka is mandatory. It should be optionally so that application can have its own way of log collection.

        Issue Links

          Activity

          Hide
          chtyim Terence Yim added a comment -

          With TWILL-24, a quick workaround is to set the log level to OFF. Better yet, if the log level is OFF, we don't need to start the Embedded Kafka server in the AM at all and the client shouldn't even attempt to subscribe to Kafka.

          Show
          chtyim Terence Yim added a comment - With TWILL-24 , a quick workaround is to set the log level to OFF. Better yet, if the log level is OFF, we don't need to start the Embedded Kafka server in the AM at all and the client shouldn't even attempt to subscribe to Kafka.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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 <chtyim@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

          Show
          githubbot ASF GitHub Bot added a comment - 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 <chtyim@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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/40#discussion_r107344378

          — 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<LogHandler> logHandlers;
          private final KafkaClientService kafkaClient;
          private ZKDiscoveryService discoveryServiceClient;
          private Cancellable logCancellable;

          • public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) {
            + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled,
            + Iterable<LogHandler> 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 –

          Should this just check `if (logCollectionEnabled)` instead since technically both conditions will be met when logCollectionEnabled is true

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on a diff in the pull request: https://github.com/apache/twill/pull/40#discussion_r107344378 — 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<LogHandler> logHandlers; private final KafkaClientService kafkaClient; private ZKDiscoveryService discoveryServiceClient; private Cancellable logCancellable; public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) { + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled, + Iterable<LogHandler> 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 – Should this just check `if (logCollectionEnabled)` instead since technically both conditions will be met when logCollectionEnabled is true
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<LogHandler> logHandlers;
          private final KafkaClientService kafkaClient;
          private ZKDiscoveryService discoveryServiceClient;
          private Cancellable logCancellable;

          • public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) {
            + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled,
            + Iterable<LogHandler> 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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<LogHandler> logHandlers; private final KafkaClientService kafkaClient; private ZKDiscoveryService discoveryServiceClient; private Cancellable logCancellable; public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) { + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled, + Iterable<LogHandler> 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/40#discussion_r107346167

          — 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<LogHandler> logHandlers;
          private final KafkaClientService kafkaClient;
          private ZKDiscoveryService discoveryServiceClient;
          private Cancellable logCancellable;

          • public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) {
            + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled,
            + Iterable<LogHandler> 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 –

          But `this.logHandlers` will always be empty if `if (kafkaClient == null)` is true? So could we just rely on check on kafkaClient as null ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on a diff in the pull request: https://github.com/apache/twill/pull/40#discussion_r107346167 — 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<LogHandler> logHandlers; private final KafkaClientService kafkaClient; private ZKDiscoveryService discoveryServiceClient; private Cancellable logCancellable; public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) { + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled, + Iterable<LogHandler> 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 – But `this.logHandlers` will always be empty if `if (kafkaClient == null)` is true? So could we just rely on check on kafkaClient as null ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/40#discussion_r107346435

          — 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<LogHandler> logHandlers;
          private final KafkaClientService kafkaClient;
          private ZKDiscoveryService discoveryServiceClient;
          private Cancellable logCancellable;

          • public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) {
            + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled,
            + Iterable<LogHandler> 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 mean have the logic as

          ```java
          if (kafkaClient == null)

          { return; }

          if (!logHandlers.isEmpty())

          { kafkaClient.startAndWait(); logCancellable = kafkaClient.getConsumer()..... }

          ```

          ??
          Doesn't look simpler than the existing code to me.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/40#discussion_r107346435 — 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<LogHandler> logHandlers; private final KafkaClientService kafkaClient; private ZKDiscoveryService discoveryServiceClient; private Cancellable logCancellable; public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) { + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled, + Iterable<LogHandler> 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 mean have the logic as ```java if (kafkaClient == null) { return; } if (!logHandlers.isEmpty()) { kafkaClient.startAndWait(); logCancellable = kafkaClient.getConsumer()..... } ``` ?? Doesn't look simpler than the existing code to me.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/40#discussion_r107347185

          — 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.<LogHandler>emptyList());
            + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>emptyList());
              • End diff –

          Can the constructor of `YarnTwillController` take new input of `logCollectionEnabled` ? I am seeing different ways to check whether to disable log or not

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on a diff in the pull request: https://github.com/apache/twill/pull/40#discussion_r107347185 — 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.<LogHandler>emptyList()); + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>emptyList()); End diff – Can the constructor of `YarnTwillController` take new input of `logCollectionEnabled` ? I am seeing different ways to check whether to disable log or not
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on a diff in the pull request:

          https://github.com/apache/twill/pull/40#discussion_r107347379

          — 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 –

          I am not too keen about having this "side effect" check for log collection enabled here to return KafkaZKConnect.
          Caller can always check `isLogCollectionEnabled()` to figure out whether log collection is disabled, right?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on a diff in the pull request: https://github.com/apache/twill/pull/40#discussion_r107347379 — 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 – I am not too keen about having this "side effect" check for log collection enabled here to return KafkaZKConnect. Caller can always check `isLogCollectionEnabled()` to figure out whether log collection is disabled, right?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/40#discussion_r107347432

          — 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.<LogHandler>emptyList());
            + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>emptyList());
              • End diff –

          ? I don't understand, can you give me an example? The current logic is, from an existing app discovered via ZK, if the AM live node data doesn't have the KafkaZKConnect string, that means the app was launched with log collection disabled, hence the logic here.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/40#discussion_r107347432 — 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.<LogHandler>emptyList()); + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>emptyList()); End diff – ? I don't understand, can you give me an example? The current logic is, from an existing app discovered via ZK, if the AM live node data doesn't have the KafkaZKConnect string, that means the app was launched with log collection disabled, hence the logic here.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on the issue:

          https://github.com/apache/twill/pull/40

          I did a pass and add some comments. Mostly kind of concern of multiple ways to check whether log collection is disabled or not.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on the issue: https://github.com/apache/twill/pull/40 I did a pass and add some comments. Mostly kind of concern of multiple ways to check whether log collection is disabled or not.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.<LogHandler>emptyList());
            + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>emptyList());
              • End diff –

          Or are you suggesting adding a new method `boolean isLogCollectionEnabled` to the `ApplicationMasterLiveNode` class?

          Show
          githubbot ASF GitHub Bot added a comment - 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.<LogHandler>emptyList()); + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>emptyList()); End diff – Or are you suggesting adding a new method `boolean isLogCollectionEnabled` to the `ApplicationMasterLiveNode` class?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/40

          @hsaputra Thanks for the review. Please see my replies.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/40 @hsaputra Thanks for the review. Please see my replies.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<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 –

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<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 – 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.<LogHandler>emptyList());
            + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>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

          Show
          githubbot ASF GitHub Bot added a comment - 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.<LogHandler>emptyList()); + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<LogHandler> logHandlers;
          private final KafkaClientService kafkaClient;
          private ZKDiscoveryService discoveryServiceClient;
          private Cancellable logCancellable;

          • public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) {
            + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled,
            + Iterable<LogHandler> 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

          Show
          githubbot ASF GitHub Bot added a comment - 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<LogHandler> logHandlers; private final KafkaClientService kafkaClient; private ZKDiscoveryService discoveryServiceClient; private Cancellable logCancellable; public AbstractTwillController(RunId runId, ZKClient zkClient, Iterable<LogHandler> logHandlers) { + public AbstractTwillController(String appName, RunId runId, ZKClient zkClient, boolean logCollectionEnabled, + Iterable<LogHandler> 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.<LogHandler>emptyList());
            + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.<LogHandler>emptyList()); + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<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 –

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<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 – 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<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 –

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<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 – 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<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 –

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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<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 – 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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()
          ```

          Show
          githubbot ASF GitHub Bot added a comment - 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() ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          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<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 –

          Ah sorry, that will work, I was looking at snippet of change. Yeah, should work

          Show
          githubbot ASF GitHub Bot added a comment - 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<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 – Ah sorry, that will work, I was looking at snippet of change. Yeah, should work
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.<LogHandler>emptyList());
            + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.<LogHandler>emptyList()); + super(appName, runId, zkClient, amLiveNodeData.getKafkaZKConnect() != null, Collections.<LogHandler>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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/40

          Thank @hsaputra for the review. Merging the change.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/40 Thank @hsaputra for the review. Merging the change.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/twill/pull/40

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/40

            People

            • Assignee:
              chtyim Terence Yim
              Reporter:
              chtyim Terence Yim
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development