You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@curator.apache.org by madrob <gi...@git.apache.org> on 2015/07/09 01:18:06 UTC

[GitHub] curator pull request: CURATOR-232 Consolidate Timing and Client

GitHub user madrob opened a pull request:

    https://github.com/apache/curator/pull/89

    CURATOR-232 Consolidate Timing and Client

    Pull references to Timing and CuratorFramework client into BeforeMethod
    and AfterMethod invocations so that we can manage them more efficiently.

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

    $ git pull https://github.com/madrob/curator CURATOR-232

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

    https://github.com/apache/curator/pull/89.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 #89
    
----
commit a1ff02690ebe24b4d4aa2c6d431c71e65ea14503
Author: Mike Drob <md...@cloudera.com>
Date:   2015-07-08T23:16:17Z

    CURATOR-232 Consolidate Timing and Client
    
    Pull references to Timing and CuratorFramework client into BeforeMethod
    and AfterMethod invocations so that we can manage them more efficiently.

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-232 Consolidate Timing and Client

Posted by Randgalt <gi...@git.apache.org>.

Github user Randgalt commented on the pull request:

    https://github.com/apache/curator/pull/89#issuecomment-122570334
  
    I'm concerned about the number of changes in this PR. I like the idea of putting some common functionality in BaseClassForTests but I think it's too risky to touch all the tests. I suggest that we add the things to BaseClassForTests but leave the tests as is an updated them as needed over time.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-232 Consolidate Timing and Client

Posted by Randgalt <gi...@git.apache.org>.

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

    https://github.com/apache/curator/pull/89#discussion_r40508128
  
    --- Diff: curator-test/src/main/java/org/apache/curator/test/BaseClassForTests.java ---
    @@ -55,6 +63,8 @@
             INTERNAL_RETRY_FAILED_TESTS = retryFailedTests;
         }
     
    +    private List<Closeable> toClose;
    --- End diff --
    
    Is this needed anymore given that Java 7 has Autocloseable?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] curator pull request: CURATOR-232 Consolidate Timing and Client

Posted by Randgalt <gi...@git.apache.org>.

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

    https://github.com/apache/curator/pull/89#discussion_r34949657
  
    --- Diff: curator-framework/src/test/java/org/apache/curator/framework/FrameworkTest.java ---
    @@ -0,0 +1,83 @@
    +/*
    --- End diff --
    
    I don't see enough value in this class to maintain it. In order for it to be used in the recipes module we'd have to build a test jar and include and/or create another test module. Create new clients isn't always consistent in the tests and it's only 1 line of code anyway. The closeable stuff could be pushed into BaseClassForTests by having a List<Closeable> that can be added to and auto closed on @After.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---