You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by milleruntime <gi...@git.apache.org> on 2017/03/22 17:50:25 UTC
[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610
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 <mm...@apache.org>
Date: 2017-03-21T20:52:26Z
ACCUMULO-4610: Added missing namespace ops to thrift proxy
commit 00bf82f0aab0285fc49280a01e3cc411d433a7fe
Author: Mike Miller <mm...@apache.org>
Date: 2017-03-22T15:47:20Z
ACCUMULO-4610: New generated thrift files
commit 5dd43feccace3df2bde663ebedda1f3ff6dd38f0
Author: Mike Miller <mm...@apache.org>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by keith-turner <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by keith-turner <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by keith-turner <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by keith-turner <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #234: ACCUMULO-4583 and ACCUMULO-4610
Posted by milleruntime <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---