kgyrtkirk commented on a change in pull request #2770:
URL: https://github.com/apache/hive/pull/2770#discussion_r752457342
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -5484,6 +5509,29 @@ private void fireReadTablePreEvent(String catName,
String dbName, String tblName
}
}
+ /**
+ * Fire a pre-event for read database operation, if there are any
+ * pre-event listeners registered
+ */
+ private void fireReadDatabasePreEvent(final String name)
+ throws MetaException, NoSuchObjectException {
+ if(preListeners.size() > 0) {
+ // do this only if there is a pre event listener registered (avoid
unnecessary
+ // metastore api call)
Review comment:
I think this comment is kinda redundant after `if(preListeners.size() >
0)`
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -3633,21 +3633,46 @@ private Table getTableInternal(GetTableRequest
getTableRequest) throws MetaExcep
@Override
public List<TableMeta> get_table_meta(String dbnames, String tblNames,
List<String> tblTypes)
throws MetaException, NoSuchObjectException {
+
List<TableMeta> t = null;
+ List<TableMeta> finalT = new ArrayList<>();
String[] parsedDbName = parseDbName(dbnames, conf);
startTableFunction("get_table_metas", parsedDbName[CAT_NAME],
parsedDbName[DB_NAME], tblNames);
Exception ex = null;
try {
t = getMS().getTableMeta(parsedDbName[CAT_NAME], parsedDbName[DB_NAME],
tblNames, tblTypes);
t = FilterUtils.filterTableMetasIfEnabled(isServerFilterEnabled,
filterHook,
parsedDbName[CAT_NAME], parsedDbName[DB_NAME], t);
+
+ // metastore authorization : If any listeners are attached then for each
tablemeta check if
+ // the database is readable, if so keep the tablemeta else remove it
from the final list.
+ if(preListeners.size() > 0) {
+ Map<String, Boolean> databaseNames = new HashMap();
+ for (TableMeta tableMeta : t) {
+ String fullDbName = prependCatalogToDbName(parsedDbName[CAT_NAME],
tableMeta.getDbName(), conf);
+ if (databaseNames.get(fullDbName) == null) {
+ boolean isExecptionThrown = false;
+ try {
+ fireReadDatabasePreEvent(fullDbName);
+ } catch (MetaException e) {
+ isExecptionThrown = true;
+ }
+ databaseNames.put(fullDbName, isExecptionThrown);
+ }
+ if (!databaseNames.get(fullDbName)) {
+ finalT.add(tableMeta);
+ }
+ }
+ } else {
+ finalT.addAll(t);
+ }
Review comment:
I think instead of adding this block here ; you could move this content
into a method; and follow the existing
```
t= someFilteringMethod(t, ...)
```
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -5484,6 +5509,29 @@ private void fireReadTablePreEvent(String catName,
String dbName, String tblName
}
}
+ /**
+ * Fire a pre-event for read database operation, if there are any
+ * pre-event listeners registered
+ */
+ private void fireReadDatabasePreEvent(final String name)
+ throws MetaException, NoSuchObjectException {
+ if(preListeners.size() > 0) {
+ // do this only if there is a pre event listener registered (avoid
unnecessary
+ // metastore api call)
+ String[] parsedDbName = parseDbName(name, conf);
+ Database db = null;
+ try {
+ db = get_database_core(parsedDbName[CAT_NAME], parsedDbName[DB_NAME]);
+ if (db == null) {
+ throw new NoSuchObjectException("Database: " + name + " not found");
+ }
+ } catch(MetaException | NoSuchObjectException e) {
+ throw new RuntimeException(e);
Review comment:
you are packaging all exceptions (including `MetaException`) into a
`RuntimeException`
a) is this really neccessary?
b) at the call site the catch is only about `MetaException` which in its
current form will never be thrown
does this work correctly?
could you add a test?
note: I think it might worth to consider to flatten this code into the new
method I suggested to create in the other comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
