> On Nov. 8, 2013, 2:08 p.m., Mike Drob 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> > > > > I would like to see this (and the other two exceptions) all extend > > RuntimeException. I think that since these are all conditions that can be > > checked for before attempting operations, it is unnecessarily burdensome on > > the client to force additional checked Exceptions on them. > > John Vines wrote: > A million times no. Do NOT force clients to catch RuntimeExceptions to > handle your error codes. > > Christopher Tubbs wrote: > I agree they should not be RuntimeExceptions. > > Mike Drob wrote: > You're already forcing clients to catch the exceptions when you declare > them as checked exceptions. And there is an exists method on operations, > which is completely pointless with the checked exceptions. Consider the > following code snippets: > > // Can write this if TableNamespaceExistsException is Runtime > if (! namespaceOperations.exists("foo")) > namespaceOperations.create("foo") > > // If it is checked, then you have to write > if (! namespaceOperations.exists("foo")) { > try { > namespaceOperations.create("foo") > } catch (TableNamespaceExistsException tnee) { > // log? nothing we can do anyway. > } > } > > // Or even worse, skip the existence check entirely and just try to > create... > // With this sample, what is even the point of having an exists() method? > try { > namespaceOperations.create("foo") > } catch (Exception e) { > // log? nothing we can do anyway. > } > > > Now, my preferred snippet (#1) _is_ susceptible to a potential race > condition, but I think the solution is a more serious API redesign. Maybe > have create return a boolean instead of throwing, similar to > Collection.add(). Or maybe add a createIfAbsent() method, similar to > ConcurrentHashMap.putIfAbsent(). > > There's lots of ways to do this, but forcing users to catch clunky > exceptions is one of my least favorite parts of Java. Also see Effective > Java, Item 59. > > John Vines wrote: > Catching a well defined exception is a hell of a lot different then > catching RuntimeException. Runtime exceptions should be reserved for errors > where things are fundamentally broken, not because you're too lazy to write a > catch clause. > > John Vines wrote: > And while I do like the idea of a boolean for operation returns, they > don't provide any input as to why they fail, which makes programmatic > handling worse. You're going to have drastically different behavior if a > table creation fails because it already exists vs. if you don't have > permissions for it vs. the cluster is on fire > > Josh Elser wrote: > I'm agree with John here. First, we already have the semantics of > throwing a named, non-Runtime exception with Table operations -- I see no > reason to treat namespaces differently than tables. Second, you're making a > sweeping assumption that all clients will do nothing when a namespace already > exists. You can't possibly know this to be true. > > Yes, it's more verbose, but this *is* Java which has never been known for > its subtlety. I'd much rather have a few more lines of code than withhold > error information from the client. > > Christopher Tubbs wrote: > After some consideration, I think I'm inclined to agree with Mike that > NamespaceNotFoundException should be a RuntimeException... because it > represents a programmer error, of the type described by Joshua Bloch in > Effective Java, rather than an easily recoverable situation. It's not that I > expect clients to do nothing if it already exists... it's that I expect > clients shouldn't be getting into this situation in the first place. I'm not > 100% convinced that it applies to NamespaceExistsException... right now I'm > inclined to think that it should be checked, because it's behavior should be > considered independently to the exists() method. > > However, I'm going to leave them both as checked exceptions so that the > API is not too inconsistent with TableOperations. I think that'd be more > disruptive to users. We can revamp both these APIs and improve them together > in the future. > > John Vines wrote: > Again, STRONGLY disagree. We're dealing with a distributed system, and as > such could have clients racing one another to check and then create the > namespace if it does not exist. These are perfectly valid conditions that can > be raced against one another and cause a failure in spite of however much > prechecking is done. So there are cases where it is NOT necessarily a > programmer error and should NOT be a runtime exception.
Hmm, the whole point of namespaces is to ease administration of groups of tables. If you're dynamically creating a namespace from multiple places, you're kind of undermining the whole point of the feature. However, the point is moot. I'm not making it a RuntimeException for now. I've talked to Mike about this, and he agreed that it'd be acceptable to keep consistency with the TableOperations API as is. So, we can discuss the merits of this if/when it comes up again. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15166/#review28554 ----------------------------------------------------------- On Oct. 31, 2013, 10:01 p.m., Christopher Tubbs wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15166/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2013, 10:01 p.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 > >
