[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

ctubbsii
GitHub user milleruntime opened a pull request:

    https://github.com/apache/accumulo/pull/232

    ACCUMULO-4600: Fix to properly read from accumulo-site.xml

    Changes to Shell that should fix this problem without having to revert ACCUMULO-4505
   
    TODO: Fix ShellConfigTest to properly test and not just compile.

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

    $ git pull https://github.com/milleruntime/accumulo ACCUMULO-4600

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

    https://github.com/apache/accumulo/pull/232.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 #232
   
----
commit 41d6d35bb980348e8cc60b8ac762451ff7f58303
Author: Mike Miller <[hidden email]>
Date:   2017-03-08T21:12:48Z

    ACCUMULO-4600: Fix to properly read from accumulo-site.xml

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/232#discussion_r105032444
 
    --- Diff: shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java ---
    @@ -126,7 +126,7 @@ public void testZooKeeperHostFallBackToSite() throws Exception {
         Map<String,String> data = new HashMap<>();
         data.put(Property.INSTANCE_ZK_HOST.getKey(), "site_hostname");
         AccumuloConfiguration conf = new ConfigurationCopy(data);
    --- End diff --
   
    Seems like this configuration is being created and not used.  Does this test pass?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/232#discussion_r105150530
 
    --- Diff: shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java ---
    @@ -126,7 +126,7 @@ public void testZooKeeperHostFallBackToSite() throws Exception {
         Map<String,String> data = new HashMap<>();
         data.put(Property.INSTANCE_ZK_HOST.getKey(), "site_hostname");
         AccumuloConfiguration conf = new ConfigurationCopy(data);
    --- End diff --
   
    No see above TODO


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/232#discussion_r105180990
 
    --- Diff: shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java ---
    @@ -123,29 +118,21 @@ public void testTokenAndOptionAndPassword() throws IOException {
       @Test
       public void testZooKeeperHostFallBackToSite() throws Exception {
         ClientConfiguration clientConfig = new ClientConfiguration();
    -    Map<String,String> data = new HashMap<>();
    -    data.put(Property.INSTANCE_ZK_HOST.getKey(), "site_hostname");
    -    AccumuloConfiguration conf = new ConfigurationCopy(data);
    -    assertEquals("site_hostname", Shell.getZooKeepers(null, clientConfig));
    +    assertFalse("Client config contains zk hosts", clientConfig.containsKey(ClientConfiguration.ClientProperty.INSTANCE_ZK_HOST.getKey()));
    +    assertEquals("localhost:2181", Shell.getZooKeepers(null, clientConfig));
    --- End diff --
   
    it does not seem like this is testing falling back to site


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/232#discussion_r105182870
 
    --- Diff: shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java ---
    @@ -123,29 +118,21 @@ public void testTokenAndOptionAndPassword() throws IOException {
       @Test
       public void testZooKeeperHostFallBackToSite() throws Exception {
         ClientConfiguration clientConfig = new ClientConfiguration();
    -    Map<String,String> data = new HashMap<>();
    -    data.put(Property.INSTANCE_ZK_HOST.getKey(), "site_hostname");
    -    AccumuloConfiguration conf = new ConfigurationCopy(data);
    -    assertEquals("site_hostname", Shell.getZooKeepers(null, clientConfig));
    +    assertFalse("Client config contains zk hosts", clientConfig.containsKey(ClientConfiguration.ClientProperty.INSTANCE_ZK_HOST.getKey()));
    +    assertEquals("localhost:2181", Shell.getZooKeepers(null, clientConfig));
    --- End diff --
   
    It does not seem that way but the last resort of `Shell.getZooKeepers()` is to make a static call to get the site configuration:
    `SiteConfiguration.getInstance(ClientContext.convertClientConfig(clientConfig)).get(Property.INSTANCE_ZK_HOST)`


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/232#discussion_r105184133
 
    --- Diff: shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java ---
    @@ -123,29 +118,21 @@ public void testTokenAndOptionAndPassword() throws IOException {
       @Test
       public void testZooKeeperHostFallBackToSite() throws Exception {
         ClientConfiguration clientConfig = new ClientConfiguration();
    -    Map<String,String> data = new HashMap<>();
    -    data.put(Property.INSTANCE_ZK_HOST.getKey(), "site_hostname");
    -    AccumuloConfiguration conf = new ConfigurationCopy(data);
    -    assertEquals("site_hostname", Shell.getZooKeepers(null, clientConfig));
    +    assertFalse("Client config contains zk hosts", clientConfig.containsKey(ClientConfiguration.ClientProperty.INSTANCE_ZK_HOST.getKey()));
    +    assertEquals("localhost:2181", Shell.getZooKeepers(null, clientConfig));
    --- End diff --
   
    So its testing that it falls back to the default?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/232#discussion_r105229771
 
    --- Diff: shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java ---
    @@ -123,29 +118,21 @@ public void testTokenAndOptionAndPassword() throws IOException {
       @Test
       public void testZooKeeperHostFallBackToSite() throws Exception {
         ClientConfiguration clientConfig = new ClientConfiguration();
    -    Map<String,String> data = new HashMap<>();
    -    data.put(Property.INSTANCE_ZK_HOST.getKey(), "site_hostname");
    -    AccumuloConfiguration conf = new ConfigurationCopy(data);
    -    assertEquals("site_hostname", Shell.getZooKeepers(null, clientConfig));
    +    assertFalse("Client config contains zk hosts", clientConfig.containsKey(ClientConfiguration.ClientProperty.INSTANCE_ZK_HOST.getKey()));
    +    assertEquals("localhost:2181", Shell.getZooKeepers(null, clientConfig));
    --- End diff --
   
    Yeah it was.  I added the site file to resources in 2b57295


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #232: ACCUMULO-4600: Fix to properly read from accumul...

ctubbsii
In reply to this post by ctubbsii
Github user asfgit closed the pull request at:

    https://github.com/apache/accumulo/pull/232


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---