New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HIVE-26246: Filter out results 'show connectors' on HMS client-side #3304
Conversation
Sorry @zhangbutao missed the notification. I am quite busy right now so I am not sure if I will find time to have a look. |
@@ -49,7 +49,7 @@ public AuthorizationMetaStoreFilterHook(Configuration conf) { | |||
public List<String> filterTableNames(String catName, String dbName, List<String> tableList) | |||
throws MetaException { | |||
List<HivePrivilegeObject> listObjs = getHivePrivObjects(dbName, tableList); | |||
return getTableNames(getFilteredObjects(listObjs)); | |||
return getObjectNames(getFilteredObjects(listObjs)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does name of the method: getFilteredObjectNames() makes more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense. fixed. thx
public static List<HivePrivilegeObject> getHivePrivDcObjects(List<String> dcList) { | ||
List<HivePrivilegeObject> objs = new ArrayList<HivePrivilegeObject>(); | ||
for (String dcname : dcList) { | ||
objs.add(new HivePrivilegeObject(HivePrivilegeObjectType.DATACONNECTOR, dcname, dcname)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is ideal to pass HivePrivilegeObjectType.DATACONNECTOR, null, dcname as arguments to HivePrivilegeObject since one of the constructors for HivePrivilegeObject class is HivePrivilegeObjectType, database name, object name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! thx
client.dropDataConnector(DCNAME1, true, true); | ||
client.dropDataConnector(DCNAME2, true, true); | ||
} catch (NoSuchObjectException e) { | ||
// no op |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why we are doing a no-op for this drop operation since we are already passing true as value for ifnotexists argument.
So, I don't think we need to catch nosuchobjectexception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we are dropping dataconnector whch does not exists, this line will throw NoSuchObjectException though passing ifnotexists argument(true):
Line 2045 in cdb1052
connector = getMS().getDataConnector(dcName); |
If this is a bug, i will fix it in another pr :). thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this is a bug and needs fixing. Because
drop connector if exists unexisting_connector;
shouldn't throw an error if the connector doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3350 I have created a pr to fix this. If you have time, please take a look. :)
cc @saihemanth-cloudera @nrg4878
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saihemanth-cloudera Because of #3350 has been merged, i removed the needless catch nosuchobjectexception. Could we continue to merge this pr? Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
@check-spelling-bot Report🔴 Please reviewSee the files view or the action log for details. Unrecognized words (3)api Previously acknowledged words that are now absentaarry timestamplocal yyyyAvailable dictionaries could cover words not in the dictionarycspell:cpp/cpp.txt (104293) covers 81 of them Consider adding them using:
To stop checking additional dictionaries, add:
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the git@github.com:zhangbutao/hive.git repository
If the flagged items do not appear to be textIf items relate to a ...
|
@nrg4878 another pr about connector auth :). please take a look too. thx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good to me. +1
What changes were proposed in this pull request?
Why are the changes needed?
HMS client-side filter out results of 'show connectors' based on authorization, eg. ranger authorization.
Does this PR introduce any user-facing change?
No
How was this patch tested?
UT and local hive cluster with ranger