Review Board 1.7.22


HIVE-78 patch 9

Review Request #183 - Created Dec. 17, 2010 and updated

John Sichi
trunk
Reviewers
hive
hive
Review by JVS.  Note that the patch had some conflicts, so I wasn't able to test this version; I'll do more testing and commenting after the next patch.

 
Posted (Dec. 17, 2010, 12:52 p.m.)

   

  
From these parameter names, it's not obvious that they are default grants.
This interface has been renamed.
Why is your patch adding this irrelevant property?  It seems to have sneaked in from some other patch.
This should be granteeName
This should be granteeType
Do you mean properties associated with the role?
Shouldn't this take a Role object so we can pass parameters?
Fix whitespace on these two lines
Since we're not using this call yet, eliminate it from the patch.
You could eliminate even more code by pushing down the type-specific dispatch below the executeWithRetry level.
See comment above about eliminating more code by pushing down below executeWithRetry.
You duplicated the same code here three times instead of using a helper method.
Why is this using RuntimeException?
ditto on RuntimeException
A redundant privilege should be ignored; it should not cause an exception.  (If it has grant option and the old one didn't, then the privilege should be upgraded to have the grant option.)
There should be a unique index on ROLE_NAME
IS_ROLE/IS_GROUP no longer exists; this should be PRINCIPAL_TYPE.  Also, where is the grantor?
rename to ROLE_GRANT_ID
Add a unique index:  PRINCIPAL_TYPE, PRINCIPAL_NAME, USER_PRIV, GRANTOR
rename to USER_GRANT_ID
Since now we're only storing one privilege per row, this and others should be 128 instead of 4000
This index should be unique, and it should include PRINCIPAL_TYPE, DB_PRIV, and GRANTOR
rename to DB_GRANT_ID
need a unique index
rename to TABLEPART_GRANT_ID
rename to COLUMN_GRANT_ID
Need a unique index
As I noted in JIRA previously, it does not make sense to have doAuthorization be a boolean method which also throws an exception.  And here we disregard the boolean return anyway.
Please don't include all of these spurious reformattings in your patch.
As previously noted in JIRA, call this HiveAuthenticationProvider and make it extend Configurable.  And add Javadoc.
As previously noted in JIRA, get rid of this unnecessary factory class and follow the classloading pattern in HiveUtils instead.
Per my previous comments, please remove this unneeded factory class and follow the classloading pattern in HiveUtils.
As noted previously in JIRA, this should implement Configurable.
For all methods here, see my comment in Driver about the inconsistency between returning boolean and throwing.

Also avoid using arrays in interfaces unless necessary for performance; use List instead.