> On July 19, 2017, 3:21 a.m., Mohit Sabharwal wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 283 (patched) > > <https://reviews.apache.org/r/60950/diff/2/?file=1778809#file1778809line283> > > > > is this really needed ? see comment below.
For same reasons as mentioned below. > On July 19, 2017, 3:21 a.m., Mohit Sabharwal wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 393 (patched) > > <https://reviews.apache.org/r/60950/diff/2/?file=1778809#file1778809line393> > > > > If you make this static, I think you can do away with the need of > > creating a thread local HMSHandler (threadLocalHMSHandler), i.e. You > > already have a thread local map (threadLocalModifiedMetaConfKeys), so you > > should not need to reference it using another thread local > > (threadLocalHMSHandler). HMSHandler object is required to be passed in ConfigChangeEvent constructor in notifyMetaListeners. DeleteContext is invoked on TServerEventHandler object where I don't have any access to HMSHandler object, hence I saved the invoking HMSHandler on threadLocal. > On July 19, 2017, 3:21 a.m., Mohit Sabharwal wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 396 (patched) > > <https://reviews.apache.org/r/60950/diff/2/?file=1778809#file1778809line396> > > > > Please rename m to something more clear (in this context), like > > modifiedConfig. Done. > On July 19, 2017, 3:21 a.m., Mohit Sabharwal wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 403 (patched) > > <https://reviews.apache.org/r/60950/diff/2/?file=1778809#file1778809line403> > > > > nit: just use threadLocalConf.get() directly, get rid of the conf above Done. > On July 19, 2017, 3:21 a.m., Mohit Sabharwal wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > > Lines 412 (patched) > > <https://reviews.apache.org/r/60950/diff/2/?file=1778809#file1778809line412> > > > > For clarity, move this to finally() inside cleanupRawStore(), so that > > all thread local cleanup is captured in one place as > > HMSHandler.threadLocalModifiedMetaConfKeys.remove(). Done. - PRASHANT ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60950/#review180897 ----------------------------------------------------------- On July 19, 2017, 7:40 a.m., PRASHANT GOLASH wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60950/ > ----------------------------------------------------------- > > (Updated July 19, 2017, 7:40 a.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/3/ > > > Testing > ------- > > Added unit test cases for the affected codepath. > > > Thanks, > > PRASHANT GOLASH > >
