----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48233/#review137434 -----------------------------------------------------------
LGTM, but couple comments regarding breaking backward compatibility. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 3179) <https://reviews.apache.org/r/48233/#comment202558> Since we are moving the functionality from driver to HMS, should we deprecate hive.limit.query.max.table.partition and introduce a new config called hive.metastore.retrieve.max.partitions ? All metastore configs have "hive.metastore" prefix. Otherwise: 1) The change is backward incompatible for existing users that are setting this config at HS2 level and are now expected to set it at HMS level to get the same functionality. 2) Name would be confusing. We could do the following: 1) Mark hive.limit.query.max.table.partition as deprecated in HiveConf and suggest that user move to hive.metastore.retrieve.max.partitions 2) Do not remove the functionality associated with hive.limit.query.max.table.partition in PartitionPruner. It does do what the description promises - i.e. fail the query before execution stage if number of partitions associated with any scan operator exceed configured value. 3) Add new config hive.metastore.retrieve.max.partitions to configure functionality in this patch. Makes sense ? metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java (line 2524) <https://reviews.apache.org/r/48233/#comment202560> As Brock suggested, it's worth figuring out the latency impact of this select count query - which will be issued for every getPartitions request. - Mohit Sabharwal On June 13, 2016, 6:28 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48233/ > ----------------------------------------------------------- > > (Updated June 13, 2016, 6:28 p.m.) > > > Review request for hive, Mohit Sabharwal and Naveen Gangam. > > > Bugs: HIVE-13884 > https://issues.apache.org/jira/browse/HIVE-13884 > > > Repository: hive-git > > > Description > ------- > > The patch verifies the # of partitions a table has before fetching any from > the metastore. I > t checks that limit from 'hive.limit.query.max.table.partition'. > > A limitation added here is that the variable must be on hive-site.xml in > order to work, and it does not accept to set this through beeline because > HiveMetaStore.java does not read the variables set through beeline. I think > it is better to keep it this way to avoid users changing the value on fly, > and crashing the metastore. > > Another change is that EXPLAIN commands won't be executed either. EXPLAIN > commands need to fetch partitions in order to create the operator tree. If we > allow EXPLAIN to do that, then we may have the same OOM situations for large > partitions. > > > Diffs > ----- > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > c0827ea9d47e569d9697649a7e16d196de3de14d > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java > c135179b97354108f842a5ca2de0c6f0ef28b7fc > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > f98de1326956b19b9d28fc9b1fcdede8d851180d > metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java > a6d3f5385b33b8a4e31ee20ca5cb8f58c97c8702 > metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java > 31f0d7b89670b8a749bbe8a7ff2b4ff9f059a8e2 > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java > 3152e77c3c7152ac4dbe7e779ce35f28044fe3c9 > > metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java > 86a243609b23e2ca9bb8849f0da863a95e477d5c > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > cd3c86064df3e7febcc16e03aab6ce407e0dc8a0 > > Diff: https://reviews.apache.org/r/48233/diff/ > > > Testing > ------- > > Waiting for HiveQA. > > > Thanks, > > Sergio Pena > >
