-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59429/#review175608
-----------------------------------------------------------
Seems like a good idea to me!
Thanks for the patch!
Nits from YETUS:
- Checkstyle:
./common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:780:
METASTORE_SCHEMA_INFO_CLASS("hive.metastore.schema.info.class",
"org.apache.hadoop.hive.metastore.MetaStoreSchemaInfo",: warning: Line is
longer than 100 characters (found 123).
./common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:782: +
"schematool to fetch the schema information. This class should implement the
IMetaStoreSchemaInfo interface"),: warning: Line is longer than 100 characters
(found 120).
./metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java:0::
warning: File does not end with a newline.
./metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java:22:public
interface IMetaStoreSchemaInfo {: warning: Missing a Javadoc comment.
./metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java:23:
static String SQL_FILE_EXTENSION = ".sql";:3: warning: Redundant 'static'
modifier.
./metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java:25:
/***: warning: First sentence should end with a period.
./metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java:34:
/***: warning: First sentence should end with a period.
./metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java:43:
/**: warning: First sentence should end with a period.
./metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfo.java:34:public
class MetaStoreSchemaInfo implements IMetaStoreSchemaInfo {: warning: Missing
a Javadoc comment.
./metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfo.java:39:
protected final String dbType;:26: warning: Variable 'dbType' must be private
and have accessor methods.
./metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfo.java:40:
private String hiveSchemaVersions[];:36: warning: Array brackets at illegal
position.
./metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfoFactory.java:28:public
class MetaStoreSchemaInfoFactory {: warning: Missing a Javadoc comment.
./metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfoFactory.java:28:public
class MetaStoreSchemaInfoFactory {:1: warning: Utility classes should not have
a public or default constructor.
./beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java:38:import
org.apache.hadoop.hive.metastore.MetaStoreSchemaInfo;:8: warning: Unused import
- org.apache.hadoop.hive.metastore.MetaStoreSchemaInfo.
- Findbugs:
Found reliance on default encoding in
org.apache.hadoop.hive.metastore.MetaStoreSchemaInfo.loadAllUpgradeScripts(String):
new java.io.FileReader(String)
Bug type DM_DEFAULT_ENCODING (click for details)
In class org.apache.hadoop.hive.metastore.MetaStoreSchemaInfo
In method
org.apache.hadoop.hive.metastore.MetaStoreSchemaInfo.loadAllUpgradeScripts(String)
Called method new java.io.FileReader(String)
At MetaStoreSchemaInfo.java:[line 64]
Do as you see fit with them (I prefer fix them all, but we can have a
discussion about changing the rules on the dev list)
Thanks,
Peter
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java
Lines 27 (patched)
<https://reviews.apache.org/r/59429/#comment248979>
nit: Could you please remove extra white spaces?
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java
Lines 36 (patched)
<https://reviews.apache.org/r/59429/#comment248980>
nit: Could you please remove extra white spaces?
metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java
Lines 45 (patched)
<https://reviews.apache.org/r/59429/#comment248981>
nit: Could you please remove extra white spaces?
metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfo.java
Lines 93 (patched)
<https://reviews.apache.org/r/59429/#comment248982>
Could you please explain why it is changed so? I do not really understand
this change.
metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreSchemaInfo.java
Lines 21 (patched)
<https://reviews.apache.org/r/59429/#comment248984>
nit: Do we need this extra import?
- Peter Vary
On May 21, 2017, 4:51 a.m., Vihang Karajgaonkar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59429/
> -----------------------------------------------------------
>
> (Updated May 21, 2017, 4:51 a.m.)
>
>
> Review request for hive, Naveen Gangam, Sergio Pena, and Sahil Takiar.
>
>
> Bugs: HIVE-16723
> https://issues.apache.org/jira/browse/HIVE-16723
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-16723 : Enable configurable MetaStoreSchemaInfo
>
>
> Diffs
> -----
>
> beeline/src/java/org/apache/hive/beeline/HiveSchemaTool.java
> a45358077a07f09f07dd14eb028fb54b812e8a69
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
> 1c37b6e091cb02f5bf1a6a795712807b50d61424
>
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreVersion.java
> 7188af62acb3cb0305e2cdaf9727576d61418013
> itests/hive-unit/src/test/java/org/apache/hive/beeline/TestSchemaTool.java
> 438a7d6dc86cb291d7d0a9b4501a9df07f1934d7
>
> metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java
> PRE-CREATION
>
> metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfo.java
> 320902bed5fc49323d2b372354fe28d38358459e
>
> metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfoFactory.java
> PRE-CREATION
> metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> b28983f7a9e36832d643c8a2293e19665dd79528
>
> metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreSchemaFactory.java
> PRE-CREATION
>
> metastore/src/test/org/apache/hadoop/hive/metastore/TestMetaStoreSchemaInfo.java
> 71420012756ca26bf399228efdf011597eb70b17
>
>
> Diff: https://reviews.apache.org/r/59429/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vihang Karajgaonkar
>
>
