You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Navis Ryu <na...@nexr.com> on 2014/07/31 03:34:36 UTC

Review Request 24137: allow disabling direct sql per query with external metastore

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

Review request for hive.


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


Repository: hive-git


Description
-------

Currently with external metastore, direct sql can only be disabled via metastore config globally. Perhaps it makes sense to have the ability to propagate the setting per query from client to override the metastore setting, e.g. if one particular query causes it to fail.


Diffs
-----

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3bfc681 
  common/src/java/org/apache/hadoop/hive/conf/SystemVariables.java ee98d17 
  itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java 9e416b5 
  metastore/if/hive_metastore.thrift 55f41db 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 5cc1cd8 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java d26183b 
  metastore/src/java/org/apache/hadoop/hive/metastore/IHMSHandler.java 1675751 
  metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 5add436 
  metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java c28c46a 
  metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java 1cf09d4 
  metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java 86172b9 
  metastore/src/java/org/apache/hadoop/hive/metastore/events/ConfigChangeEvent.java PRE-CREATION 

Diff: https://reviews.apache.org/r/24137/diff/


Testing
-------


Thanks,

Navis Ryu

Re: Review Request 24137: allow disabling direct sql per query with external metastore

Posted by Navis Ryu <na...@nexr.com>.

> On Aug. 1, 2014, 9:35 a.m., Lars Francke wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 71
> > <https://reviews.apache.org/r/24137/diff/1/?file=646536#file646536line71>
> >
> >     Should be named `META_CONFS`

There are fields something like "vars" and "metaVars", which is static. I also think uppercase names are better for static fields, but also feel that consistency is more important.


> On Aug. 1, 2014, 9:35 a.m., Lars Francke wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 161
> > <https://reviews.apache.org/r/24137/diff/1/?file=646536#file646536line161>
> >
> >     Should be named `META_CONF_VARS`
> >     
> >     Also all the `HiveConf` qualifiers are not necessary
> >     
> >     Indentation is 2 not 4 as per the Wiki

Same as above.


> On Aug. 1, 2014, 9:35 a.m., Lars Francke wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java, line 63
> > <https://reviews.apache.org/r/24137/diff/1/?file=646546#file646546line63>
> >
> >     no need to call getConf() here

To make that same form with other invocations in the class. Just for consistency.


- Navis


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


On July 31, 2014, 1:34 a.m., Navis Ryu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24137/
> -----------------------------------------------------------
> 
> (Updated July 31, 2014, 1:34 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-7532
>     https://issues.apache.org/jira/browse/HIVE-7532
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Currently with external metastore, direct sql can only be disabled via metastore config globally. Perhaps it makes sense to have the ability to propagate the setting per query from client to override the metastore setting, e.g. if one particular query causes it to fail.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3bfc681 
>   common/src/java/org/apache/hadoop/hive/conf/SystemVariables.java ee98d17 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java 9e416b5 
>   metastore/if/hive_metastore.thrift 55f41db 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 5cc1cd8 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java d26183b 
>   metastore/src/java/org/apache/hadoop/hive/metastore/IHMSHandler.java 1675751 
>   metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 5add436 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java c28c46a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java 1cf09d4 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java 86172b9 
>   metastore/src/java/org/apache/hadoop/hive/metastore/events/ConfigChangeEvent.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24137/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Navis Ryu
> 
>

Re: Review Request 24137: allow disabling direct sql per query with external metastore

Posted by Lars Francke <as...@lars-francke.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24137/#review49330
-----------------------------------------------------------


Code style issues only


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/24137/#comment86318>

    Should be named `META_CONFS`



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/24137/#comment86319>

    Should be named `META_CONF_VARS`
    
    Also all the `HiveConf` qualifiers are not necessary
    
    Indentation is 2 not 4 as per the Wiki



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/24137/#comment86320>

    Line too long



metastore/src/java/org/apache/hadoop/hive/metastore/IHMSHandler.java
<https://reviews.apache.org/r/24137/#comment86321>

    extra space between extends and ThriftHiveMetastore.Iface



metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
<https://reviews.apache.org/r/24137/#comment86322>

    `public` is not needed in interfaces



metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java
<https://reviews.apache.org/r/24137/#comment86317>

    Javadoc is wrong



metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
<https://reviews.apache.org/r/24137/#comment86323>

    no need to call getConf() here



metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java
<https://reviews.apache.org/r/24137/#comment86324>

    line too long



metastore/src/java/org/apache/hadoop/hive/metastore/events/ConfigChangeEvent.java
<https://reviews.apache.org/r/24137/#comment86325>

    line too long


- Lars Francke


On July 31, 2014, 1:34 a.m., Navis Ryu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24137/
> -----------------------------------------------------------
> 
> (Updated July 31, 2014, 1:34 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-7532
>     https://issues.apache.org/jira/browse/HIVE-7532
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Currently with external metastore, direct sql can only be disabled via metastore config globally. Perhaps it makes sense to have the ability to propagate the setting per query from client to override the metastore setting, e.g. if one particular query causes it to fail.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3bfc681 
>   common/src/java/org/apache/hadoop/hive/conf/SystemVariables.java ee98d17 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java 9e416b5 
>   metastore/if/hive_metastore.thrift 55f41db 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 5cc1cd8 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java d26183b 
>   metastore/src/java/org/apache/hadoop/hive/metastore/IHMSHandler.java 1675751 
>   metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 5add436 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java c28c46a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java 1cf09d4 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java 86172b9 
>   metastore/src/java/org/apache/hadoop/hive/metastore/events/ConfigChangeEvent.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24137/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Navis Ryu
> 
>

Re: Review Request 24137: allow disabling direct sql per query with external metastore

Posted by Navis Ryu <na...@nexr.com>.

> On July 31, 2014, 9:01 p.m., Sergey Shelukhin wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line 395
> > <https://reviews.apache.org/r/24137/diff/1/?file=646540#file646540line395>
> >
> >     would it be enough to make it package-visible?

It's on IHMSHandler, which is a public interface. 


> On July 31, 2014, 9:01 p.m., Sergey Shelukhin wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line 270
> > <https://reviews.apache.org/r/24137/diff/1/?file=646540#file646540line270>
> >
> >     why are these made static? I see that there can be multiple HMSHandler-s, incl. thru derived classes. Are they always one per thread? If multiple handlers (incl. derived ones), semantics can change

I've been misunderstood till now that the HMSHandler is created per client like hiveservers. Cannot imagine there would be a case that a client is using multiple processors simultaneously but I've understood your point. Will be rolled-back.


- Navis


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


On July 31, 2014, 1:34 a.m., Navis Ryu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24137/
> -----------------------------------------------------------
> 
> (Updated July 31, 2014, 1:34 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-7532
>     https://issues.apache.org/jira/browse/HIVE-7532
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Currently with external metastore, direct sql can only be disabled via metastore config globally. Perhaps it makes sense to have the ability to propagate the setting per query from client to override the metastore setting, e.g. if one particular query causes it to fail.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3bfc681 
>   common/src/java/org/apache/hadoop/hive/conf/SystemVariables.java ee98d17 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java 9e416b5 
>   metastore/if/hive_metastore.thrift 55f41db 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 5cc1cd8 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java d26183b 
>   metastore/src/java/org/apache/hadoop/hive/metastore/IHMSHandler.java 1675751 
>   metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 5add436 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java c28c46a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java 1cf09d4 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java 86172b9 
>   metastore/src/java/org/apache/hadoop/hive/metastore/events/ConfigChangeEvent.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24137/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Navis Ryu
> 
>

Re: Review Request 24137: allow disabling direct sql per query with external metastore

Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24137/#review49290
-----------------------------------------------------------



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/24137/#comment86238>

    why are these made static? I see that there can be multiple HMSHandler-s, incl. thru derived classes. Are they always one per thread? If multiple handlers (incl. derived ones), semantics can change



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
<https://reviews.apache.org/r/24137/#comment86240>

    would it be enough to make it package-visible?


- Sergey Shelukhin


On July 31, 2014, 1:34 a.m., Navis Ryu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24137/
> -----------------------------------------------------------
> 
> (Updated July 31, 2014, 1:34 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-7532
>     https://issues.apache.org/jira/browse/HIVE-7532
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Currently with external metastore, direct sql can only be disabled via metastore config globally. Perhaps it makes sense to have the ability to propagate the setting per query from client to override the metastore setting, e.g. if one particular query causes it to fail.
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 3bfc681 
>   common/src/java/org/apache/hadoop/hive/conf/SystemVariables.java ee98d17 
>   itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java 9e416b5 
>   metastore/if/hive_metastore.thrift 55f41db 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 5cc1cd8 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java d26183b 
>   metastore/src/java/org/apache/hadoop/hive/metastore/IHMSHandler.java 1675751 
>   metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 5add436 
>   metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java c28c46a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStoreProxy.java 1cf09d4 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java 86172b9 
>   metastore/src/java/org/apache/hadoop/hive/metastore/events/ConfigChangeEvent.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24137/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Navis Ryu
> 
>

Re: Review Request 24137: allow disabling direct sql per query with external metastore

Posted by Navis Ryu <na...@nexr.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24137/
-----------------------------------------------------------

(Updated Aug. 11, 2014, 7:37 a.m.)


Review request for hive.


Changes
-------

added getMetaConf(), which shows current value of the meta variable.


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


Repository: hive-git


Description
-------

Currently with external metastore, direct sql can only be disabled via metastore config globally. Perhaps it makes sense to have the ability to propagate the setting per query from client to override the metastore setting, e.g. if one particular query causes it to fail.


Diffs (updated)
-----

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 8490558 
  common/src/java/org/apache/hadoop/hive/conf/SystemVariables.java ee98d17 
  itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java 9e416b5 
  metastore/if/hive_metastore.thrift 9e93b95 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 6e689d0 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 85a77d9 
  metastore/src/java/org/apache/hadoop/hive/metastore/IHMSHandler.java 1675751 
  metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java 8746c37 
  metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java c28c46a 
  metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java 86172b9 
  metastore/src/java/org/apache/hadoop/hive/metastore/events/ConfigChangeEvent.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 39b032e 
  ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java 2baa24a 
  ql/src/test/queries/clientpositive/set_metaconf.q PRE-CREATION 
  ql/src/test/results/clientpositive/set_metaconf.q.out PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionBase.java 4c3164e 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java b39d64d 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java c2f0495 

Diff: https://reviews.apache.org/r/24137/diff/


Testing
-------


Thanks,

Navis Ryu

Re: Review Request 24137: allow disabling direct sql per query with external metastore

Posted by Navis Ryu <na...@nexr.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24137/
-----------------------------------------------------------

(Updated Aug. 4, 2014, 2:12 a.m.)


Review request for hive.


Changes
-------

Addressed comments


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


Repository: hive-git


Description
-------

Currently with external metastore, direct sql can only be disabled via metastore config globally. Perhaps it makes sense to have the ability to propagate the setting per query from client to override the metastore setting, e.g. if one particular query causes it to fail.


Diffs (updated)
-----

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 15bc0a3 
  common/src/java/org/apache/hadoop/hive/conf/SystemVariables.java ee98d17 
  itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java 9e416b5 
  metastore/if/hive_metastore.thrift 55f41db 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java b74868b 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 4c9a597 
  metastore/src/java/org/apache/hadoop/hive/metastore/IHMSHandler.java 1675751 
  metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java d6e849f 
  metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java c28c46a 
  metastore/src/java/org/apache/hadoop/hive/metastore/RetryingHMSHandler.java 86172b9 
  metastore/src/java/org/apache/hadoop/hive/metastore/events/ConfigChangeEvent.java PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java a7e50ad 
  ql/src/java/org/apache/hadoop/hive/ql/processors/SetProcessor.java 2baa24a 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionBase.java 4c3164e 
  service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java b39d64d 
  service/src/java/org/apache/hive/service/cli/session/SessionManager.java c2f0495 

Diff: https://reviews.apache.org/r/24137/diff/


Testing
-------


Thanks,

Navis Ryu