[ https://issues.apache.org/jira/browse/HBASE-5732?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13271919#comment-13271919 ]
jirapos...@reviews.apache.org commented on HBASE-5732: ------------------------------------------------------ bq. On 2012-05-09 18:02:12, Andrew Purtell wrote: bq. > Mostly +1. A couple of minor comments, one question on API annotation. Thanks, Andrew. bq. On 2012-05-09 18:02:12, Andrew Purtell wrote: bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java, line 677 bq. > <https://reviews.apache.org/r/4953/diff/4/?file=107883#file107883line677> bq. > bq. > Do we have a testcase for relogin? bq. > bq. > In our production with the older implementation of secure HBase RPC, we see swarms of GSS initiaition failure due to missing TGT for 5-10 seconds, and we speculate this is a race around relogin. It's not straightforward to write a testcase for the security APIs, unfortunately. I'd propose we open a jira for the relogin issue (over in hadoop, iirc, we've fixed some issues to do with relogin races in the recent past; we should put the fixes here)... bq. On 2012-05-09 18:02:12, Andrew Purtell wrote: bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 1328 bq. > <https://reviews.apache.org/r/4953/diff/4/?file=107884#file107884line1328> bq. > bq. > Debug logging should be wrapped in a conditional, no biggie. Done. bq. On 2012-05-09 18:02:12, Andrew Purtell wrote: bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java, line 1937 bq. > <https://reviews.apache.org/r/4953/diff/4/?file=107884#file107884line1937> bq. > bq. > This is probably just going to add noise. Removed. bq. On 2012-05-09 18:02:12, Andrew Purtell wrote: bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java, line 35 bq. > <https://reviews.apache.org/r/4953/diff/4/?file=107889#file107889line35> bq. > bq. > Maybe should be "security.hbase.*", what do you think? This is how it is in the current trunk. But yeah, makes sense to have the key as "security.hbase.*". bq. On 2012-05-09 18:02:12, Andrew Purtell wrote: bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java, line 94 bq. > <https://reviews.apache.org/r/4953/diff/4/?file=107890#file107890line94> bq. > bq. > It might be better to say 'Kerberos principal does not have the expected format' Done. bq. On 2012-05-09 18:02:12, Andrew Purtell wrote: bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java, line 53 bq. > <https://reviews.apache.org/r/4953/diff/4/?file=107892#file107892line53> bq. > bq. > Should this be Public+Evolving? Done. Although I was wondering whether it makes sense to have it as Private+Evolving. But if it is currently used by apps outside the core of hbase, it makes sense to have it as Public+Evolving.. bq. On 2012-05-09 18:02:12, Andrew Purtell wrote: bq. > http://svn.apache.org/repos/asf/hbase/trunk/src/test/resources/hbase-site.xml, line 126 bq. > <https://reviews.apache.org/r/4953/diff/4/?file=107918#file107918line126> bq. > bq. > Why this change? Unrelated junk from other work? In the hbase-site.xml of security/src/test/resources, this block of configuration was there. Now that there is a single profile, I thought I should not lose this and added this in the src/test/resources/hbase-site.xml.. - Devaraj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4953/#review7738 ----------------------------------------------------------- On 2012-05-09 23:03:45, Devaraj Das wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/4953/ bq. ----------------------------------------------------------- bq. bq. (Updated 2012-05-09 23:03:45) bq. bq. bq. Review request for Ted Yu, Michael Stack and Andrew Purtell. bq. bq. bq. Summary bq. ------- bq. bq. Reviewboard request for HBASE-5732 bq. bq. bq. This addresses bug HBASE-5732. bq. https://issues.apache.org/jira/browse/HBASE-5732 bq. bq. bq. Diffs bq. ----- bq. bq. http://svn.apache.org/repos/asf/hbase/trunk/conf/hbase-policy.xml 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/pom.xml 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/AdminProtocol.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/ClientProtocol.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/ConnectionHeader.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/RegionServerStatusProtocol.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/AccessDeniedException.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcClient.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/User.java 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlFilter.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/AccessControlLists.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/AccessControllerProtocol.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/Permission.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/TableAuthManager.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/TablePermission.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/UserPermission.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/access/ZKPermissionWatcher.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationKey.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationProtocol.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationTokenIdentifier.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationTokenSecretManager.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/token/AuthenticationTokenSelector.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/token/TokenProvider.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/token/TokenUtil.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/security/token/ZKSecretWatcher.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/protobuf/RPC.proto 1335370 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessControlFilter.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/security/access/TestTablePermissions.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/security/access/TestZKPermissionsWatcher.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/security/token/TestTokenAuthentication.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/security/token/TestZKSecretWatcher.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/resources/hbase-site.xml 1335370 bq. bq. Diff: https://reviews.apache.org/r/4953/diff bq. bq. bq. Testing bq. ------- bq. bq. All unit tests pass. bq. bq. bq. Thanks, bq. bq. Devaraj bq. bq. > Remove the SecureRPCEngine and merge the security-related logic in the core > engine > ---------------------------------------------------------------------------------- > > Key: HBASE-5732 > URL: https://issues.apache.org/jira/browse/HBASE-5732 > Project: HBase > Issue Type: Improvement > Reporter: Devaraj Das > Assignee: Devaraj Das > Attachments: 5732-rpcengine-merge.7.patch, rpcengine-merge.3.patch, > rpcengine-merge.4.patch, rpcengine-merge.9.patch, rpcengine-merge.patch > > > Remove the SecureRPCEngine and merge the security-related logic in the core > engine. Follow up to HBASE-5727. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira