[ https://issues.apache.org/jira/browse/HIVE-2720?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13189551#comment-13189551 ]
Phabricator commented on HIVE-2720: ----------------------------------- cwsteinbach has requested changes to the revision "HIVE-2720 [jira] Merge MetaStoreListener and HiveMetaHook interfaces". This also needs some test coverage. In particular I'd like to see testcases that cover the scenario where multiple hooks are registered. INLINE COMMENTS metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaHook.java:64 Right now pre-methods can also modify the input parameters before they are seen by the metastore. I don't think this should be allowed. We can prevent it by only passing the pre-methods copies of the input parameters, but that may be expensive. At the very least I think the javadoc needs to stipulate that pre-methods are *not* allowed to mutate the input parameters. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaHook.java:54 "and for some of the of the operations" metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaHook.java:66 If multiple metahooks are registered, then it's also possible that the first metahook will raise an exception and the later metahooks won't see the event at all. This seems pretty bad from the standpoint of a person trying to write a audit plugin, since they risk not seeing a lot of operations that are failed by hooks that precede them in the list. Also, what happens if the second prehook throws an exception? Will the first posthook still get called with a failure? It's possible that the first prehook created state that will get cleared by the posthook, but this won't happen if the posthook is not called. I think the javadoc either needs to say that you (a) can't assume a posthook will get called just because the prehook was called, or else (b) say that the posthook will always get called if the prehook is called. Option (b) will make it much easier to write hooks at the expense of more complexity on the Hive side. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaHook.java:65 What happens if a post-method throws an exception on an operation that has already been committed to the metastore? What does that exception mean, and how is Hive supposed to handle it? metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java:883 Where's the prehook? I noticed missing prehooks elsewhere as well. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java:1287 This is a hack and is going to be broken by someone who doesn't understand that you're only calling get_database to trigger the hook. Also, the fact that pre/post hooks aren't symmetrical is going to cause a lot of grief for hook implementors. This needs to be fixed. This also introduces a time-of-check-to-time-of-use bug since you're not grabbing the DB and table definitions from within the same txn. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java:1458 Is this a separate bug? Is it being tracked in a different JIRA? metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java:1624 This convention for triggering the hooks is not acceptable. I think code analysis tools will probably flag this since the db and tbl variables are not read. metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java:970 Implementations of what? Not sure if this method belongs here in the first place. metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java:42 What is the plan for removing this interface? Can we remove it on trunk as soon as HCatalog has migrated to the new interface? REVISION DETAIL https://reviews.facebook.net/D1299 > Merge MetaStoreListener and HiveMetaHook interfaces > --------------------------------------------------- > > Key: HIVE-2720 > URL: https://issues.apache.org/jira/browse/HIVE-2720 > Project: Hive > Issue Type: Sub-task > Components: JDBC, Metastore, ODBC, Security > Reporter: Enis Soztutar > Assignee: Enis Soztutar > Attachments: HIVE-2720.D1299.1.patch, HIVE-2720.D1299.2.patch, > HIVE-2720.D1299.3.patch > > > MetaStoreListener and HiveMetaHook both serve as a notification mechanism for > metastore-related events. The former is used by hcat and the latter is by the > hbase-storage handler, and invoked by the client. > I propose to merge these interfaces, and extend the MetaStoreListener, to add > most of the on- and pre- methods at the Thrift interface. This way, extending > metastore will be easier, and validation, storage-driver notification, and > enforcement can be delegated to individual listeners. Besides, more > functionality can be plugged-in by Hcat at this level. -- 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