You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Mohit Sabharwal <mo...@cloudera.com> on 2016/11/07 23:25:35 UTC
Review Request 52800: HIVE-13966: DbNotificationListener: can loose
DDL operation notifications
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/
-----------------------------------------------------------
Review request for hive.
Bugs: HIVE-13966
https://issues.apache.org/jira/browse/HIVE-13966
Repository: hive-git
Description
-------
Metadata event and associated notification should be committed in the same
JDO transaction.
Diffs
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 80cd5ada060331797a603848e268c7d2a78a679c
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
Diff: https://reviews.apache.org/r/52800/diff/
Testing
-------
Enhanced TestDbNotificationListener
Thanks,
Mohit Sabharwal
Re: Review Request 52800: HIVE-13966: DbNotificationListener: can
loose DDL operation notifications
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/#review155616
-----------------------------------------------------------
Ship it!
Ship It!
- Chaoyu Tang
On Nov. 10, 2016, 5:40 p.m., Mohit Sabharwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52800/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2016, 5:40 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-13966
> https://issues.apache.org/jira/browse/HIVE-13966
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Metadata event and associated notification should be committed in the same
> JDO transaction.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 7ce29e10798730061d02fadb55423bea3b69c6ac
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
> metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
>
> Diff: https://reviews.apache.org/r/52800/diff/
>
>
> Testing
> -------
>
> Enhanced TestDbNotificationListener
>
>
> Thanks,
>
> Mohit Sabharwal
>
>
Re: Review Request 52800: HIVE-13966: DbNotificationListener: can
loose DDL operation notifications
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/
-----------------------------------------------------------
(Updated Nov. 10, 2016, 5:40 p.m.)
Review request for hive.
Changes
-------
Added Deprecated annotation.
Bugs: HIVE-13966
https://issues.apache.org/jira/browse/HIVE-13966
Repository: hive-git
Description
-------
Metadata event and associated notification should be committed in the same
JDO transaction.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 7ce29e10798730061d02fadb55423bea3b69c6ac
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
Diff: https://reviews.apache.org/r/52800/diff/
Testing
-------
Enhanced TestDbNotificationListener
Thanks,
Mohit Sabharwal
Re: Review Request 52800: HIVE-13966: DbNotificationListener: can
loose DDL operation notifications
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/
-----------------------------------------------------------
(Updated Nov. 10, 2016, 5:34 p.m.)
Review request for hive.
Changes
-------
Incorp. review feedback.
Bugs: HIVE-13966
https://issues.apache.org/jira/browse/HIVE-13966
Repository: hive-git
Description
-------
Metadata event and associated notification should be committed in the same
JDO transaction.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 7ce29e10798730061d02fadb55423bea3b69c6ac
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
Diff: https://reviews.apache.org/r/52800/diff/
Testing
-------
Enhanced TestDbNotificationListener
Thanks,
Mohit Sabharwal
Re: Review Request 52800: HIVE-13966: DbNotificationListener: can
loose DDL operation notifications
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/
-----------------------------------------------------------
(Updated Nov. 10, 2016, 3:43 a.m.)
Review request for hive.
Changes
-------
Incorp. review feedback
Bugs: HIVE-13966
https://issues.apache.org/jira/browse/HIVE-13966
Repository: hive-git
Description
-------
Metadata event and associated notification should be committed in the same
JDO transaction.
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 7ce29e10798730061d02fadb55423bea3b69c6ac
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
Diff: https://reviews.apache.org/r/52800/diff/
Testing
-------
Enhanced TestDbNotificationListener
Thanks,
Mohit Sabharwal
Re: Review Request 52800: HIVE-13966: DbNotificationListener: can
loose DDL operation notifications
Posted by Mohit Sabharwal <mo...@cloudera.com>.
> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java, line 86
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556283#file1556283line86>
> >
> > I wonder if the API changes will cause some backward compatibility issue, if the HiveMetaStore is currently the only consumer of the AlterHandler, it should be fine.
>
> Mohit Sabharwal wrote:
> That's a good point. I have created new versions of these methods. But adding new methods makes it backward compatibility as well, right ? I'm not sure there is a good solution here.
>
> Chaoyu Tang wrote:
> Yeah, that was what I thought, add new APIs and mark the old ones as deprecated. Besides this, I do not have other better solution as well.
Ok, thanks, marking old ones as deprecated.
- Mohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/#review155256
-----------------------------------------------------------
On Nov. 10, 2016, 3:43 a.m., Mohit Sabharwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52800/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2016, 3:43 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-13966
> https://issues.apache.org/jira/browse/HIVE-13966
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Metadata event and associated notification should be committed in the same
> JDO transaction.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 7ce29e10798730061d02fadb55423bea3b69c6ac
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
> metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
>
> Diff: https://reviews.apache.org/r/52800/diff/
>
>
> Testing
> -------
>
> Enhanced TestDbNotificationListener
>
>
> Thanks,
>
> Mohit Sabharwal
>
>
Re: Review Request 52800: HIVE-13966: DbNotificationListener: can
loose DDL operation notifications
Posted by Mohit Sabharwal <mo...@cloudera.com>.
> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 790
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556279#file1556279line790>
> >
> > What kind of events need to be invoked in a separate JDO transactions? or did you mean the events that do not require a JDO transaction?
There are few scenarios where it may make sense to still use separate JDO transactions (and hence the old config), like:
(1) When the listener method invocation is expensive. For example, say an external authorization service has it's own listener implementation
that makes an RPC call to an (external) server. This call may block forever. To avoid rolling back original metadata from HMS db, user may want
to do these in separate transactions -- with the understanding that the two may get inconsistent.
If, on the other hand, the listener is doing a relatively predictable operation like adding notification to a queue or persisting to the
HMS db schema (like DbNotificationListener), it may be reasonable to expect that both happen in the same transaction. This is the motivation
for the new config.
(2) When the notification needs to be logged irrespective of whether the metadata event failed or succeeds. Here the notification acts more
like a audit.
(3) For backward compatibility.
> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 794
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556279#file1556279line794>
> >
> > I wonder why we need a separate transactional event listener in Hive. Can it be deferred to listener implementation? For example, a listener which needs be transactional first checks if it is under a transaction context, joins (or enlists) itself to the transaction if it is. This way might also be applicable to the cases where multiple backends invovle (e.g. distributed transaction). It is just my thought, and may not necessarily be right or feasible.
Any implementation of MetaStoreEventListener interface can be configured so that it's invoked in the same
transaction as the metadata event. This patch does not introduce any separate transactional event listener.
If user wants both the metadata event and notification to happen in the same JDO transaction, they
configure hive.metastore.transactional.event.listeners, otherwise they configure hive.metastore.event.listeners.
This way the interface implementation does not need to be changed.
Only the configuration needs to be changed.
> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java, line 86
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556283#file1556283line86>
> >
> > I wonder if the API changes will cause some backward compatibility issue, if the HiveMetaStore is currently the only consumer of the AlterHandler, it should be fine.
That's a good point. I have created new versions of these methods. But adding new methods makes it backward compatibility as well, right ? I'm not sure there is a good solution here.
- Mohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/#review155256
-----------------------------------------------------------
On Nov. 7, 2016, 11:25 p.m., Mohit Sabharwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52800/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 11:25 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-13966
> https://issues.apache.org/jira/browse/HIVE-13966
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Metadata event and associated notification should be committed in the same
> JDO transaction.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 80cd5ada060331797a603848e268c7d2a78a679c
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
> metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
>
> Diff: https://reviews.apache.org/r/52800/diff/
>
>
> Testing
> -------
>
> Enhanced TestDbNotificationListener
>
>
> Thanks,
>
> Mohit Sabharwal
>
>
Re: Review Request 52800: HIVE-13966: DbNotificationListener: can
loose DDL operation notifications
Posted by Chaoyu Tang <ct...@gmail.com>.
> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java, line 86
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556283#file1556283line86>
> >
> > I wonder if the API changes will cause some backward compatibility issue, if the HiveMetaStore is currently the only consumer of the AlterHandler, it should be fine.
>
> Mohit Sabharwal wrote:
> That's a good point. I have created new versions of these methods. But adding new methods makes it backward compatibility as well, right ? I'm not sure there is a good solution here.
Yeah, that was what I thought, add new APIs and mark the old ones as deprecated. Besides this, I do not have other better solution as well.
- Chaoyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/#review155256
-----------------------------------------------------------
On Nov. 10, 2016, 3:43 a.m., Mohit Sabharwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52800/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2016, 3:43 a.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-13966
> https://issues.apache.org/jira/browse/HIVE-13966
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Metadata event and associated notification should be committed in the same
> JDO transaction.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 7ce29e10798730061d02fadb55423bea3b69c6ac
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
> metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
>
> Diff: https://reviews.apache.org/r/52800/diff/
>
>
> Testing
> -------
>
> Enhanced TestDbNotificationListener
>
>
> Thanks,
>
> Mohit Sabharwal
>
>
Re: Review Request 52800: HIVE-13966: DbNotificationListener: can
loose DDL operation notifications
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52800/#review155256
-----------------------------------------------------------
The patch looks good, however, I have a couple of questions.
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 764)
<https://reviews.apache.org/r/52800/#comment225169>
What kind of events need to be invoked in a separate JDO transactions? or did you mean the events that do not require a JDO transaction?
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 768)
<https://reviews.apache.org/r/52800/#comment225171>
I wonder why we need a separate transactional event listener in Hive. Can it be deferred to listener implementation? For example, a listener which needs be transactional first checks if it is under a transaction context, joins (or enlists) itself to the transaction if it is. This way might also be applicable to the cases where multiple backends invovle (e.g. distributed transaction). It is just my thought, and may not necessarily be right or feasible.
metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java (line 86)
<https://reviews.apache.org/r/52800/#comment225118>
I wonder if the API changes will cause some backward compatibility issue, if the HiveMetaStore is currently the only consumer of the AlterHandler, it should be fine.
- Chaoyu Tang
On Nov. 7, 2016, 11:25 p.m., Mohit Sabharwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52800/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2016, 11:25 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-13966
> https://issues.apache.org/jira/browse/HIVE-13966
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Metadata event and associated notification should be committed in the same
> JDO transaction.
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 80cd5ada060331797a603848e268c7d2a78a679c
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java PRE-CREATION
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java af16f75e63c372c37bfd73567b777bba53f94db3
> metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java dedd4497adfcc9d57090a943f6bb4f35ea87fa61
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 40b337a9e40ea04a37f108146853d2d1f42dcd29
> metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 60e462fd06a3f84d5b87cd335afb49768cb27562
>
> Diff: https://reviews.apache.org/r/52800/diff/
>
>
> Testing
> -------
>
> Enhanced TestDbNotificationListener
>
>
> Thanks,
>
> Mohit Sabharwal
>
>