> 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. > > Sean Hickey wrote: > 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. > > Christopher Tubbs wrote: > I think this can be addressed later. I think that where the rare places > where a TableNamespaceNotFoundException is wrapped with a RuntimeException, > it is sufficient, because namespace operations should be rare, and namespaces > can generally be stable. It's not that I think it shouldn't be addressed... I > just think it's not a priority.
I disagree. Wrapping legitimate exceptions in RTEs A. lead to shitty user APIs and B. lead to APIs that will have to be deprecated in order to implement them properly > On Nov. 1, 2013, 10:19 p.m., John Vines wrote: > > core/src/main/java/org/apache/accumulo/core/client/admin/TableNamespaceOperationsImpl.java, > > line 335 > > <https://reviews.apache.org/r/15166/diff/1/?file=376084#file376084line335> > > > > RuntimeException should not be used here How was this fixed? > 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. > > Sean Hickey wrote: > 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. > > Christopher Tubbs wrote: > I don't think users should parse error messages. But, your initial > comment wasn't about being able to programmatically determine the error... it > was about potential for confusion. My comment was in that regard. > > I don't want to create a bunch of new thrift exceptions at this point... > but it's an option. For now, I can fix the NOTFOUND oversight, so a better > exception is thrown in the client API. This also seems to be marked as resolved by not fixed in the 802 branch... > On Nov. 1, 2013, 10:19 p.m., John Vines wrote: > > server/base/src/main/java/org/apache/accumulo/server/conf/TableNamespaceConfWatcher.java, > > line 29 > > <https://reviews.apache.org/r/15166/diff/1/?file=376133#file376133line29> > > > > Why is this overriding the logging? > > Christopher Tubbs wrote: > Because TableConfWatcher does. If we don't need to do that, we can create > a separate ticket. > > John Vines wrote: > Ticket opened > > Christopher Tubbs wrote: > Ticket number? ACCUMULO-1871 > 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. > > Sean Hickey wrote: > 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. > > Christopher Tubbs wrote: > JV, yes, I took the opportunity to muse, following what looked to be a > musing yourself. If you were proposing a change, and not simply musing, I'm > not sure I understand what specifically you are suggesting, if it's not a > permissions policy issue... (eg. which permissions are required to allow the > specified action). I'm suggesting instead of replicating every single Table operation and table permission call to reflect table namespaces that we change the table operations to do both the individual table and the table namespace lookup. It should be adding a _hasTableNamespacePermission and changing hasTablePermission to call both _hasTablePermission and _hasTableNamespacePermission (or changing _hasTablePermission to do both the Table and TableNamespace checks, I could go either way currently). Then every single can* operation can remain unchanged. This is critical because any other can* checks could easily slip up the logic or miss a namespace, which would not be good. - John ----------------------------------------------------------- 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 > >
