[
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
