> On 2011-01-25 20:44:24, Carl Steinbach wrote: > >
Hi Carl, thanks for the review. > On 2011-01-25 20:44:24, Carl Steinbach wrote: > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, > > line 236 > > <https://reviews.apache.org/r/343/diff/2/?file=10183#file10183line236> > > > > According to javadoc PersistenceManagerFactory.getDataStoreCache() > > never returns null, but if it does then we just silently skip this setup. I > > think we should remove this null check. Since we call dsc.pinAll() a few lines later, it will throw NPE if the JDO implementation is not fully compliant with the doc. What do you think if we keep the check and add an else block with a WARN level log entry? > On 2011-01-25 20:44:24, Carl Steinbach wrote: > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java, > > line 244 > > <https://reviews.apache.org/r/343/diff/2/?file=10183#file10183line244> > > > > If the user has specified a classname that is not found in the > > PINCLASSMAP then we just silently ignore it, which is not a good idea. > > > > Please add some INFO level logging statements a) echo the > > METASTORE_CACHE_PINOBJTYPES value and indicate that this is being used to > > the set the pin list, and b) log a message if the list contains a name that > > is not found in the PINCLASSMAP. > > > > I also want to suggest that replace the PINCLASSMAP with a list of the > > actual classnames, and require METASTORE_CACHE_PINOBJTYPES to specify the > > actual classname (e.g. MSerDeInfo, MTable, etc), instead of a name that you > > transform and then map to the real name. - I will update the patch to include the logging as you suggested. - The reason why I choose to use logical names is to provide some isolation so we wont break user's config if we ever want to refactor. What are the benefits of using the actual class names for this config? - Mac ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/343/#review156 ----------------------------------------------------------- On 2011-01-24 11:50:46, Mac Yang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/343/ > ----------------------------------------------------------- > > (Updated 2011-01-24 11:50:46) > > > Review request for hive. > > > Summary > ------- > > Review for HIVE-1910 > > > This addresses bug HIVE-1910. > https://issues.apache.org/jira/browse/HIVE-1910 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java > 1062921 > http://svn.apache.org/repos/asf/hive/trunk/conf/hive-default.xml 1062921 > > http://svn.apache.org/repos/asf/hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 1062921 > > Diff: https://reviews.apache.org/r/343/diff > > > Testing > ------- > > > Thanks, > > Mac > >
