Uploaded image for project: 'Apache Twill'
  1. Apache Twill
  2. TWILL-177

ZKDiscoveryService should have a way to remove its connection watcher

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.0
    • Component/s: discovery
    • Labels:
      None

      Description

      The ZKDiscoveryService adds a connection watcher in its constructor, but has no way to remove it. As a result, if somebody is creating new instances, the number of watches will slowly grow.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user chtyim commented on the issue:

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

          Squashed commits.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/8 Squashed commits.
          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/8#discussion_r78632801

          — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/YarnTestSuite.java —
          @@ -17,8 +17,15 @@
          */
          package org.apache.twill.yarn;

          +import org.junit.After;
          +import org.junit.Before;
          +import org.junit.Rule;
          +import org.junit.rules.ExternalResource;
          +import org.junit.rules.TestName;
          — End diff –

          fixed

          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/8#discussion_r78632801 — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/YarnTestSuite.java — @@ -17,8 +17,15 @@ */ package org.apache.twill.yarn; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.rules.ExternalResource; +import org.junit.rules.TestName; — End diff – fixed
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user albertshau commented on the issue:

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

          one minor comment, otherwise lgtm

          Show
          githubbot ASF GitHub Bot added a comment - Github user albertshau commented on the issue: https://github.com/apache/twill/pull/8 one minor comment, otherwise lgtm
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/8#discussion_r78623437

          — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/YarnTestSuite.java —
          @@ -17,8 +17,15 @@
          */
          package org.apache.twill.yarn;

          +import org.junit.After;
          +import org.junit.Before;
          +import org.junit.Rule;
          +import org.junit.rules.ExternalResource;
          +import org.junit.rules.TestName;
          — End diff –

          unused imports?

          Show
          githubbot ASF GitHub Bot added a comment - Github user albertshau commented on a diff in the pull request: https://github.com/apache/twill/pull/8#discussion_r78623437 — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/YarnTestSuite.java — @@ -17,8 +17,15 @@ */ package org.apache.twill.yarn; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.rules.ExternalResource; +import org.junit.rules.TestName; — End diff – unused imports?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

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

          Rebased on master and resolved conflicts.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/8 Rebased on master and resolved conflicts.
          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/8#discussion_r78132097

          — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/BaseYarnTest.java —
          @@ -66,6 +69,18 @@ protected void after() {
          }
          };

          + @Rule
          + public final TestName testName = new TestName();
          +
          + @Before
          + public void beforeTest() {
          + LOG.info("Before test {}", testName.getMethodName());
          — End diff –

          No, it won't

          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/8#discussion_r78132097 — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/BaseYarnTest.java — @@ -66,6 +69,18 @@ protected void after() { } }; + @Rule + public final TestName testName = new TestName(); + + @Before + public void beforeTest() { + LOG.info("Before test {}", testName.getMethodName()); — End diff – No, it won't
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/8#discussion_r78106987

          — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/BaseYarnTest.java —
          @@ -66,6 +69,18 @@ protected void after() {
          }
          };

          + @Rule
          + public final TestName testName = new TestName();
          +
          + @Before
          + public void beforeTest() {
          + LOG.info("Before test {}", testName.getMethodName());
          — End diff –

          Wouldn't JUnit already log the test method before it is executed?

          Show
          githubbot ASF GitHub Bot added a comment - Github user anwar6953 commented on a diff in the pull request: https://github.com/apache/twill/pull/8#discussion_r78106987 — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/BaseYarnTest.java — @@ -66,6 +69,18 @@ protected void after() { } }; + @Rule + public final TestName testName = new TestName(); + + @Before + public void beforeTest() { + LOG.info("Before test {}", testName.getMethodName()); — End diff – Wouldn't JUnit already log the test method before it is executed?
          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/8#discussion_r76718497

          — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/BaseYarnTest.java —
          @@ -66,6 +69,18 @@ protected void after() {
          }
          };

          + @Rule
          + public final TestName testName = new TestName();
          +
          + @Before
          + public void beforeTest() {
          + LOG.info("Before test {}", testName.getMethodName());
          — End diff –

          Why? It helps identify which parts in the log is for which test.

          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/8#discussion_r76718497 — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/BaseYarnTest.java — @@ -66,6 +69,18 @@ protected void after() { } }; + @Rule + public final TestName testName = new TestName(); + + @Before + public void beforeTest() { + LOG.info("Before test {}", testName.getMethodName()); — End diff – Why? It helps identify which parts in the log is for which test.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/twill/pull/8#discussion_r76680691

          — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/BaseYarnTest.java —
          @@ -66,6 +69,18 @@ protected void after() {
          }
          };

          + @Rule
          + public final TestName testName = new TestName();
          +
          + @Before
          + public void beforeTest() {
          + LOG.info("Before test {}", testName.getMethodName());
          — End diff –

          this was just for investigation? Can be removed now?

          Show
          githubbot ASF GitHub Bot added a comment - Github user anwar6953 commented on a diff in the pull request: https://github.com/apache/twill/pull/8#discussion_r76680691 — Diff: twill-yarn/src/test/java/org/apache/twill/yarn/BaseYarnTest.java — @@ -66,6 +69,18 @@ protected void after() { } }; + @Rule + public final TestName testName = new TestName(); + + @Before + public void beforeTest() { + LOG.info("Before test {}", testName.getMethodName()); — End diff – this was just for investigation? Can be removed now?
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user chtyim opened a pull request:

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

          (TWILL-177) Make ZKDiscoveryService AutoCloseable

          • Release ZK watches when closed

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/chtyim/twill feature/twill-177

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/twill/pull/8.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 #8


          commit 98bcb2d05b02b45bdf0937fb936befb558260b3b
          Author: Terence Yim <chtyim@apache.org>
          Date: 2016-08-26T01:41:15Z

          (TWILL-177) Make ZKDiscoveryService AutoCloseable

          • Release ZK watches when closed

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/8 ( TWILL-177 ) Make ZKDiscoveryService AutoCloseable Release ZK watches when closed You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/twill-177 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/8.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 #8 commit 98bcb2d05b02b45bdf0937fb936befb558260b3b Author: Terence Yim <chtyim@apache.org> Date: 2016-08-26T01:41:15Z ( TWILL-177 ) Make ZKDiscoveryService AutoCloseable Release ZK watches when closed
          Hide
          ashau Albert Shau added a comment -

          AbstractTwillController will new a ZKDiscoveryService every time its created. This is what's returned when a user calls TwillPreparer.start(), so any process that starts twill applications will slowly leak memory.

          Show
          ashau Albert Shau added a comment - AbstractTwillController will new a ZKDiscoveryService every time its created. This is what's returned when a user calls TwillPreparer.start(), so any process that starts twill applications will slowly leak memory.

            People

            • Assignee:
              chtyim Terence Yim
              Reporter:
              ashau Albert Shau
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development