hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From PRASHANT GOLASH via Review Board <nore...@reviews.apache.org>
Subject Re: Review Request 60950: [HIVE-17117] - Meta listeners are not notified of meta-conf cleanup.
Date Wed, 19 Jul 2017 20:49:20 GMT


> On July 19, 2017, 11:46 a.m., Mohit Sabharwal wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
> > Lines 471 (patched)
> > <https://reviews.apache.org/r/60950/diff/3/?file=1779140#file1779140line473>
> >
> >     Don't think you need this sleep since you are explicitly calling HiveMetaStoreClient#close

It looks close -> thrift#shutdown is async call (inherited in ThriftHiveMetastore via FacebookService.iface),
hence I guess sleep would be necessary.


> On July 19, 2017, 11:46 a.m., Mohit Sabharwal wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
> > Lines 500 (patched)
> > <https://reviews.apache.org/r/60950/diff/3/?file=1779140#file1779140line502>
> >
> >     closing explicitly, so sleep should not be needed.

Same as above.


> On July 19, 2017, 11:46 a.m., Mohit Sabharwal wrote:
> > itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
> > Lines 515 (patched)
> > <https://reviews.apache.org/r/60950/diff/3/?file=1779140#file1779140line517>
> >
> >     same here.

Same as above.


> On July 19, 2017, 11:46 a.m., Mohit Sabharwal wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 298 (patched)
> > <https://reviews.apache.org/r/60950/diff/3/?file=1779141#file1779141line298>
> >
> >     You can set this to null here. And explicitly initialize the thread local in
setMetaConf. That way, you don't need to create HashMap for every thread if setMetaConf is
never called (common case).

Done.


> On July 19, 2017, 11:46 a.m., Mohit Sabharwal wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 395 (patched)
> > <https://reviews.apache.org/r/60950/diff/3/?file=1779141#file1779141line395>
> >
> >     null check needed if you init this to null based on feedback comment.

Done. Did a smaller change to handle unexpected null case of threadLocalConf on the similar
lines.


> On July 19, 2017, 11:46 a.m., Mohit Sabharwal wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
> > Lines 7598 (patched)
> > <https://reviews.apache.org/r/60950/diff/3/?file=1779141#file1779141line7611>
> >
> >     Once you initialize threadLocalModifiedConfig to null, check if threadLocalModifiedConfig
is non null before calling notifyMetaListenersOnShutDown

I am already checking for handler (which gets initialized in along with threadLocalModifiedConfig
in setMetaConf), so not adding additional check.


- PRASHANT


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60950/#review180914
-----------------------------------------------------------


On July 19, 2017, 8:49 p.m., PRASHANT GOLASH wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60950/
> -----------------------------------------------------------
> 
> (Updated July 19, 2017, 8:49 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Added the code to notify meta listeners during shutdown. Shutdown would eventually call
cleanupRawStore (In both cases HMSHandler#close and TServerEventHandler#DeleteContext), so
called the notification code in that function.
> 
> 
> Diffs
> -----
> 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
fd4527e653 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 58b9044930 
> 
> 
> Diff: https://reviews.apache.org/r/60950/diff/4/
> 
> 
> Testing
> -------
> 
> Added unit test cases for the affected codepath.
> 
> 
> Thanks,
> 
> PRASHANT GOLASH
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message