[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610

joshelser
GitHub user milleruntime opened a pull request:

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

    ACCUMULO-4583 and ACCUMULO-4610

    Created Namespace operations that were missing from proxy/src/main/thrift/proxy.thrift and proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java
   
    Added Tests to test/src/test/java/org/apache/accumulo/test/proxy/SimpleProxyBase.java
   
    The remaining changes were all generated using -Pthrift

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

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

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

    https://github.com/apache/accumulo/pull/234.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 #234
   
----
commit 1a0ee9028aa762924d4108f3c75ecd4d7e831d8d
Author: Mike Miller <[hidden email]>
Date:   2017-03-21T20:52:26Z

    ACCUMULO-4610: Added missing namespace ops to thrift proxy

commit 00bf82f0aab0285fc49280a01e3cc411d433a7fe
Author: Mike Miller <[hidden email]>
Date:   2017-03-22T15:47:20Z

    ACCUMULO-4610: New generated thrift files

commit 5dd43feccace3df2bde663ebedda1f3ff6dd38f0
Author: Mike Miller <[hidden email]>
Date:   2017-03-22T17:38:25Z

    ACCUMULO-4583: Added Test for Thrift namespace permissions

----


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610

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

    https://github.com/apache/accumulo/pull/234#discussion_r107486999
 
    --- Diff: test/src/test/java/org/apache/accumulo/test/proxy/SimpleProxyBase.java ---
    @@ -1362,6 +1364,82 @@ public void userPermissions() throws Exception {
       }
     
       @Test
    +  public void testNamespaceOps() throws Exception {
    +    String testNamespace = "testnamespace";
    +    String testNamespace2 = "testnamespace2";
    +    assertEquals("", client.defaultNamespace(creds));
    +    assertEquals("accumulo", client.systemNamespace(creds));
    +    client.createNamespace(creds, testNamespace);
    +    assertTrue(client.namespaceExists(creds, testNamespace));
    +    assertTrue(client.listNamespaces(creds).contains(testNamespace));
    --- End diff --
   
    Would be good to also check the size of the returned list.  This will make the test catch unexpected junk in the returned list.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610

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

    https://github.com/apache/accumulo/pull/234#discussion_r107487146
 
    --- Diff: test/src/test/java/org/apache/accumulo/test/proxy/SimpleProxyBase.java ---
    @@ -1362,6 +1364,82 @@ public void userPermissions() throws Exception {
       }
     
       @Test
    +  public void testNamespaceOps() throws Exception {
    +    String testNamespace = "testnamespace";
    +    String testNamespace2 = "testnamespace2";
    +    assertEquals("", client.defaultNamespace(creds));
    +    assertEquals("accumulo", client.systemNamespace(creds));
    +    client.createNamespace(creds, testNamespace);
    +    assertTrue(client.namespaceExists(creds, testNamespace));
    +    assertTrue(client.listNamespaces(creds).contains(testNamespace));
    +    client.renameNamespace(creds, testNamespace, testNamespace2);
    +    assertTrue(client.namespaceExists(creds, testNamespace2));
    +    assertFalse(client.namespaceExists(creds, testNamespace));
    --- End diff --
   
    could also call `listnamespaces` after the rename


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610

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

    https://github.com/apache/accumulo/pull/234#discussion_r107487883
 
    --- Diff: proxy/src/main/thrift/proxy.thrift ---
    @@ -369,6 +382,19 @@ service AccumuloProxy
       bool testTableClassLoad (1:binary login, 2:string tableName, 3:string className                    
                                , 4:string asTypeName)                                                      throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:TableNotFoundException ouch3);
     
    +  //namespace operations
    +  string systemNamespace (1:binary login);
    +  string defaultNamespace (1:binary login);
    +  bool namespaceExists (1:binary login, 2:string namespaceName)                                        throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2);
    +  set<string> listNamespaces (1:binary login)                                                          throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2);
    +  void createNamespace (1:binary login, 2:string namespaceName)                                        throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:NamespaceExistsException ouch3);
    +  void deleteNamespace (1:binary login, 2:string namespaceName)                                        throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:NamespaceNotFoundException ouch3, 4:NamespaceNotEmptyException ouch4);
    +  void renameNamespace (1:binary login, 2:string oldNamespace, 3:string newNamespace)                  throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:NamespaceNotFoundException ouch3, 4:NamespaceExistsException ouch4);
    +  void setNamespaceProperty (1:binary login, 2:string namespaceName, 3:string property, 4:string value)throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:NamespaceNotFoundException ouch3);
    --- End diff --
   
    I didn't see test for namespace props


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610

joshelser
In reply to this post by joshelser
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/234
 
    Since we added the fix for the missing permissions in 1.7.3, I was adding the tests for that regression. And I had no way to do that with the current proxy.  Even though the thrift proxy doesn't fall under semver, I would be OK with pushing these changes back to master since it is new API.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610

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

    https://github.com/apache/accumulo/pull/234#discussion_r107500158
 
    --- Diff: test/src/test/java/org/apache/accumulo/test/proxy/SimpleProxyBase.java ---
    @@ -1362,6 +1364,82 @@ public void userPermissions() throws Exception {
       }
     
       @Test
    +  public void testNamespaceOps() throws Exception {
    +    String testNamespace = "testnamespace";
    +    String testNamespace2 = "testnamespace2";
    +    assertEquals("", client.defaultNamespace(creds));
    +    assertEquals("accumulo", client.systemNamespace(creds));
    +    client.createNamespace(creds, testNamespace);
    +    assertTrue(client.namespaceExists(creds, testNamespace));
    +    assertTrue(client.listNamespaces(creds).contains(testNamespace));
    --- End diff --
   
    Yeah I like that, I will add it.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610

joshelser
In reply to this post by joshelser
Github user keith-turner commented on the issue:

    https://github.com/apache/accumulo/pull/234
 
    > Even though the thrift proxy doesn't fall under semver, I would be OK with pushing these changes back to master since it is new API.
   
    Since the proxy is not officially under semver we could technically make this change to 1.7.X.  I would not oppose such a change, but I am not in favor of it.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610

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

    https://github.com/apache/accumulo/pull/234#discussion_r107504732
 
    --- Diff: proxy/src/main/thrift/proxy.thrift ---
    @@ -369,6 +382,19 @@ service AccumuloProxy
       bool testTableClassLoad (1:binary login, 2:string tableName, 3:string className                    
                                , 4:string asTypeName)                                                      throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:TableNotFoundException ouch3);
     
    +  //namespace operations
    +  string systemNamespace (1:binary login);
    +  string defaultNamespace (1:binary login);
    +  bool namespaceExists (1:binary login, 2:string namespaceName)                                        throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2);
    +  set<string> listNamespaces (1:binary login)                                                          throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2);
    +  void createNamespace (1:binary login, 2:string namespaceName)                                        throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:NamespaceExistsException ouch3);
    +  void deleteNamespace (1:binary login, 2:string namespaceName)                                        throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:NamespaceNotFoundException ouch3, 4:NamespaceNotEmptyException ouch4);
    +  void renameNamespace (1:binary login, 2:string oldNamespace, 3:string newNamespace)                  throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:NamespaceNotFoundException ouch3, 4:NamespaceExistsException ouch4);
    +  void setNamespaceProperty (1:binary login, 2:string namespaceName, 3:string property, 4:string value)throws (1:AccumuloException ouch1, 2:AccumuloSecurityException ouch2, 3:NamespaceNotFoundException ouch3);
    --- End diff --
   
    Forgot tests for properties, will add, thanks.


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610

joshelser
In reply to this post by joshelser
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/234
 
    @mjwall @joshelser any opinion/desire spurred from https://issues.apache.org/jira/browse/ACCUMULO-4519 for this to go in bug fix branch?


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610

joshelser
In reply to this post by joshelser
Github user joshelser commented on the issue:

    https://github.com/apache/accumulo/pull/234
 
    > for this to go in bug fix branch?
   
    I am OK with this landing on bug-fixes


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610

joshelser
In reply to this post by joshelser
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/234
 
    Nevermind, closing this PR.  It looks like these changes have already been done in master. #facepalm


---
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
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610

joshelser
In reply to this post by joshelser
Github user milleruntime closed the pull request at:

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


---
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.
---
Loading...