Re: Review Request 52800: HIVE-13966: DbNotificationListener: can loose DDL operation notifications

2016-11-10 Thread Chaoyu Tang 

---
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

2016-11-10 Thread Mohit Sabharwal 

---
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

2016-11-10 Thread Mohit Sabharwal 

---
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

2016-11-10 Thread Mohit Sabharwal 


> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java, line 
> > 86
> > 
> >
> > 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

2016-11-10 Thread Chaoyu Tang 


> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java, line 
> > 86
> > 
> >
> > 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

2016-11-09 Thread Mohit Sabharwal 

---
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

2016-11-09 Thread Mohit Sabharwal 


> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 790
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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:

Re: Review Request 52800: HIVE-13966: DbNotificationListener: can loose DDL operation notifications

2016-11-08 Thread Chaoyu Tang 

---
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)


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)


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)


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
> 
>