Re: Review Request 59429: HIVE-16723 : Enable configurable MetaStoreSchemaInfo

2017-05-22 Thread Vihang Karajgaonkar 

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

(Updated May 22, 2017, 10:13 p.m.)


Review request for hive, Naveen Gangam, Sergio Pena, and Sahil Takiar.


Changes
---

addressed review comments


Bugs: HIVE-16723
https://issues.apache.org/jira/browse/HIVE-16723


Repository: hive-git


Description
---

HIVE-16723 : Enable configurable MetaStoreSchemaInfo


Diffs (updated)
-

  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/3/

Changes: https://reviews.apache.org/r/59429/diff/2-3/


Testing
---


Thanks,

Vihang Karajgaonkar

Re: Review Request 59429: HIVE-16723 : Enable configurable MetaStoreSchemaInfo

2017-05-22 Thread Naveen Gangam 


> On May 22, 2017, 8:30 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java
> > Lines 27 (patched)
> > 
> >
> > Can you add javadocs for the rest of these methods.
> > 
> > Since this is an interface can you all add target audience annotations.

+1


> On May 22, 2017, 8:30 p.m., Sahil Takiar wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfoFactory.java
> > Lines 59 (patched)
> > 
> >
> > Should also document that this constructor is necessary

good point. Can we add a testcase to ensure that the class has such constructor?


- Naveen


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


On May 22, 2017, 4:03 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59429/
> ---
> 
> (Updated May 22, 2017, 4:03 p.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/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 59429: HIVE-16723 : Enable configurable MetaStoreSchemaInfo

2017-05-22 Thread Sahil Takiar 

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


Fix it, then Ship it!





metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java
Lines 27 (patched)


Can you add javadocs for the rest of these methods.

Since this is an interface can you all add target audience annotations.



metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfoFactory.java
Lines 59 (patched)


Should also document that this constructor is necessary


- Sahil Takiar


On May 22, 2017, 4:03 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59429/
> ---
> 
> (Updated May 22, 2017, 4:03 p.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/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 59429: HIVE-16723 : Enable configurable MetaStoreSchemaInfo

2017-05-22 Thread Vihang Karajgaonkar 

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

(Updated May 22, 2017, 4:03 p.m.)


Review request for hive, Naveen Gangam, Sergio Pena, and Sahil Takiar.


Changes
---

Added suggestions by Peter


Bugs: HIVE-16723
https://issues.apache.org/jira/browse/HIVE-16723


Repository: hive-git


Description
---

HIVE-16723 : Enable configurable MetaStoreSchemaInfo


Diffs (updated)
-

  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/2/

Changes: https://reviews.apache.org/r/59429/diff/1-2/


Testing
---


Thanks,

Vihang Karajgaonkar

Re: Review Request 59429: HIVE-16723 : Enable configurable MetaStoreSchemaInfo

2017-05-22 Thread Peter Vary 

---
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)


nit: Could you please remove extra white spaces?



metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java
Lines 36 (patched)


nit: Could you please remove extra white spaces?



metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreSchemaInfo.java
Lines 45 (patched)


nit: Could you please remove extra white spaces?



metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreSchemaInfo.java
Lines 93 (patched)


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)


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
> 
> 
>

Review Request 59429: HIVE-16723 : Enable configurable MetaStoreSchemaInfo

2017-05-20 Thread Vihang Karajgaonkar 

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

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