accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Hickey" <tallirishll...@gmail.com>
Subject Re: Review Request 15166: ACCUMULO-802 Tablespaces
Date Sun, 10 Nov 2013 00:50:17 GMT


> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > core/src/main/java/org/apache/accumulo/core/security/TableNamespacePermission.java,
line 26
> > <https://reviews.apache.org/r/15166/diff/1/?file=376101#file376101line26>
> >
> >     I think we also need BULK_IMPORT for bulk importing to namespace tables, DROP
for dropping the entire namespace, and possibly GRANT_TABLE (though I can see that being iffy)

I thought about adding BULK_IMPORT, but I didn't add it because I don't think I added bulk
importing for namespaces. I made DROP_NAMESPACE in SystemPermission, but I agree that it should
also be in TableNamespacePermission (because TablePermission has DROP_TABLE). I was a little
confused how permissions were supposed to work in general, so I don't know how well GRANT_TABLE
would work as a namespace permission.


> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java,
line 311
> > <https://reviews.apache.org/r/15166/diff/1/?file=376139#file376139line311>
> >
> >     Part of is wondering we want to change the hasTablePermission(blah) to hit both
the table and the namespace
> 
> Christopher Tubbs wrote:
>     I'm a big fan of changing this whole pluggable permissions model to one where we
have a simple:
>     boolean canPerform(User user, Action action); // possibly with an optional "environment"
as an arbitrary property map
>     
>     With a few defined Actions, like TableRenameAction(oldName, newName), etc. and User
with a strict interface like "boolean isAuthenticated(); String getPrincipal();", the pluggable
permissions handler becomes an operations-based implementation of *policy*, rather than a
dumb storage for a difficult to extend set of permissions *tokens*. The default implementation
will still be token based, but the API will be much cleaner, much more extensible/pluggable,
and wouldn't change when we add new features like this.
>     
>     Had we done that, this would be trivial to extend in a sensible way.
> 
> John Vines wrote:
>     It's still trivial now (more trivial than the current implementation), you're just
using this circumstance to gripe about how you want things to be, we have other tickets for
that. That doesn't change things for this release.
>     
>     This change simply adds a namespace argument to the hasTablePermission or _hasTablePermission
call that can be propagated throughout with simple signature changing.

It might prove convenient to have hasTablePermission to check both, but the structure I was
following was that each security check (all the "canDoSomething" functions) had a separate
check for the table and system permissions that might allow it to do that. Because of that,
the permission that might allow you do something might be different for the table vs namespace
vs system. This might be from me misunderstanding the permissions model, but for example,
when you check for canCompact(table), we're checking for ALTER_TABLE in both System and Namespace
permissions but then WRITE for Table permissions. If we change it so that hasTablePermission
checks for namespace (and maybe even system) permissions, we'll just need to realize what
that specific table permission might translate to for the higher level permissions.


> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java,
line 309
> > <https://reviews.apache.org/r/15166/diff/1/?file=376152#file376152line309>
> >
> >     Locking on NamespaceIds for compactions, and many other FATE operations, could
be a nasty bottleneck. Do we really need to lock on them?!
> 
> Christopher Tubbs wrote:
>     I'm not sure. Probably not, but could there be problems with moving tables between
namespaces in some cases if not? Or, perhaps preventing that would be a better way to go.
> 
> John Vines wrote:
>     I'm sure some operations need to lock namespaces, but not as many as we do now. Possibly
a multi-read/write lock is needed

Maybe not, but I just didn't want to introduce any concurrency issues. For most table operations,
it does only grab a read-lock on the namespace (so other people just can't change the namespace
while you're changing a table). 


> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > server/master/src/main/java/org/apache/accumulo/master/Master.java, line 953
> > <https://reviews.apache.org/r/15166/diff/1/?file=376147#file376147line953>
> >
> >     I think we should have dedicated thrift returns for these. Or at least dedicated
thrift error codes

Ya, I wasn't sure how to handle that exactly. Looking back at the code, there is a TableNotFoundException
that's a Thrift Exception, so we should probably just make a similar one for namespaces.


> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java,
line 431
> > <https://reviews.apache.org/r/15166/diff/1/?file=376130#file376130line431>
> >
> >     This error could cause confusion when you have a table in the default namespace
with the same name as a namespace (if that's possible?)
> 
> Christopher Tubbs wrote:
>     I don't think so, not since the "why" string explicitly says "Could not find table
namespace while getting configuration."
> 
> John Vines wrote:
>     Resorting to parsing strings to determine errors programmatically is pretty crummy
for end users. Also, looking through this code path shows that TableNamespaceOperationsImpl#getProperties
doesn't work right since it looks for the type NOTFOUND, which this error isn't setting. Perhaps
we should make TableOperation a combination of Table and TableNamespace options in order to
support backwards compatibility while maintaining a solid set of easily decipherable errors.

Well, if you really want to be able to easily find it programmatically, it might be best to
just make a new type of exception (like ThriftNamespaceOperationException or something) because
all the namespace stuff is new and won't break any backwards compatibility. I was just trying
to reuse some existing stuff, especially with the thrift calls because I was brand new to
thrift.


> On Nov. 1, 2013, 10:19 p.m., John Vines wrote:
> > core/src/main/java/org/apache/accumulo/core/client/TableNamespaceExistsException.java,
line 24
> > <https://reviews.apache.org/r/15166/diff/1/?file=376077#file376077line24>
> >
> >     If we make TableNamespaceExistException extend AccumuloExtension we can get
around a lot of api compatibility issues
> 
> Christopher Tubbs wrote:
>     That doesn't follow the existing pattern with TableExistsException, and the Exception
does not appear to be thrown in any existing API. To what API-compatibility issues are you
referring?
> 
> John Vines wrote:
>     There is an issue with TableNamespaceNotFoundException and I think all 3 of these
exceptions should be in the same family, i.e. implemented by similar means.

Like Christopher pointed out, I was just following what Tables did with their exceptions.
If you think that should be different, then we might want to change the table exceptions too.


- Sean


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15166/#review28029
-----------------------------------------------------------


On Nov. 1, 2013, 2:01 a.m., Christopher Tubbs wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15166/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2013, 2:01 a.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-802
>     https://issues.apache.org/jira/browse/ACCUMULO-802
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-802 Tablespaces (Table Namespaces), work done by Sean Hickey, https://github.com/Wisellama/accumulo/tree/ACCUMULO-802,
rebase'd onto latest master (including 210 commits)
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/Constants.java 9db0c405c5b9fbac13fa735c3ffd6433e9831051

>   core/src/main/java/org/apache/accumulo/core/client/Connector.java bbfa55f4b9ad8fc0e5f0c0058e2e0564685d7c85

>   core/src/main/java/org/apache/accumulo/core/client/TableNamespaceExistsException.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/TableNamespaceNotEmptyException.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/TableNamespaceNotFoundException.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/admin/SecurityOperations.java 86a3ff271fc2e085c426da3156cfab9cdbb5c36b

>   core/src/main/java/org/apache/accumulo/core/client/admin/SecurityOperationsImpl.java
0f0e998f334631cfb342f9a39fdd12e62aa98f13 
>   core/src/main/java/org/apache/accumulo/core/client/admin/TableNamespaceOperations.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/admin/TableNamespaceOperationsHelper.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/admin/TableNamespaceOperationsImpl.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/admin/TableOperationsImpl.java 1b652f99e23e2dfa06c7373a6f4c3c044f5cd1a3

>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java bd1156912849b13ebad8f6b974fd35d8adf18f1d

>   core/src/main/java/org/apache/accumulo/core/client/impl/TableNamespaces.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java 8bc725a3405a79c20c690bff3f6fdb6f713be523

>   core/src/main/java/org/apache/accumulo/core/client/impl/thrift/ClientService.java 488e0654720250542145e0d543ad16813f8d8b6d

>   core/src/main/java/org/apache/accumulo/core/client/impl/thrift/SecurityErrorCode.java
b706ce87bb1e8a525e585b14b7fb8563c3493c2c 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 5ee144d9eb67c8e79dad870abbd823cac64e111e

>   core/src/main/java/org/apache/accumulo/core/client/mock/MockConnector.java 80ec5133334f9ef466dd7e4077230acfd2c77fb5

>   core/src/main/java/org/apache/accumulo/core/client/mock/MockSecurityOperations.java
765cda9cac6fb3bc7df9eb8db1c83846960e85c3 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 3dcab11bd50bf189ccc4cbaf97d3d4ecb9133047

>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableNamespace.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableNamespaceOperations.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java a3c2043b8120e13a190eb173d8342c205700382f

>   core/src/main/java/org/apache/accumulo/core/client/security/SecurityErrorCode.java
fb51387fabea6d47854048daad4c8ec14047ab33 
>   core/src/main/java/org/apache/accumulo/core/master/thrift/MasterClientService.java
5b9949a1b6b7997d08de371c9a36c1e1a2b50b1f 
>   core/src/main/java/org/apache/accumulo/core/security/SystemPermission.java 699396e7e47b858ea8273c969f79dc7b3ebdc577

>   core/src/main/java/org/apache/accumulo/core/security/TableNamespacePermission.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java 52e1d04998060954b7f09a3b9a6d9fdd898208b0

>   core/src/main/java/org/apache/accumulo/core/util/shell/ShellCompletor.java 85f68ef2464cb784861a587badd4835d842d8b87

>   core/src/main/java/org/apache/accumulo/core/util/shell/ShellOptions.java 31529730935ff78de6c39fea531e20e2ade30a2d

>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/CloneNamespaceCommand.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/ConfigCommand.java
73d21ef6108282c9dd8dd8bc0f2ce77524acedcf 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/ConstraintCommand.java
4280555dfce0856e2e1329412b0051b15ce5a748 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/CreateNamespaceCommand.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/CreateTableCommand.java
0ad787d67eaa5a90d26e113d7a79f0f3218ce0f4 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/DUCommand.java 189458a504dce012f3c579ff3b8425f030ad1b54

>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/DeleteIterCommand.java
d0644828af83f5c2d2ecbc58bd7545d958be31a0 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/DeleteNamespaceCommand.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/DeleteTableCommand.java
35c534d791648267539833b922643fc65dd600d4 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/GrantCommand.java f3e720029714da63a7ec7e2bfb7587443ed7ed8b

>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/ListIterCommand.java
e2d4d915c3311205ca4731b449769c37cd92ac82 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/NamespacePermissionsCommand.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/NamespacesCommand.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/OptUtil.java f0c14d4cfd9ebf5a480ac63bb9dcd41e085b22f7

>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/RenameNamespaceCommand.java
PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/RenameTableCommand.java
057f2b4374b3cc137a20158e70c33dd97c019c87 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/RevokeCommand.java
676284aa9591d05a946d0bc4290ff88c3eb04eba 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/SetIterCommand.java
3a7155687994049d2580f7b39e0df1a39d52c385 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/TableOperation.java
2bec526af38d328ea86cb21860471d8bed384eac 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/TablesCommand.java
19b49e9cb4ca8483de55e644700d2a68d14ab42f 
>   core/src/main/java/org/apache/accumulo/core/util/shell/commands/UserPermissionsCommand.java
79d78da3b0f6de47f57740ee13678792b61a87c7 
>   core/src/main/thrift/client.thrift 2f5e9d191a0d80d32e2a88a6e31845f3b93ae289 
>   core/src/main/thrift/master.thrift 19960c8857295d22ffba728c95501f069ba953c0 
>   core/src/test/java/org/apache/accumulo/core/client/mock/MockTableNamespacesTest.java
PRE-CREATION 
>   server/base/src/main/java/org/apache/accumulo/server/ServerConstants.java 9e0ac393deb962799e4e98753db1814dd630d14e

>   server/base/src/main/java/org/apache/accumulo/server/client/ClientServiceHandler.java
3f1aaa29ef023d2f33fa5b35aad2e57df9b6aff4 
>   server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfiguration.java
c9fd5a141a437aef6f1a9d8f8c80999f19af6ec8 
>   server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java 4c581530b77a88e141837507a32cdc4f89982c56

>   server/base/src/main/java/org/apache/accumulo/server/conf/TableNamespaceConfWatcher.java
PRE-CREATION 
>   server/base/src/main/java/org/apache/accumulo/server/conf/TableNamespaceConfiguration.java
PRE-CREATION 
>   server/base/src/main/java/org/apache/accumulo/server/conf/TableParentConfiguration.java
PRE-CREATION 
>   server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 0477a443e307e174965140f0fd14c898a0d98727

>   server/base/src/main/java/org/apache/accumulo/server/master/balancer/TableLoadBalancer.java
3e0a2bf0e06227780adb92967d439212c0316924 
>   server/base/src/main/java/org/apache/accumulo/server/security/AuditedSecurityOperation.java
ee337a5d23f04b321a306a306c41ea59e2f731a0 
>   server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java
f00159cc21d8a7b8a47efbbc3d1f2bd96f479a03 
>   server/base/src/main/java/org/apache/accumulo/server/security/handler/InsecurePermHandler.java
b57abfec483e3e07b85ed41148e11789fc45461c 
>   server/base/src/main/java/org/apache/accumulo/server/security/handler/PermissionHandler.java
72c64b5897a4b0bf7de8081a76288fd8fb639021 
>   server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java
f2196035d93ce5718b09e9754d130015515da91f 
>   server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java
3b9d8b24b85996dfbf734d4910aa6cd0d9247beb 
>   server/base/src/main/java/org/apache/accumulo/server/tables/TableManager.java 8a74d0b936d8fad3b020a230b6b20536d740288f

>   server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java PRE-CREATION

>   server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java cdee8fbe448592120a5cc5b6682aedfafca7e7bf

>   server/master/src/main/java/org/apache/accumulo/master/Master.java 8a1a9b272bf332258febf46b7535d12f83a1dbd0

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/CancelCompactions.java
dd4c2292996d758860191fc054631812adfa34b2 
>   server/master/src/main/java/org/apache/accumulo/master/tableOps/ChangeTableState.java
697c15e173b13bc5909862998b8742708bc933c1 
>   server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneTable.java 8d906d602ae1abdd2f4d774c64b3b4cd1174aa72

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/CloneTableNamespace.java
PRE-CREATION 
>   server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java 160fc7e89320250d24e89526188528ff4786cd6d

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/CreateTable.java df2e0285b10fbdacb9fcacd007b48c1e2891d5d1

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/CreateTableNamespace.java
PRE-CREATION 
>   server/master/src/main/java/org/apache/accumulo/master/tableOps/DeleteTable.java 427125721cfaa122378bfe74b6c89b96b2f93137

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/DeleteTableNamespace.java
PRE-CREATION 
>   server/master/src/main/java/org/apache/accumulo/master/tableOps/ExportTable.java 22df3b340bbb9e76ef1fabe16b521654eb7a6bcb

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/ImportTable.java b91da522ff13b2be4de3fe82c47875e74cc1e569

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/RenameTable.java 3069aaf9eea3044d14bf7bb7b81c0270a0997f08

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/RenameTableNamespace.java
PRE-CREATION 
>   server/master/src/main/java/org/apache/accumulo/master/tableOps/TableRangeOp.java 0ad2196a30a959f5ca5c7bdaf6d46a8628fd957a

>   server/master/src/main/java/org/apache/accumulo/master/tableOps/Utils.java fa14f43100f5a13b16d2d34b95a8466501b93299

>   server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java 0ef6c6b867b284e86f8252ebb19913b3c1831362

>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/ChangePermissions.java
be2de2a547aecc72eca7e2c980964ef85551aae8 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CheckPermission.java
5fa9bc49439974071fe12d97b371eb4e6ac3db3f 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CloneTable.java 65d3aed4faf1f2a360aa0d4df123390206eae0f3

>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CloneTableNamespace.java
PRE-CREATION 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/Config.java b47f1a946e904820a0cc83a4b3e078d40f11bb99

>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CreateTable.java
0aa7f7ad0882a3d82bd6ca423a3a8a3dac706fba 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/CreateTableNamespace.java
PRE-CREATION 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/DeleteTableNamespace.java
PRE-CREATION 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/Merge.java 8fcfab593071458954939970892a88b8793088b3

>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/OfflineTableNamespace.java
PRE-CREATION 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/RenameTable.java
d4a4e956699c2b2a483d6360e201da3eccd29847 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/RenameTableNamespace.java
PRE-CREATION 
>   test/src/main/java/org/apache/accumulo/test/randomwalk/concurrent/Setup.java 2cf37cecb8223b31b58e1afa4fa08ebf9967a8e1

>   test/src/main/java/org/apache/accumulo/test/randomwalk/security/WalkingSecurity.java
0ddb75254da156d75bbb0ca1f0a6e63c2443ab1c 
>   test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 31867f39dd4d9661a00557c3291258b5e2507b30

>   test/src/test/java/org/apache/accumulo/test/TableNamespacesIT.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/functional/PermissionsIT.java b8e1a4fcfcd1f02aa12926a44046493dbe74fe15

>   test/system/randomwalk/conf/modules/Concurrent.xml 03e5542d6b5b02f09e899301c68cf1d6746285f5

> 
> Diff: https://reviews.apache.org/r/15166/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christopher Tubbs
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message