[jira] [Created] (HIVE-18559) Allow hive.metastore.limit.partition.request to be user configurable

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-18559:
--

 Summary: Allow hive.metastore.limit.partition.request to be user 
configurable
 Key: HIVE-18559
 URL: https://issues.apache.org/jira/browse/HIVE-18559
 Project: Hive
  Issue Type: Bug
  Components: Metastore
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


HIVE-13884 introduced hive.metastore.limit.partition.request to limit the 
number of partitions that can be requested from the Metastore for a given table.

This config is set by cluster admins to prevent ad-hoc queries from putting 
memory pressure on HMS. But the limit can be too restrictive for some (savvy) 
users who require the ability to set a higher limit for a subset of workloads 
(during low HMS memory pressure, for example). Such users generally instruct 
admins not to set this confg.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Review Request 61244: HIVE-15305: Add tests for METASTORE_EVENT_LISTENERS

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

HIVE-15232 reused TestDbNotificationListener to test 
METASTORE_TRANSACTIONAL_EVENT_LISTENERS and removed unit testing of 
METASTORE_EVENT_LISTENERS config. We should test both.


Diffs
-

  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
 351546c3bc9edb69a435f04795b5ea6c3421f5b0 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 35989f5b09d016ebde1c9953893f145cae414e42 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
 c36b632fd17791f00c9b69247a520a1bed22a337 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestNonTransactionalDbNotificationListener.java
 PRE-CREATION 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestTransactionalDbNotificationListener.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/61244/diff/1/


Testing
---


Thanks,

Mohit Sabharwal

Re: Review Request 60950: [HIVE-17117] - Meta listeners are not notified of meta-conf cleanup.

[Mohit Sabharwal]

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


Ship it!




Ship It!

- Mohit Sabharwal


On July 20, 2017, 12:39 a.m., PRASHANT GOLASH wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60950/
> ---
> 
> (Updated July 20, 2017, 12:39 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Added the code to notify meta listeners during shutdown. Shutdown would 
> eventually call cleanupRawStore (In both cases HMSHandler#close and 
> TServerEventHandler#DeleteContext), so called the notification code in that 
> function.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
>  fd4527e653 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 58b9044930 
> 
> 
> Diff: https://reviews.apache.org/r/60950/diff/5/
> 
> 
> Testing
> ---
> 
> Added unit test cases for the affected codepath.
> 
> 
> Thanks,
> 
> PRASHANT GOLASH
> 
>

Re: Review Request 60950: [HIVE-17117] - Meta listeners are not notified of meta-conf cleanup.

[Mohit Sabharwal]

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



Thanks, couple more items.


itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
Lines 471 (patched)
<https://reviews.apache.org/r/60950/#comment256257>

Don't think you need this sleep since you are explicitly calling 
HiveMetaStoreClient#close



itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
Lines 500 (patched)
<https://reviews.apache.org/r/60950/#comment256258>

closing explicitly, so sleep should not be needed.



itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
Lines 515 (patched)
<https://reviews.apache.org/r/60950/#comment256262>

same here.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 298 (patched)
<https://reviews.apache.org/r/60950/#comment256259>

You can set this to null here. And explicitly initialize the thread local 
in setMetaConf. That way, you don't need to create HashMap for every thread if 
setMetaConf is never called (common case).



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 395 (patched)
<https://reviews.apache.org/r/60950/#comment256261>

null check needed if you init this to null based on feedback comment.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 7598 (patched)
<https://reviews.apache.org/r/60950/#comment256260>

Once you initialize threadLocalModifiedConfig to null, check if 
threadLocalModifiedConfig is non null before calling 
notifyMetaListenersOnShutDown


- Mohit Sabharwal


On July 19, 2017, 7:40 a.m., PRASHANT GOLASH wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60950/
> ---
> 
> (Updated July 19, 2017, 7:40 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Added the code to notify meta listeners during shutdown. Shutdown would 
> eventually call cleanupRawStore (In both cases HMSHandler#close and 
> TServerEventHandler#DeleteContext), so called the notification code in that 
> function.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
>  fd4527e653 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 58b9044930 
> 
> 
> Diff: https://reviews.apache.org/r/60950/diff/3/
> 
> 
> Testing
> ---
> 
> Added unit test cases for the affected codepath.
> 
> 
> Thanks,
> 
> PRASHANT GOLASH
> 
>

Re: Review Request 60950: [HIVE-17117] - Meta listeners are not notified of meta-conf cleanup.

[Mohit Sabharwal]

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




metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 283 (patched)
<https://reviews.apache.org/r/60950/#comment256218>

is this really needed ? see comment below.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 393 (patched)
<https://reviews.apache.org/r/60950/#comment256214>

If you make this static, I think you can do away with the need of creating 
a thread local HMSHandler (threadLocalHMSHandler), i.e. You already have a 
thread local map (threadLocalModifiedMetaConfKeys), so you should not need to 
reference it using another thread local (threadLocalHMSHandler).



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 396 (patched)
<https://reviews.apache.org/r/60950/#comment256215>

Please rename m to something more clear (in this context), like 
modifiedConfig.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 403 (patched)
<https://reviews.apache.org/r/60950/#comment256216>

nit: just use threadLocalConf.get() directly, get rid of the conf above



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 412 (patched)
<https://reviews.apache.org/r/60950/#comment256217>

For clarity, move this to finally() inside cleanupRawStore(), so that all 
thread local cleanup is captured in one place as 
HMSHandler.threadLocalModifiedMetaConfKeys.remove().


- Mohit Sabharwal


On July 18, 2017, 10:58 p.m., PRASHANT GOLASH wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60950/
> ---
> 
> (Updated July 18, 2017, 10:58 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Added the code to notify meta listeners during shutdown. Shutdown would 
> eventually call cleanupRawStore (In both cases HMSHandler#close and 
> TServerEventHandler#DeleteContext), so called the notification code in that 
> function.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
>  fd4527e653 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 58b9044930 
> 
> 
> Diff: https://reviews.apache.org/r/60950/diff/2/
> 
> 
> Testing
> ---
> 
> Added unit test cases for the affected codepath.
> 
> 
> Thanks,
> 
> PRASHANT GOLASH
> 
>

[jira] [Created] (HIVE-17022) Add mode in lock debug statements

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-17022:
--

 Summary: Add mode in lock debug statements
 Key: HIVE-17022
 URL: https://issues.apache.org/jira/browse/HIVE-17022
 Project: Hive
  Issue Type: Improvement
  Components: Locking
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal
Priority: Trivial


Currently, lock debug statements print IMPLICIT/EXPLICIT as lock mode,
whereas SHARED/EXCLUSIVE/SEMI_SHARED are more useful
when debugging.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.

[Mohit Sabharwal]

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


Ship it!




Ship It!

- Mohit Sabharwal


On April 3, 2017, 10:34 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> ---
> 
> (Updated April 3, 2017, 10:34 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-16164
> https://issues.apache.org/jira/browse/HIVE-16164
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This fix updates the EnvironmentContext with a DB_NOTIFICATION_EVENT_ID 
> property from withing the DbNotificationListener class. It then passes the 
> EnvironmentContext from transactional listeners to non-transactional 
> listeners so that the eventId is shared between them.
> 
> The patch provides the following changes:
> - DbNotificationListener   Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - HiveAlterHandler Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - MetaStoreListenerNotifierNew helper class that wraps the notification 
> call to the listeners.
> - TestObjectStore  Verifies that the addNotificationEvent() 
> method saves the eventId on the NotificationEvent object.
> - TestDbNotificationListener   Verifies that any HMS call is passing the 
> DB_NOTIFICATION_EVENT_ID to non-transactional listeners.
> 
> 
> Diffs
> -
> 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
>  ea6cb790bc03fc38be07a3be7be34aa9c5d293ba 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/MetaStoreEventListenerConstants.java
>  PRE-CREATION 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  1cf47c36cb490ce0b17ffe312cd2e9fc4bb7cd9a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
> 4ce6a659c94265d83abbaedb9c52d7e15bf8dde6 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 80b1e98b9efbf068f0cd32303499a3a9b5bbd67b 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
>  PRE-CREATION 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java 
> 62aeb8cc343fc4aab72e76da890e36ac3a16b7bf 
>   metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java 
> 1f87eeb18f6edf7351b3c8da6a6826c08656e48c 
> 
> 
> Diff: https://reviews.apache.org/r/57626/diff/7/
> 
> 
> Testing
> ---
> 
> HiveQA showed only one test failure. it is fixed, and waiting for HiveQA to 
> complete 100% tests.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.

[Mohit Sabharwal]

> On March 30, 2017, 1:54 p.m., Mohit Sabharwal wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
> > Lines 51-52 (patched)
> > <https://reviews.apache.org/r/57626/diff/6/?file=1677663#file1677663line51>
> >
> > What's the point of have duplicate parameter map ? What is the use case 
> > ?
> > 
> > If you need unmodifiable paramters, why can't you add a new method 
> > called getunmodifiableParameters() which simply return 
> > Collections.unmodifiableMap(parameters) ?
> > 
> > Do you really need to call updateUnmodifiableParameters() everytime ? 
> > Looks like you're only updating 'parameters' variable, but then creating a 
> > new updateUnmodifiableParameters every time a new items is added. 
> > 
> > As I said, you can simply add a new method called 
> > getunmodifiableParameters() which returns 
> > Collections.unmodifiableMap(parameters) without the need for this copy.
> 
> Sergio Pena wrote:
> We want to avoid that an external listener gets a reference to the 
> mutable map object and override any key/value already set by other listeners. 
> Part of the contract on the ListenerEvent.putParameter() is that you cannot 
> override an existing key/value. Because we don't control the external 
> listeners, we prefered to return an inmutable map whenever getParameters() 
> was called.
> 
> Does it make sense?
> 
> Mohit Sabharwal wrote:
> That makes complete sense. But why do you need two explicit references to 
> the same map stored as class members ? You could just have one reference 
> called 'parameters' and add a getter (called, say, getunmodifiableParameters) 
> that returns Collections.unmodifiableMap(parameters).
> 
> Sergio Pena wrote:
> I did that to avoid having a "live" unmodifiable map in case the 
> ListenerEvent was called by multiple threads. But, I don't think that would 
> happen. I even documented the class to state that this was not supposed to be 
> called by multiple threads. So, I think it make sense just using 
> Collections.unmodifiableMap(parameters) directly  to avoid making a copy of 
> the mutable reference for every new key/value pair.
> 
> Sergio Pena wrote:
> Mohit, forget what I said on the above response. I had in my mind I was 
> doing a copy of mutable map values to inmutable maps, but I see I am only 
> wrapping the mutable map into the inmutable map.
> 
> Anyways, this is what Sasha commented last time when I had the following 
> code:
> 
> public final Map getParameters() {
>return Collections.unmodifiableMap(parameters);
> }
> 
> Sasha comment:
> This is fine conceptually, but I'm a bit concerned about performance 
> - there are many gets (for each call to notify()) and each one has to create 
> a new copy of the map.
> If you want to preserve the code structure (many getters), I would 
> suggest caching an unmodifiable map when it is updated since updates are rare.
> 
> So far we only have 1 put on the DbNotificationListener. After that, many 
> events can call the getParameters(), and we won't know how they handle that, 
> if storing a reference of the inmutable parameters once, or if they like to 
> do 'event.getParameters().get(key)' many times. I thing that's what Sasha 
> tried to avoid.

ok, if performance is the concern, we need to add comments, because it's just 
confusing otherwise.


- Mohit


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


On March 28, 2017, 3:17 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> ---
> 
> (Updated March 28, 2017, 3:17 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-16164
> https://issues.apache.org/jira/browse/HIVE-16164
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This fix updates the EnvironmentContext with a DB_NOTIFICATION_EVENT_ID 
> property from withing the DbNotificationListener class. It then passes the 
> EnvironmentContext from transactional listeners to non-transactional 
> listeners so that the eventId is shared between them.
> 
> The patch provides the following changes:
> - DbNotificationListener  

Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.

[Mohit Sabharwal]

> On March 30, 2017, 1:54 p.m., Mohit Sabharwal wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
> > Lines 51-52 (patched)
> > <https://reviews.apache.org/r/57626/diff/6/?file=1677663#file1677663line51>
> >
> > What's the point of have duplicate parameter map ? What is the use case 
> > ?
> > 
> > If you need unmodifiable paramters, why can't you add a new method 
> > called getunmodifiableParameters() which simply return 
> > Collections.unmodifiableMap(parameters) ?
> > 
> > Do you really need to call updateUnmodifiableParameters() everytime ? 
> > Looks like you're only updating 'parameters' variable, but then creating a 
> > new updateUnmodifiableParameters every time a new items is added. 
> > 
> > As I said, you can simply add a new method called 
> > getunmodifiableParameters() which returns 
> > Collections.unmodifiableMap(parameters) without the need for this copy.
> 
> Sergio Pena wrote:
> We want to avoid that an external listener gets a reference to the 
> mutable map object and override any key/value already set by other listeners. 
> Part of the contract on the ListenerEvent.putParameter() is that you cannot 
> override an existing key/value. Because we don't control the external 
> listeners, we prefered to return an inmutable map whenever getParameters() 
> was called.
> 
> Does it make sense?

That makes complete sense. But why do you need two explicit references to the 
same map stored as class members ? You could just have one reference called 
'parameters' and add a getter (called, say, getunmodifiableParameters) that 
returns Collections.unmodifiableMap(parameters).


- Mohit


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


On March 28, 2017, 3:17 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> ---
> 
> (Updated March 28, 2017, 3:17 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-16164
> https://issues.apache.org/jira/browse/HIVE-16164
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This fix updates the EnvironmentContext with a DB_NOTIFICATION_EVENT_ID 
> property from withing the DbNotificationListener class. It then passes the 
> EnvironmentContext from transactional listeners to non-transactional 
> listeners so that the eventId is shared between them.
> 
> The patch provides the following changes:
> - DbNotificationListener   Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - HiveAlterHandler Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - MetaStoreListenerNotifierNew helper class that wraps the notification 
> call to the listeners.
> - TestObjectStore  Verifies that the addNotificationEvent() 
> method saves the eventId on the NotificationEvent object.
> - TestDbNotificationListener   Verifies that any HMS call is passing the 
> DB_NOTIFICATION_EVENT_ID to non-transactional listeners.
> 
> 
> Diffs
> -
> 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
>  ea6cb790bc03fc38be07a3be7be34aa9c5d293ba 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/MetaStoreEventListenerConstants.java
>  PRE-CREATION 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  1cf47c36cb490ce0b17ffe312cd2e9fc4bb7cd9a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
> 4ce6a659c94265d83abbaedb9c52d7e15bf8dde6 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 80b1e98b9efbf068f0cd32303499a3a9b5bbd67b 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
>  PRE-CREATION 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java 
> 62aeb8cc343fc4aab72e76da890e36ac3a16b7bf 
>   metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java 
> 1f87eeb18f6edf7351b3c8da6a6826c08656e48c 
> 
> 
> Diff: https://reviews.apache.org/r/57626/diff/6/
> 
> 
> Testing
> ---
> 
> HiveQA showed only one test failure. it is fixed, and waiting for HiveQA to 
> complete 100% tests.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.

[Mohit Sabharwal]

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




hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
Lines 487 (patched)
<https://reviews.apache.org/r/57626/#comment243432>

I didn't mean write a new method called isSetEventId. I meant that 
NotificationEvent already has a public method called isSetEventId. You can use 
that.

My basic objection is that you cannot use eventId > 0 as a proxy for 
whether eventid is set or not, because 0 can be a valid event id if the code 
ObjectStore#addNotificationEvent is changed. (The eventid > 0 assumption 
depends on code in ObjectStore#addNotificationEvent remaining same, which is 
not a safe assumption).



metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
Lines 49 (patched)
<https://reviews.apache.org/r/57626/#comment243433>

nit: 
"may be" -> is
"for a" -> about



metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java
Lines 51-52 (patched)
<https://reviews.apache.org/r/57626/#comment243434>

What's the point of have duplicate parameter map ? What is the use case ?

If you need unmodifiable paramters, why can't you add a new method called 
getunmodifiableParameters() which simply return 
Collections.unmodifiableMap(parameters) ?

Do you really need to call updateUnmodifiableParameters() everytime ? Looks 
like you're only updating 'parameters' variable, but then creating a new 
updateUnmodifiableParameters every time a new items is added. 

As I said, you can simply add a new method called 
getunmodifiableParameters() which returns 
Collections.unmodifiableMap(parameters) without the need for this copy.


- Mohit Sabharwal


On March 28, 2017, 3:17 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> ---
> 
> (Updated March 28, 2017, 3:17 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-16164
> https://issues.apache.org/jira/browse/HIVE-16164
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This fix updates the EnvironmentContext with a DB_NOTIFICATION_EVENT_ID 
> property from withing the DbNotificationListener class. It then passes the 
> EnvironmentContext from transactional listeners to non-transactional 
> listeners so that the eventId is shared between them.
> 
> The patch provides the following changes:
> - DbNotificationListener   Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - HiveAlterHandler Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - MetaStoreListenerNotifierNew helper class that wraps the notification 
> call to the listeners.
> - TestObjectStore  Verifies that the addNotificationEvent() 
> method saves the eventId on the NotificationEvent object.
> - TestDbNotificationListener   Verifies that any HMS call is passing the 
> DB_NOTIFICATION_EVENT_ID to non-transactional listeners.
> 
> 
> Diffs
> -
> 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
>  ea6cb790bc03fc38be07a3be7be34aa9c5d293ba 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/MetaStoreEventListenerConstants.java
>  PRE-CREATION 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  1cf47c36cb490ce0b17ffe312cd2e9fc4bb7cd9a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
> 4ce6a659c94265d83abbaedb9c52d7e15bf8dde6 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 80b1e98b9efbf068f0cd32303499a3a9b5bbd67b 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
>  PRE-CREATION 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java 
> 62aeb8cc343fc4aab72e76da890e36ac3a16b7bf 
>   metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java 
> 1f87eeb18f6edf7351b3c8da6a6826c08656e48c 
> 
> 
> Diff: https://reviews.apache.org/r/57626/diff/6/
> 
> 
> Testing
> ---
> 
> HiveQA showed only one test failure. it is fixed, and waiting for HiveQA to 
> complete 100% tests.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.

[Mohit Sabharwal]

> On March 17, 2017, 10:34 p.m., Mohit Sabharwal wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
> > Lines 138 (patched)
> > <https://reviews.apache.org/r/57626/diff/2/?file=1666420#file1666420line138>
> >
> > I agree with Alexander. We can avoid code duplication by doing most 
> > work inside notify.
> > 
> > I also agree that a simple static method is going to be much cleaner 
> > than MetaStoreListenerNotifier in this use case. 
> > 
> > The point of builder-like pattern (like MetaStoreListenerNotifier) to 
> > make it cleaner to construct an object. But in this case, we are 
> > introducing a new object when none is really needed. A simple static method 
> > is going to way more readable.
> > 
> > Even cost-wise the branch (listeners != null) is expensive, so let's 
> > not do it twice.
> 
> Sergio Pena wrote:
> I can avoid the creationg of the new object, and just have a static 
> notifyEvent() method that accepts some parameters, perhaps something like 
> this:
> 
> if (!listeners.isEmpty()) {
>   CreateTableEvent createTableEvent = new CreateTableEvent(tbl, success, 
> this);
>   createTableEvent.addParameters(transactionalListenerResponses);
>   createTableEvent.setEnvironmentContext(envContext);
> 
>   MetaStoreListenerNotifier
> .notifyEvent(EventType.CREATE_TABLE, createTableEvent, listeners);
> }
>   
> But, I think I still need to check that listeners is not empty before 
> calling the above code to avoid creating the CreateTableEvent object if 
> listeners is empty.
> 
> I can avoid too much code, and make it cleaner by passing the 2 extra 
> parameters to the notifier, but I still need to create the event.
> 
> if (!listeners.isEmpty()) {
>   CreateTableEvent createTableEvent = new CreateTableEvent(tbl, success, 
> this);
> 
>   MetaStoreListenerNotifier
> .notifyEvent(EventType.CREATE_TABLE, createTableEvent, 
> transactionalListenerResponses, envContext, listeners);
> }
> 
> I was also thinking on create one method per event, and pass the same 
> parameters, and let the method to create the event only if listeners is not 
> empty. Something like this?
> 
> MetaStoreListenerNotifier.
>.onCreateTable(tbl, success, this, transactionalListenerResponses, 
> envContext, listeners);
>
> The above code would look cleaner on the HiveMetaStore, but the 
> MetaStoreListenerNotifier must support every event method ,and accept at 
> least 6 parameters like the above code.
> 
> Sasha, Mohit, which option would you think is ideal? I can also go back 
> to the original code, and keep the for() loops to walk through each listener.

Why not do what Alexander suggests. His method signature looks like:

public static Optional> notifyAll(EventType eventType, 
ListenerEvent 
listenerEvent,
EnvironmentContext 
context,
final 
List listeners
)
So, in other words, no MetaStoreListenerNotifier object is needed.

I don't increasing number of parameters to methods is a bad idea. I find
it more readable -- you know exactly what parameters the method is dealing with.


- Mohit


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


On March 20, 2017, 10:33 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> ---
> 
> (Updated March 20, 2017, 10:33 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-16164
> https://issues.apache.org/jira/browse/HIVE-16164
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This fix updates the EnvironmentContext with a DB_NOTIFICATION_EVENT_ID 
> property from withing the DbNotificationListener class. It then passes the 
> EnvironmentContext from transactional listeners to non-transactional 
> listeners so that the eventId is shared between them.
> 
> The patch provides the following changes:
> - DbNotificationListener   Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
>

Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.

[Mohit Sabharwal]

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




hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
Lines 493 (patched)
<https://reviews.apache.org/r/57626/#comment242041>

use isSetEventId() instead ? 

Don't think comparing with 0 works, 
because this depends on current
assumption in ObjectStore.addNotificationEvent,
which makes it brittle.


- Mohit Sabharwal


On March 20, 2017, 10:33 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> ---
> 
> (Updated March 20, 2017, 10:33 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-16164
> https://issues.apache.org/jira/browse/HIVE-16164
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This fix updates the EnvironmentContext with a DB_NOTIFICATION_EVENT_ID 
> property from withing the DbNotificationListener class. It then passes the 
> EnvironmentContext from transactional listeners to non-transactional 
> listeners so that the eventId is shared between them.
> 
> The patch provides the following changes:
> - DbNotificationListener   Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - HiveAlterHandler Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - MetaStoreListenerNotifierNew helper class that wraps the notification 
> call to the listeners.
> - TestObjectStore  Verifies that the addNotificationEvent() 
> method saves the eventId on the NotificationEvent object.
> - TestDbNotificationListener   Verifies that any HMS call is passing the 
> DB_NOTIFICATION_EVENT_ID to non-transactional listeners.
> 
> 
> Diffs
> -
> 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
>  f7e3e3a0a71094992fdf4bd3ceea2da0bf7d1ff0 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/MetaStoreEventListenerConstants.java
>  PRE-CREATION 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  1cf47c36cb490ce0b17ffe312cd2e9fc4bb7cd9a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
> 4ce6a659c94265d83abbaedb9c52d7e15bf8dde6 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 07eca38190c1b05bb4a3977e9154423449828957 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
>  PRE-CREATION 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/events/ListenerEvent.java 
> 62aeb8cc343fc4aab72e76da890e36ac3a16b7bf 
>   metastore/src/test/org/apache/hadoop/hive/metastore/TestObjectStore.java 
> 1f87eeb18f6edf7351b3c8da6a6826c08656e48c 
> 
> 
> Diff: https://reviews.apache.org/r/57626/diff/4/
> 
> 
> Testing
> ---
> 
> HiveQA showed only one test failure. it is fixed, and waiting for HiveQA to 
> complete 100% tests.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 57626: HIVE-16164: Provide mechanism for passing HMS notification ID between transactional and non-transactional listeners.

[Mohit Sabharwal]

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




hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
Lines 80 (patched)
<https://reviews.apache.org/r/57626/#comment241665>

I'd call this DB_NOTIFICATION_EVENT_ID_KEY_NAME, since this is not the id, 
but a key to find the id.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Line 82 (original), 87 (patched)
<https://reviews.apache.org/r/57626/#comment241666>

This really clutters the namespace, so please don't remove the wildcard, 
even if every class is needed.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Line 2476 (original), 2484 (patched)
<https://reviews.apache.org/r/57626/#comment241674>

I like the idea of keeping fireMetaStoreAddPartitionEventTransactional 
method - because it makes it more readable that way. Otherwise, very similar 
looking code is duplicated 7 times. 

You can have fireMetaStoreAddPartitionEventTransactional return 
transactionalListenerResponses for later re-use.  But I wouldn't inline this 
code directly in add_partitions_core for readability reasons.



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 3020 (patched)
<https://reviews.apache.org/r/57626/#comment241694>

use i for consistency :)



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
Lines 3308 (patched)
<https://reviews.apache.org/r/57626/#comment241695>

extra line



metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
Lines 138 (patched)
<https://reviews.apache.org/r/57626/#comment241699>

I agree with Alexander. We can avoid code duplication by doing most work 
inside notify.

I also agree that a simple static method is going to be much cleaner than 
MetaStoreListenerNotifier in this use case. 

The point of builder-like pattern (like MetaStoreListenerNotifier) to make 
it cleaner to construct an object. But in this case, we are introducing a new 
object when none is really needed. A simple static method is going to way more 
readable.

Even cost-wise the branch (listeners != null) is expensive, so let's not do 
it twice.


- Mohit Sabharwal


On March 17, 2017, 8:14 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57626/
> ---
> 
> (Updated March 17, 2017, 8:14 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-16164
> https://issues.apache.org/jira/browse/HIVE-16164
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This fix updates the EnvironmentContext with a DB_NOTIFICATION_EVENT_ID 
> property from withing the DbNotificationListener class. It then passes the 
> EnvironmentContext from transactional listeners to non-transactional 
> listeners so that the eventId is shared between them.
> 
> The patch provides the following changes:
> - DbNotificationListener   Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - HiveAlterHandler Changes to pass the EnvironmentContext from 
> transactional to non-transactional listeners.
> - MetaStoreListenerNotifierNew helper class that wraps the notification 
> call to the listeners.
> - TestObjectStore  Verifies that the addNotificationEvent() 
> method saves the eventId on the NotificationEvent object.
> - TestDbNotificationListener   Verifies that any HMS call is passing the 
> DB_NOTIFICATION_EVENT_ID to non-transactional listeners.
> 
> 
> Diffs
> -
> 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
>  f7e3e3a0a71094992fdf4bd3ceea2da0bf7d1ff0 
>   
> hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/MetaStoreEventListenerConstants.java
>  PRE-CREATION 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  1cf47c36cb490ce0b17ffe312cd2e9fc4bb7cd9a 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
> bae39acafeb86d04ac8ec66098be125cd3cef3e0 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 07eca38190c1b05bb4a3977e9154423449828957 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreListenerNotifier.java
>  PRE-CREATION 
>   
> metastore/src/java/org/ap

Re: [VOTE] Drop support for Java7 in master branch

[Mohit Sabharwal]

+1



On Tue, Feb 28, 2017 at 11:12 AM, Thejas Nair  wrote:

> Note that upgrading the minimum required version to JDK8 also gives Hive
> the option of using more recent versions of several libraries including
> hikaricp (as default connection pool option) [1] and jetty [2]
>
> [1] https://issues.apache.org/jira/browse/HIVE-13931
> [2] https://issues.apache.org/jira/browse/HIVE-16049
>
>
> On Tue, Feb 28, 2017 at 7:45 AM, Sergio Pena 
> wrote:
>
> > Thanks Alan for the proposal. JDK8 seems to have very useful API. It
> should
> > be good to start using it.
> >
> > +1.
> >
> > On Tue, Feb 28, 2017 at 12:55 AM, Rajat Khandelwal <
> > rajat.khandel...@inmobi.com> wrote:
> >
> > > +1
> > >
> > > On Tue, Feb 28, 2017 at 10:37 AM, Prasanth Jayachandran <
> > > pjayachand...@hortonworks.com> wrote:
> > >
> > > > +1
> > > >
> > > > Thanks
> > > > Prasanth
> > > >
> > > >
> > > >
> > > >
> > > > On Mon, Feb 27, 2017 at 8:55 PM -0800, "Thejas Nair" <
> > > > thejas.n...@gmail.com> wrote:
> > > >
> > > >
> > > > There was a [DISCUSS] thread on the topic of moving to jdk8 for unit
> > > tests
> > > > [1], and many people also expressed the opinion that we should drop
> > JDK 7
> > > > support in Hive. Public updates by Oracle was stopped on Apr 2015
> [2].
> > > >
> > > > This vote thread proposes to dropping JDK 7 support in the next
> Apache
> > > Hive
> > > > 2.x release (ie master branch), so that we can start leveraging new
> > > > features in Java 8 and also libraries that require java8.
> > > >
> > > > [1] https://s.apache.org/hive-jdk8-test
> > > > [2] http://www.oracle.com/technetwork/java/eol-135779.html
> > > >
> > > > Here is my +1.
> > > >
> > > > Vote ends in 72 hours.
> > > > Thanks,
> > > > Thejas
> > > >
> > > > PS: I think this would fall under "Code change" under Hive-bylaws, so
> > it
> > > > doesn't seem to really require a formal vote thread. But I think this
> > > does
> > > > merit wider attention than a jira ticket.
> > > >
> > > >
> > >
> > >
> > > --
> > > Rajat Khandelwal
> > > Tech Lead
> > >
> > > --
> > > _
> > > The information contained in this communication is intended solely for
> > the
> > > use of the individual or entity to whom it is addressed and others
> > > authorized to receive it. It may contain confidential or legally
> > privileged
> > > information. If you are not the intended recipient you are hereby
> > notified
> > > that any disclosure, copying, distribution or taking any action in
> > reliance
> > > on the contents of this information is strictly prohibited and may be
> > > unlawful. If you have received this communication in error, please
> notify
> > > us immediately by responding to this email and then delete it from your
> > > system. The firm is neither liable for the proper and complete
> > transmission
> > > of the information contained in this communication nor for any delay in
> > its
> > > receipt.
> > >
> >
>

Re: Review Request 56687: Intern strings in various critical places to reduce memory consumption.

[Mohit Sabharwal]

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




common/src/java/org/apache/hadoop/hive/common/StringInternUtils.java (line 69)
<https://reviews.apache.org/r/56687/#comment238743>

Nit: please follow hive coding conventions for if statements. Same in other 
places. 
(http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-142311.html#431)



ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockObject.java (line 57)
<https://reviews.apache.org/r/56687/#comment238746>

any point in interning a timestamp ? likelihood of this hitting the pool is 
almost zero, correct ?



ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java (lines 205 - 220)
<https://reviews.apache.org/r/56687/#comment238751>

Do these paths eventually getting interned up the chain or are these 
ignored because these are aren't used/accessed in PartitionDesc ?...wasn't 
clear to me.



ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java (lines 238 - 245)
<https://reviews.apache.org/r/56687/#comment238756>

same for database name, table name strings accessed via 
MetaStoreUtils.getSchema -- getting interned someplace ?



ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java (lines 324 - 334)
<https://reviews.apache.org/r/56687/#comment238758>

same for this.



ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java (lines 367 - 379)
<https://reviews.apache.org/r/56687/#comment238761>

same for this.



ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java (line 91)
<https://reviews.apache.org/r/56687/#comment238763>

what about this ?


- Mohit Sabharwal


On Feb. 23, 2017, 9:01 p.m., Misha Dmitriev wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56687/
> ---
> 
> (Updated Feb. 23, 2017, 9:01 p.m.)
> 
> 
> Review request for hive, Chaoyu Tang, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/HIVE-15882
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/HIVE-15882
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See the description of the problem in 
> https://issues.apache.org/jira/browse/HIVE-15882 Interning strings per this 
> review removes most of the overhead due to duplicate strings.
> 
> Also, where maps in several places are created from other maps, use the 
> original map's size for the new map. This is to avoid the situation when a 
> map with default capacity (typically 16) is created to hold just 2-3 entries, 
> and the rest of the internal 16-entry array is wasted.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/StringInternUtils.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 
> e81cbce3e333d44a4088c10491f399e92a505293 
>   ql/src/java/org/apache/hadoop/hive/ql/hooks/Entity.java 
> 08420664d59f28f75872c25c9f8ee42577b23451 
>   ql/src/java/org/apache/hadoop/hive/ql/io/CombineHiveInputFormat.java 
> e91064b9c75e8adb2b36f21ff19ec0c1539b03b9 
>   ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 
> 51530ac16c92cc75d501bfcb573557754ba0c964 
>   ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java 
> 55b3b551a1dac92583b6e03b10beb8172ca93d45 
>   ql/src/java/org/apache/hadoop/hive/ql/lockmgr/HiveLockObject.java 
> 82dc89803be9cf9e0018720eeceb90ff450bfdc8 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java 
> c0edde9e92314d86482b5c46178987e79fae57fe 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java 
> c6ae6f290857cfd10f1023058ede99bf4a10f057 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java 
> 24d16812515bdfa90b4be7a295c0388fcdfe95ef 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/GenMRSkewJoinProcessor.java
>  ede4fcbe342052ad86dadebcc49da2c0f515ea98 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/NullScanTaskDispatcher.java
>  0882ae2c6205b1636cbc92e76ef66bb70faadc76 
>   
> ql/src/java/org/apache/hadoop/hive/ql/plan/ConditionalResolverMergeFiles.java 
> 68b0ad9ea63f051f16fec3652d8525f7ab07eb3f 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/MapWork.java 
> d4bdd96eaf8d179bed43b8a8c3be0d338940154a 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/MsckDesc.java 
> b7a7e4b7a5f8941b080c7805d224d3885885f444 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java 
> 73981e826

[jira] [Created] (HIVE-16016) Use same PersistenceManager for metadata and notification

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-16016:
--

 Summary: Use same PersistenceManager for metadata and notification
 Key: HIVE-16016
 URL: https://issues.apache.org/jira/browse/HIVE-16016
 Project: Hive
  Issue Type: Bug
  Components: repl
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


HIVE-13966 added to support for persisting notification in the same JDO 
transaction as the metadata event. However, the notification is currently added 
using a different ObjectStore object from the one used to persist the metadata 
event.  

The notification is added using the ObjectStore constructed in 
DbNotificationListener, whereas the metadata event is added via the thread 
local ObjectStore (i.e. threadLocalMS in HiveMetaStore.HMSHandler).

As a result, different PersistenceManagers (different transactions) are used to 
persist notification and metadata events.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Re: Review Request 54393: HIVE-15361: INSERT dynamic partition on S3 fails with a MoveTask failure

[Mohit Sabharwal]

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



lgtm overall, one question.


ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java (line 1649)
<https://reviews.apache.org/r/54393/#comment229109>

can BlobStorageUtils.areOptimizationsEnabled(conf) return true for non 
blobstores  ?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java (line 1726)
<https://reviews.apache.org/r/54393/#comment229110>

throw RuntimeException


- Mohit Sabharwal


On Dec. 6, 2016, 8:13 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54393/
> ---
> 
> (Updated Dec. 6, 2016, 8:13 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-15361
> https://issues.apache.org/jira/browse/HIVE-15361
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Problem:
> - DynamicPartitionCtx and ListBucketingCtx objects weren't set on the new 
> MoveWork created when merging the two MoveWork objects from the 
> ConditionalTask.
> 
> Solution
> - Set the DynamicPartitionCtx and ListBucketingCtx objects to the new 
> MoveWork created for the S3 optimization.
> 
> Other changes
> - Merge the MoveWork objects inside the createCondTask() method for better 
> error handling.
> - Only merge the MoveWork related to the moveOnlyMoveTask. The MoveWork from 
> the mergeAndMoveMoveTask may cause other issues that are not correctly tested.
> - Two new private methods are added to check and merge the conditional 
> input/output tasks to the linked MoveWork.
> 
> 
> Diffs
> -
> 
>   
> itests/hive-blobstore/src/test/queries/clientpositive/insert_into_dynamic_partitions.q
>  PRE-CREATION 
>   itests/hive-blobstore/src/test/queries/clientpositive/insert_into_table.q 
> 25e2e7007ff539223d9244ca9822aa65d1441eb0 
>   
> itests/hive-blobstore/src/test/queries/clientpositive/insert_overwrite_dynamic_partitions.q
>  PRE-CREATION 
>   
> itests/hive-blobstore/src/test/queries/clientpositive/insert_overwrite_table.q
>  846b2b113f09a74a3f05c13ffb56163e81dc1e8e 
>   
> itests/hive-blobstore/src/test/results/clientpositive/insert_into_dynamic_partitions.q.out
>  PRE-CREATION 
>   
> itests/hive-blobstore/src/test/results/clientpositive/insert_into_table.q.out 
> fbb52c132a331aefe870264e035c397078f3c82e 
>   
> itests/hive-blobstore/src/test/results/clientpositive/insert_overwrite_directory.q.out
>  9f575a66ecefc3933b16dff554bdcc1c1f6420ee 
>   
> itests/hive-blobstore/src/test/results/clientpositive/insert_overwrite_dynamic_partitions.q.out
>  PRE-CREATION 
>   
> itests/hive-blobstore/src/test/results/clientpositive/insert_overwrite_table.q.out
>  c725c96cbb6b0374e67308a54204c7c25e827567 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/GenMapRedUtils.java 
> adc1188f09c8019a8aa60403d5813d6fa4509ceb 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/LoadDesc.java 
> bcd3125ab4ad20c00fec565e5004ee200c0187d5 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/LoadFileDesc.java 
> 9a868a04ce93d5c2ee75b5c6e96a1401cea93133 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/LoadTableDesc.java 
> 771a919ccd0bd75fe6197299ae057647ece89a7e 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/MoveWork.java 
> 9f498c7fb88a7a9f77b8c6739c097a2b26b0c617 
>   
> ql/src/test/org/apache/hadoop/hive/ql/optimizer/TestGenMapRedUtilsCreateConditionalTask.java
>  e6ec44504685bd9e53f158cc359b8a7b79fd0166 
> 
> Diff: https://reviews.apache.org/r/54393/diff/
> 
> 
> Testing
> ---
> 
> All itests/hive-blobstore tests run.
> 
> Added new blobstore tests:
> - insert_into_dynamic_partitions.q
> - insert_overwrite_dynamic_partitions.q
> 
> Waiting for HiveQA to run the rest of the q-tests.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

[jira] [Created] (HIVE-15305) Add tests for METASTORE_EVENT_LISTENERS

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-15305:
--

 Summary: Add tests for METASTORE_EVENT_LISTENERS
 Key: HIVE-15305
 URL: https://issues.apache.org/jira/browse/HIVE-15305
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


HIVE-15232 reused TestDbNotificationListener to test 
METASTORE_TRANSACTIONAL_EVENT_LISTENERS and removed unit testing of 
METASTORE_EVENT_LISTENERS config. We should test both. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 53838: HIVE-15232: Add notification events for functions and indexes

[Mohit Sabharwal]

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

(Updated Nov. 17, 2016, 9:26 p.m.)


Review request for hive.


Changes
---

Incorp. review feedback.


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


Repository: hive-git


Description
---

Adds notification events for Create/Drop Function and Create/Drop/Alter Index.


Diffs (updated)
-

  
hcatalog/core/src/main/java/org/apache/hive/hcatalog/common/HCatConstants.java 
72930eb66c2b44fc6ee414c40ead713eb83413c1 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
 0b3d89198f1d89c057d2639b89799b53b4a09163 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/AlterIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/CreateFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/CreateIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/DropFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/DropIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/HCatEventMessage.java
 538fa68a83081604ca6598d886021a633dfecea7 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/MessageDeserializer.java
 8ea39987f022175f0bcb6e1ecd0945847fd70996 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/MessageFactory.java
 0710dd09dbafb11ce3a4819bc2bd5f50bf70dc84 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONAlterIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONCreateFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONCreateIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONDropFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONDropIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONMessageDeserializer.java
 834fdde113a293fcc595c8bfe329a9299ae8aabc 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONMessageFactory.java
 6b74b54b5d8f3ef8735b8bae5535567a7c4dc76a 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 4a7801bbf010dbcfa2044381cfd2e913f83f5445 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
 1cd32d5859899960337c9525ac033cd1cfb64ab2 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
c0ef25e103d0b7cbb187cea2111efc6a40e01d85 
  
metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java 
5e46ae1d83296d11957038c76ee2e4d207db8992 
  
metastore/src/java/org/apache/hadoop/hive/metastore/events/CreateFunctionEvent.java
 PRE-CREATION 
  
metastore/src/java/org/apache/hadoop/hive/metastore/events/DropFunctionEvent.java
 PRE-CREATION 
  metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java 
a3b16d049c9c06f08e5ef8809ad386274479d66d 

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


Testing
---

Verified using TestDbNotificationListener


Thanks,

Mohit Sabharwal

Re: Review Request 53838: HIVE-15232: Add notification events for functions and indexes

[Mohit Sabharwal]

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

(Updated Nov. 17, 2016, 8:48 p.m.)


Review request for hive.


Changes
---

Fixed test failure.


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


Repository: hive-git


Description
---

Adds notification events for Create/Drop Function and Create/Drop/Alter Index.


Diffs (updated)
-

  
hcatalog/core/src/main/java/org/apache/hive/hcatalog/common/HCatConstants.java 
72930eb66c2b44fc6ee414c40ead713eb83413c1 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
 0b3d89198f1d89c057d2639b89799b53b4a09163 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/AlterIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/CreateFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/CreateIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/DropFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/DropIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/HCatEventMessage.java
 538fa68a83081604ca6598d886021a633dfecea7 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/MessageDeserializer.java
 8ea39987f022175f0bcb6e1ecd0945847fd70996 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/MessageFactory.java
 0710dd09dbafb11ce3a4819bc2bd5f50bf70dc84 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONAlterIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONCreateFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONCreateIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONDropFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONDropIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONMessageDeserializer.java
 834fdde113a293fcc595c8bfe329a9299ae8aabc 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONMessageFactory.java
 6b74b54b5d8f3ef8735b8bae5535567a7c4dc76a 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 4a7801bbf010dbcfa2044381cfd2e913f83f5445 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
 1cd32d5859899960337c9525ac033cd1cfb64ab2 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
c0ef25e103d0b7cbb187cea2111efc6a40e01d85 
  
metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java 
5e46ae1d83296d11957038c76ee2e4d207db8992 
  
metastore/src/java/org/apache/hadoop/hive/metastore/events/CreateFunctionEvent.java
 PRE-CREATION 
  
metastore/src/java/org/apache/hadoop/hive/metastore/events/DropFunctionEvent.java
 PRE-CREATION 
  metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java 
a3b16d049c9c06f08e5ef8809ad386274479d66d 

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


Testing
---

Verified using TestDbNotificationListener


Thanks,

Mohit Sabharwal

Review Request 53838: HIVE-15232: Add notification events for functions and indexes

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

Adds notification events for Create/Drop Function and Create/Drop/Alter Index.


Diffs
-

  
hcatalog/core/src/main/java/org/apache/hive/hcatalog/common/HCatConstants.java 
72930eb66c2b44fc6ee414c40ead713eb83413c1 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/listener/DbNotificationListener.java
 0b3d89198f1d89c057d2639b89799b53b4a09163 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/AlterIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/CreateFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/CreateIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/DropFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/DropIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/HCatEventMessage.java
 538fa68a83081604ca6598d886021a633dfecea7 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/MessageDeserializer.java
 8ea39987f022175f0bcb6e1ecd0945847fd70996 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/MessageFactory.java
 0710dd09dbafb11ce3a4819bc2bd5f50bf70dc84 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONAlterIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONCreateFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONCreateIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONDropFunctionMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONDropIndexMessage.java
 PRE-CREATION 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONMessageDeserializer.java
 834fdde113a293fcc595c8bfe329a9299ae8aabc 
  
hcatalog/server-extensions/src/main/java/org/apache/hive/hcatalog/messaging/json/JSONMessageFactory.java
 6b74b54b5d8f3ef8735b8bae5535567a7c4dc76a 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 4a7801bbf010dbcfa2044381cfd2e913f83f5445 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
 1cd32d5859899960337c9525ac033cd1cfb64ab2 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
c0ef25e103d0b7cbb187cea2111efc6a40e01d85 
  
metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreEventListener.java 
5e46ae1d83296d11957038c76ee2e4d207db8992 
  
metastore/src/java/org/apache/hadoop/hive/metastore/events/CreateFunctionEvent.java
 PRE-CREATION 
  
metastore/src/java/org/apache/hadoop/hive/metastore/events/DropFunctionEvent.java
 PRE-CREATION 
  metastore/src/test/org/apache/hadoop/hive/metastore/DummyListener.java 
a3b16d049c9c06f08e5ef8809ad386274479d66d 

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


Testing
---

Verified using TestDbNotificationListener


Thanks,

Mohit Sabharwal

[jira] [Created] (HIVE-15232) Add notification events for functions and indexes

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-15232:
--

 Summary: Add notification events for functions and indexes
 Key: HIVE-15232
 URL: https://issues.apache.org/jira/browse/HIVE-15232
 Project: Hive
  Issue Type: Improvement
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


Create/Drop Function and Create/Drop/Alter Index should also generate metastore 
notification events.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 52800: HIVE-13966: DbNotificationListener: can loose DDL operation notifications

[Mohit Sabharwal]

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

(Updated Nov. 10, 2016, 5:40 p.m.)


Review request for hive.


Changes
---

Added Deprecated annotation.


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


Repository: hive-git


Description
---

Metadata event and associated notification should be committed in the same
JDO transaction.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
7ce29e10798730061d02fadb55423bea3b69c6ac 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 PRE-CREATION 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
 af16f75e63c372c37bfd73567b777bba53f94db3 
  metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java 
dedd4497adfcc9d57090a943f6bb4f35ea87fa61 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
40b337a9e40ea04a37f108146853d2d1f42dcd29 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
60e462fd06a3f84d5b87cd335afb49768cb27562 

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


Testing
---

Enhanced TestDbNotificationListener


Thanks,

Mohit Sabharwal

Re: Review Request 52800: HIVE-13966: DbNotificationListener: can loose DDL operation notifications

[Mohit Sabharwal]

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

(Updated Nov. 10, 2016, 5:34 p.m.)


Review request for hive.


Changes
---

Incorp. review feedback.


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


Repository: hive-git


Description
---

Metadata event and associated notification should be committed in the same
JDO transaction.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
7ce29e10798730061d02fadb55423bea3b69c6ac 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 PRE-CREATION 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
 af16f75e63c372c37bfd73567b777bba53f94db3 
  metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java 
dedd4497adfcc9d57090a943f6bb4f35ea87fa61 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
40b337a9e40ea04a37f108146853d2d1f42dcd29 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
60e462fd06a3f84d5b87cd335afb49768cb27562 

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


Testing
---

Enhanced TestDbNotificationListener


Thanks,

Mohit Sabharwal

Re: Review Request 52800: HIVE-13966: DbNotificationListener: can loose DDL operation notifications

[Mohit Sabharwal]

> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java, line 
> > 86
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556283#file1556283line86>
> >
> > I wonder if the API changes will cause some backward compatibility 
> > issue, if the HiveMetaStore is currently the only consumer of the 
> > AlterHandler, it should be fine.
> 
> Mohit Sabharwal wrote:
> That's a good point. I have created new versions of these methods. But 
> adding new methods makes it backward compatibility as well, right ? I'm not 
> sure there is a good solution here.
> 
> Chaoyu Tang wrote:
> Yeah, that was what I thought, add new APIs and mark the old ones as 
> deprecated. Besides this, I do not have other better solution as well.

Ok, thanks, marking old ones as deprecated.


- Mohit


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


On Nov. 10, 2016, 3:43 a.m., Mohit Sabharwal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52800/
> ---
> 
> (Updated Nov. 10, 2016, 3:43 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-13966
> https://issues.apache.org/jira/browse/HIVE-13966
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Metadata event and associated notification should be committed in the same
> JDO transaction.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 7ce29e10798730061d02fadb55423bea3b69c6ac 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  PRE-CREATION 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
>  af16f75e63c372c37bfd73567b777bba53f94db3 
>   metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java 
> dedd4497adfcc9d57090a943f6bb4f35ea87fa61 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
> 40b337a9e40ea04a37f108146853d2d1f42dcd29 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 60e462fd06a3f84d5b87cd335afb49768cb27562 
> 
> Diff: https://reviews.apache.org/r/52800/diff/
> 
> 
> Testing
> ---
> 
> Enhanced TestDbNotificationListener
> 
> 
> Thanks,
> 
> Mohit Sabharwal
> 
>

Re: Review Request 52800: HIVE-13966: DbNotificationListener: can loose DDL operation notifications

[Mohit Sabharwal]

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

(Updated Nov. 10, 2016, 3:43 a.m.)


Review request for hive.


Changes
---

Incorp. review feedback


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


Repository: hive-git


Description
---

Metadata event and associated notification should be committed in the same
JDO transaction.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
7ce29e10798730061d02fadb55423bea3b69c6ac 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 PRE-CREATION 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
 af16f75e63c372c37bfd73567b777bba53f94db3 
  metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java 
dedd4497adfcc9d57090a943f6bb4f35ea87fa61 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
40b337a9e40ea04a37f108146853d2d1f42dcd29 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
60e462fd06a3f84d5b87cd335afb49768cb27562 

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


Testing
---

Enhanced TestDbNotificationListener


Thanks,

Mohit Sabharwal

Re: Review Request 52800: HIVE-13966: DbNotificationListener: can loose DDL operation notifications

[Mohit Sabharwal]

> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 790
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556279#file1556279line790>
> >
> > What kind of events need to be invoked in a separate JDO transactions? 
> > or did you mean the events that do not require a JDO transaction?

There are few scenarios where it may make sense to still use separate JDO 
transactions (and hence the old config), like:

(1) When the listener method invocation is expensive. For example, say an 
external authorization service has it's own listener implementation
that makes an RPC call to an (external) server. This call may block forever. To 
avoid rolling back original metadata from HMS db, user may want 
to do these in separate transactions -- with the understanding that the two may 
get inconsistent.

If, on the other hand, the listener is doing a relatively predictable operation 
like adding notification to a queue or persisting to the 
HMS db schema (like DbNotificationListener), it may be reasonable to expect 
that both happen in the same transaction. This is the motivation
for the new config.

(2) When the notification needs to be logged irrespective of whether the 
metadata event failed or succeeds. Here the notification acts more
like a audit.

(3) For backward compatibility.


> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 794
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556279#file1556279line794>
> >
> > I wonder why we need a separate transactional event listener in Hive. 
> > Can it be deferred to listener implementation? For example, a listener 
> > which needs be transactional first checks if it is under a transaction 
> > context, joins (or enlists) itself to the transaction if it is. This way 
> > might also be applicable to the cases where multiple backends invovle (e.g. 
> > distributed transaction). It is just my thought, and may not necessarily be 
> > right or feasible.

Any implementation of MetaStoreEventListener interface can be configured so 
that it's invoked in the same 
transaction as the metadata event. This patch does not introduce any separate 
transactional event listener.

If user wants both the metadata event and notification to happen in the same 
JDO transaction, they 
configure hive.metastore.transactional.event.listeners, otherwise they 
configure hive.metastore.event.listeners.

This way the interface implementation does not need to be changed. 

Only the configuration needs to be changed.


> On Nov. 8, 2016, 4:13 p.m., Chaoyu Tang wrote:
> > metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java, line 
> > 86
> > <https://reviews.apache.org/r/52800/diff/1/?file=1556283#file1556283line86>
> >
> > I wonder if the API changes will cause some backward compatibility 
> > issue, if the HiveMetaStore is currently the only consumer of the 
> > AlterHandler, it should be fine.

That's a good point. I have created new versions of these methods. But adding 
new methods makes it backward compatibility as well, right ? I'm not sure there 
is a good solution here.


- Mohit


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


On Nov. 7, 2016, 11:25 p.m., Mohit Sabharwal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52800/
> ---
> 
> (Updated Nov. 7, 2016, 11:25 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-13966
> https://issues.apache.org/jira/browse/HIVE-13966
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Metadata event and associated notification should be committed in the same
> JDO transaction.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 80cd5ada060331797a603848e268c7d2a78a679c 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  PRE-CREATION 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
>  81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
>  af16f75e63c372c37bfd73567b777bba53f94db3 
>   metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java 
> dedd4497adfcc9d57090a943f

[jira] [Created] (HIVE-15145) Alter partition and fs renameDir should happen in same transaction

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-15145:
--

 Summary: Alter partition and fs renameDir should happen in same 
transaction
 Key: HIVE-15145
 URL: https://issues.apache.org/jira/browse/HIVE-15145
 Project: Hive
  Issue Type: Improvement
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


In HiveAlterTable#alterPartition, the rename partition logic is as follows:
- open txn
- db.alterPartition(old, new)
- commit txn
- fs.mkdirs
- if mkdirs fails
-- open txn
-- db.alterPartition(new, old)
-- commit txn

Instead, both db.alterPartition(old, new) and fs.mkdirs can be moved under a 
single transaction.

Currently, if the second db.alterPartition(new, old) fails, we have a partition 
metadata committed without corresponding partition directory.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Review Request 52800: HIVE-13966: DbNotificationListener: can loose DDL operation notifications

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

Metadata event and associated notification should be committed in the same
JDO transaction.


Diffs
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
80cd5ada060331797a603848e268c7d2a78a679c 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 PRE-CREATION 
  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
 81ce67bdc8fdaf11ff4fec3f255ed0021a4752c7 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreEventListener.java
 af16f75e63c372c37bfd73567b777bba53f94db3 
  metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java 
dedd4497adfcc9d57090a943f6bb4f35ea87fa61 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 
40b337a9e40ea04a37f108146853d2d1f42dcd29 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
60e462fd06a3f84d5b87cd335afb49768cb27562 

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


Testing
---

Enhanced TestDbNotificationListener


Thanks,

Mohit Sabharwal

Re: Review Request 52283: HIVE-14822 : Add support for credential provider for jobs launched from Hiveserver2

[Mohit Sabharwal]

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


Ship it!




Ship It!

- Mohit Sabharwal


On Oct. 14, 2016, 6:32 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52283/
> ---
> 
> (Updated Oct. 14, 2016, 6:32 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Sergio Pena.
> 
> 
> Bugs: HIVE-14822
> https://issues.apache.org/jira/browse/HIVE-14822
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This change adds support for credential provider for jobs launched from 
> HiveServer2. It also adds support for job-specific credential provider and 
> password which is passed to the jobs via the job configs when they are 
> launched from HS2. The hive specific credential provider location is 
> specified by a new configuration property specific to hiveserver2 and the 
> password to job credential provider is provided using the environment variable
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/FileUtils.java 
> 3ed2d086fd8e14b9a70550c02082c1456f905a08 
>   common/src/java/org/apache/hadoop/hive/conf/Constants.java 
> 77c6aa5663c2c5358715801bc039c0fe8035e3dc 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 18b98e9f28d363611da9b150ed9480cf350220cf 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 
> 16c2eafdb6888ed62168dd00f76335fa2484753b 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
> 507e369bbbae55cf2e6006209558dbb9112cc684 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 
> cea9582c8ccb0c79700ac7913735d4cdf52f0c7e 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 
> 784b9c9fa769eeb088e3a6408a0b29147a8b4086 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java 
> c75333d7022f776aab184a4b804fd7ad7befac64 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/RemoteHiveSparkClient.java 
> a9f70c4100219c8929abd4e31b30d340e6ba98bd 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
> PRE-CREATION 
>   
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
> 936fdafdb37c461a7a5deb97cba72d4db54a49e1 
> 
> Diff: https://reviews.apache.org/r/52283/diff/
> 
> 
> Testing
> ---
> 
> Testing running multiple queries on S3 with keys stored in a credential 
> provider
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 52283: HIVE-14822 : Add support for credential provider for jobs launched from Hiveserver2

[Mohit Sabharwal]

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


Fix it, then Ship it!





common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java (line 187)
<https://reviews.apache.org/r/52283/#comment221543>

private



common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java (lines 212 - 230)
<https://reviews.apache.org/r/52283/#comment221546>

I think it is worth isolating this into a utility method in 
HiveStringUtils, no ? There even might be something there or Utilities.java 
that you could re-use.



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(line 191)
<https://reviews.apache.org/r/52283/#comment221547>

extra line



spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
(line 495)
<https://reviews.apache.org/r/52283/#comment221548>

static ?


- Mohit Sabharwal


On Oct. 12, 2016, 8:34 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52283/
> ---
> 
> (Updated Oct. 12, 2016, 8:34 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Sergio Pena.
> 
> 
> Bugs: HIVE-14822
> https://issues.apache.org/jira/browse/HIVE-14822
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This change adds support for credential provider for jobs launched from 
> HiveServer2. It also adds support for job-specific credential provider and 
> password which is passed to the jobs via the job configs when they are 
> launched from HS2. The hive specific credential provider location is 
> specified by a new configuration property specific to hiveserver2 and the 
> password to job credential provider is provided using the environment variable
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/FileUtils.java 
> 3ed2d086fd8e14b9a70550c02082c1456f905a08 
>   common/src/java/org/apache/hadoop/hive/conf/Constants.java 
> 77c6aa5663c2c5358715801bc039c0fe8035e3dc 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 43a16d7fed1d38400793e7c96a7febebe42d351b 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 
> 16c2eafdb6888ed62168dd00f76335fa2484753b 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 
> cea9582c8ccb0c79700ac7913735d4cdf52f0c7e 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 
> 784b9c9fa769eeb088e3a6408a0b29147a8b4086 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java 
> c75333d7022f776aab184a4b804fd7ad7befac64 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/RemoteHiveSparkClient.java 
> a9f70c4100219c8929abd4e31b30d340e6ba98bd 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
> PRE-CREATION 
>   
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
> 936fdafdb37c461a7a5deb97cba72d4db54a49e1 
> 
> Diff: https://reviews.apache.org/r/52283/diff/
> 
> 
> Testing
> ---
> 
> Testing running multiple queries on S3 with keys stored in a credential 
> provider
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 52283: HIVE-14822 : Add support for credential provider for jobs launched from Hiveserver2

[Mohit Sabharwal]

 - 202)
<https://reviews.apache.org/r/52283/#comment220179>

Needs space after //

What happens to HIVE_SERVER2_JOB_CREDSTORE_LOCATION ? Does that need to be 
set or checked if it is set ? I mean, do both 
HIVE_SERVER2_JOB_CREDSTORE_PASSWORD_ENVVAR and  
HIVE_SERVER2_JOB_CREDSTORE_LOCATION need to be set for former to be used ?

It's worth clarifying here how Spark implementation differs from MR. I am 
assuming this step is in addition to the updateJobCredentialProviders 
invocation in Spark.



ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java 
(line 117)
<https://reviews.apache.org/r/52283/#comment220180>

//u -> // U...

same in other places



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(line 20)
<https://reviews.apache.org/r/52283/#comment220181>

expand all imports



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(lines 84 - 85)
<https://reviews.apache.org/r/52283/#comment220182>

nit: inline oldCredStoreLocation here and in other places



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(line 87)
<https://reviews.apache.org/r/52283/#comment220164>

Please limit lines to max 100 characters. 

Same in other places.



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(lines 90 - 91)
<https://reviews.apache.org/r/52283/#comment220183>

again, inlined value easier to read



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(line 103)
<https://reviews.apache.org/r/52283/#comment220184>

Are we testing the case where both HADOOP_CREDENTIAL_PASSWORD_ENVVAR and 
HIVE_JOB_CREDSTORE_PASSWORD_ENVVAR_VAL are not set ?



spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
(line 447)
<https://reviews.apache.org/r/52283/#comment220185>

// -> // A...



spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
(lines 450 - 454)
<https://reviews.apache.org/r/52283/#comment220190>

This pattern of choosing the correct password is repeated couple times in 
this patch. Should we write a single utility method for this ?


- Mohit Sabharwal


On Oct. 5, 2016, 9:58 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52283/
> ---
> 
> (Updated Oct. 5, 2016, 9:58 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Sergio Pena.
> 
> 
> Bugs: HIVE-14822
> https://issues.apache.org/jira/browse/HIVE-14822
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This change adds support for credential provider for jobs launched from 
> HiveServer2. It also adds support for job-specific credential provider and 
> password which is passed to the jobs via the job configs when they are 
> launched from HS2. The hive specific credential provider location is 
> specified by a new configuration property specific to hiveserver2 and the 
> password to job credential provider is provided using the environment variable
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/EnvironmentUtils.java 
> PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/FileUtils.java 
> 3ed2d086fd8e14b9a70550c02082c1456f905a08 
>   common/src/java/org/apache/hadoop/hive/conf/Constants.java 
> 77c6aa5663c2c5358715801bc039c0fe8035e3dc 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 43a16d7fed1d38400793e7c96a7febebe42d351b 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 
> 16c2eafdb6888ed62168dd00f76335fa2484753b 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 
> fd640567124e1bb8b558359732764a07c8b0f2ed 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 
> cea9582c8ccb0c79700ac7913735d4cdf52f0c7e 
>   
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 
> 784b9c9fa769eeb088e3a6408a0b29147a8b4086 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java 
> c75333d7022f776aab184a4b804fd7ad7befac64 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/RemoteHiveSparkClient.java 
> a9f70c4100219c8929abd4e31b30d340e6ba98bd 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
> PRE-CREATION 
>   
> spark-client/src/main/java/org/apache/hive/spark/client/SparkClientImpl.java 
> 936fdafdb37c461a7a5deb97cba72d4db54a49e1 
> 
> Diff: https://reviews.apache.org/r/52283/diff/
> 
> 
> Testing
> ---
> 
> Testing running multiple queries on S3 with keys stored in a credential 
> provider
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 52283: HIVE-14822 : Add support for credential provider for jobs launched from Hiveserver2

[Mohit Sabharwal]

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



Took a quick pass. Have some questions and suggestions.

Will take a another pass once those are addressed.


common/src/java/org/apache/hadoop/hive/conf/Constants.java (lines 34 - 35)
<https://reviews.apache.org/r/52283/#comment219968>

Please provide some hint in the name that this is storing name of an 
environment variable.

For example: using ENVVAR instead of NAME

HIVE_SERVER2_JOB_CREDSTORE_PASSWORD_ENVVAR
HADOOP_CREDENTIAL_PASSWORD_ENVVAR



common/src/java/org/apache/hadoop/hive/conf/Constants.java (line 36)
<https://reviews.apache.org/r/52283/#comment219969>

Same. This is name of config and not the path, right ?

HADOOP_CREDENTIAL_PROVIDER_PATH_CONFIG



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2522)
<https://reviews.apache.org/r/52283/#comment219970>

Does this work for Spark ? Tez ? Good to explicitly add all engines that 
are known to support this functionality. If an engine has not been tested, I 
think it's ok to state that "this functionality has not been tested against 
".



common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java (lines 136 - 137)
<https://reviews.apache.org/r/52283/#comment219972>

Worth adding why environment variables are used instead of hive configs.

Since this is the main method could you summarize everything you are doing ?

For example:

If the driver is configured with HIVE_SERVER2_JOB_CREDSTORE_LOCATION, 
AND 
HIVE_JOB_CREDSTORE_PASSWORD environment is set then:
(1) JobConf.MAPRED_MAP_TASK_ENV should contain 
HIVE_SERVER2_JOB_CREDSTORE_LOCATION
(2) Set hadoop.security.credential.provider.path to 
HIVE_SERVER2_JOB_CREDSTORE_LOCATION

What do you mean by HIVE_JOB_CREDSTORE_PASSWORD
or HADOOP_CREDSTORE_PASSWORD ? You mean you'll take either one if set ? 
What if both are set ?



ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java (line 422)
<https://reviews.apache.org/r/52283/#comment219705>

Check if string is empty?



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(lines 35 - 36)
<https://reviews.apache.org/r/52283/#comment219706>

nit: please prefix with word "test"

nit: HIVE_JOB_CREDSTORE_PASSWORD_ENVVAR_VAL
HADOOP_CREDSTORE_PASSWORD_ENVVAR_VAL



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(lines 81 - 82)
<https://reviews.apache.org/r/52283/#comment219703>

I would use assertEquals here and get rid of the message. It's making these 
statements difficult to read without adding much value. If this assert fails, 
we know exactly what happened.  

Same for all other instances in this file.



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(lines 88 - 94)
<https://reviews.apache.org/r/52283/#comment219975>

Are you testing the same thing twice ?



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(lines 109 - 112)
<https://reviews.apache.org/r/52283/#comment219977>

This is super unread-able... please make this into one line



ql/src/test/org/apache/hadoop/hive/ql/exec/TestHiveCredentialProviders.java 
(lines 333 - 335)
<https://reviews.apache.org/r/52283/#comment219974>

not need based on prior comment


- Mohit Sabharwal


On Sept. 30, 2016, 6:58 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52283/
> ---
> 
> (Updated Sept. 30, 2016, 6:58 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal and Sergio Pena.
> 
> 
> Bugs: HIVE-14822
> https://issues.apache.org/jira/browse/HIVE-14822
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This change adds support for credential provider for jobs launched from 
> HiveServer2. It also adds support for job-specific credential provider and 
> password which is passed to the jobs via the job configs when they are 
> launched from HS2. The hive specific credential provider location is 
> specified by a new configuration property specific to hiveserver2 and the 
> password to job credential provider is provided using the environment variable
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/EnvironmentUtils.java 
> PRE-CREATION 
>   common/src/java/org/apache/hadoop/hive/common/FileUtils.java 
> 3ed2d086fd8e14b9a70550c02082c1456f905a08 
>   

Re: Review Request 52084: HIVE-14775: Investigate IOException usage in Metrics APIs

[Mohit Sabharwal]

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




common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java (line 
93)
<https://reviews.apache.org/r/52084/#comment218598>

Log numCounter in the string so we know which counter value could not be 
found.

Same for other places where you're handling JMException and know what 
metric is getting logged.



common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java (line 
191)
<https://reviews.apache.org/r/52084/#comment218599>

same for this file.


- Mohit Sabharwal


On Sept. 21, 2016, 3:20 p.m., Barna Zsombor Klara wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52084/
> ---
> 
> (Updated Sept. 21, 2016, 3:20 p.m.)
> 
> 
> Review request for hive, Gabor Szadovszky, Peter Vary, Sergio Pena, and 
> Szehon Ho.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> I have refactored the exception handling in the metrics API. The initial 
> objective was to remove the throws clause from the methods which did not 
> throw IOExceptions but were declaring it. Finally I continued to remove the 
> throws clause from the rest of the APIs since we were never able to do 
> anything else with the excpetions except log them out. New log messages were 
> introduced inside the metrics classes to capture exceptional conditions so we 
> can omit the try catch blocks when creating/updating metrics.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/metrics/LegacyMetrics.java 
> 9be9b50aa02ff88816eb92079eaff9afa3e1be90 
>   common/src/java/org/apache/hadoop/hive/common/metrics/MetricsMBean.java 
> 19946d9e93d55bbffb6d03cbc35e569849a86dd8 
>   common/src/java/org/apache/hadoop/hive/common/metrics/MetricsMBeanImpl.java 
> a973155f079c6124a6981b04123d9496dc5d3448 
>   common/src/java/org/apache/hadoop/hive/common/metrics/common/Metrics.java 
> 4297233ed12a7d9a2fa03ac3204e8335c0aed821 
>   
> common/src/java/org/apache/hadoop/hive/common/metrics/metrics2/CodahaleMetrics.java
>  4c433678bd62ea74b80babce9856681192deb25f 
>   common/src/java/org/apache/hadoop/hive/ql/log/PerfLogger.java 
> 6a5d22f2bcc0a7efd869f7c0c0c8fad33ca35f74 
>   
> common/src/test/org/apache/hadoop/hive/common/metrics/TestLegacyMetrics.java 
> a3fb04f1ab9be8be9f69c616eabeb534b2a2d560 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HMSMetricsListener.java 
> 6830cf75dc163230cdabb4ed41d65c222c6ca54d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> f0b84768e891e4281a613e68590524646a038435 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> 42d398dcc9a37bdb2d90c940c579c55fb76b4cca 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 
> 90fe76d00e6f833e18d183c290f13c23db9303a1 
> 
> Diff: https://reviews.apache.org/r/52084/diff/
> 
> 
> Testing
> ---
> 
> Tested basic metric gathering with both codahale and legacy metrics class.
> Updated and ran unit tests in the common project.
> 
> 
> Thanks,
> 
> Barna Zsombor Klara
> 
>

Re: Review Request 50405: HIVE-14251: Union All of different types resolves to incorrect data

[Mohit Sabharwal]

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


Ship it!




Ship It!

- Mohit Sabharwal


On July 26, 2016, 3:13 p.m., Aihua Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50405/
> ---
> 
> (Updated July 26, 2016, 3:13 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14251: Union All of different types resolves to incorrect data
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 69a18cd 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestFunctionRegistry.java 
> 59ecd1e 
>   ql/src/test/queries/clientpositive/alter_partition_change_col.q 360f4d2 
>   ql/src/test/queries/clientpositive/alter_table_cascade.q acca4e8 
>   ql/src/test/queries/clientpositive/groupby_sort_1_23.q 67fdd23 
>   ql/src/test/queries/clientpositive/groupby_sort_skew_1_23.q 39b9420 
>   ql/src/test/queries/clientpositive/union32.q f47f0af 
>   ql/src/test/queries/clientpositive/union33.q 017442e 
>   ql/src/test/queries/clientpositive/union36.q c38e7b1 
>   ql/src/test/queries/clientpositive/unionDistinct_1.q fd7f075 
>   ql/src/test/queries/clientpositive/union_date_trim.q 6842e56 
>   ql/src/test/queries/clientpositive/union_null.q a17325c 
>   ql/src/test/queries/clientpositive/union_remove_12.q f6436f5 
>   ql/src/test/queries/clientpositive/union_remove_13.q b02451b 
>   ql/src/test/queries/clientpositive/union_remove_14.q bec6226 
>   ql/src/test/queries/clientpositive/union_type_chk.q 3b7b478 
>   ql/src/test/queries/clientpositive/unionall_join_nullconstant.q 4f0ffa6 
>   ql/src/test/results/clientpositive/alter_partition_change_col.q.out 23febee 
>   ql/src/test/results/clientpositive/alter_table_cascade.q.out 1d8204c 
>   ql/src/test/results/clientpositive/groupby_sort_1_23.q.out 81fe0d9 
>   ql/src/test/results/clientpositive/groupby_sort_skew_1_23.q.out 5cf0ea2 
>   ql/src/test/results/clientpositive/spark/groupby_sort_1_23.q.out 408c1b9 
>   ql/src/test/results/clientpositive/spark/groupby_sort_skew_1_23.q.out 
> 6325889 
>   ql/src/test/results/clientpositive/spark/union32.q.out 1ec7e64 
>   ql/src/test/results/clientpositive/spark/union33.q.out a61a8df 
>   ql/src/test/results/clientpositive/spark/union_date_trim.q.out 324e8b7 
>   ql/src/test/results/clientpositive/spark/union_null.q.out 32cdf65 
>   ql/src/test/results/clientpositive/spark/union_remove_12.q.out 94b4211 
>   ql/src/test/results/clientpositive/spark/union_remove_13.q.out 42aea66 
>   ql/src/test/results/clientpositive/spark/union_remove_14.q.out cf6d36f 
>   ql/src/test/results/clientpositive/tez/unionDistinct_1.q.out ee33086 
>   ql/src/test/results/clientpositive/tez/union_type_chk.q.out 12f060b 
>   ql/src/test/results/clientpositive/union32.q.out a3fefa8 
>   ql/src/test/results/clientpositive/union33.q.out a91e74c 
>   ql/src/test/results/clientpositive/union36.q.out 12f060b 
>   ql/src/test/results/clientpositive/unionDistinct_1.q.out 0330133 
>   ql/src/test/results/clientpositive/union_date_trim.q.out 324e8b7 
>   ql/src/test/results/clientpositive/union_null.q.out 32cdf65 
>   ql/src/test/results/clientpositive/union_remove_12.q.out 5f73c9a 
>   ql/src/test/results/clientpositive/union_remove_13.q.out c7063cd 
>   ql/src/test/results/clientpositive/union_remove_14.q.out 52dc7c5 
>   ql/src/test/results/clientpositive/union_type_chk.q.out 12f060b 
>   ql/src/test/results/clientpositive/unionall_join_nullconstant.q.out fca26b4 
>   serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoUtils.java 
> 8f7b799 
> 
> Diff: https://reviews.apache.org/r/50405/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aihua Xu
> 
>

Re: Review Request 50160: HIVE-14229: the jars in hive.aux.jar.paths are not added to session classpath

[Mohit Sabharwal]

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


Ship it!




Ship It!

- Mohit Sabharwal


On July 18, 2016, 8:41 p.m., Aihua Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50160/
> ---
> 
> (Updated July 18, 2016, 8:41 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14229: the jars in hive.aux.jar.paths are not added to session classpath
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 
> a376023860711964bfa6cf19578d09428b987bf1 
>   ql/src/java/org/apache/hadoop/hive/ql/processors/ReloadProcessor.java 
> 7a59833b2e3920cccdce9ae0c118bd54f85a737b 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> 03010ea5809037cbbc0b5338a3bca898440ca531 
>   ql/src/test/org/apache/hadoop/hive/ql/session/TestSessionState.java 
> 145a53167959edf10b982799b2e00c34606e95fa 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> eca02d903105efccfcd52231ebe32989eb672577 
> 
> Diff: https://reviews.apache.org/r/50160/diff/
> 
> 
> Testing
> ---
> 
> Testing has been done manually.
> 
> 
> Thanks,
> 
> Aihua Xu
> 
>

Re: Review Request 50073: HIVE-14198: Refactor aux jar related code to make them more consistency

[Mohit Sabharwal]

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


Ship it!




Ship It!

- Mohit Sabharwal


On July 15, 2016, 6:52 p.m., Aihua Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50073/
> ---
> 
> (Updated July 15, 2016, 6:52 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14198: Refactor aux jar related code to make them more consistency
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/FileUtils.java d755798 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 42f7d88 
>   common/src/test/org/apache/hadoop/hive/common/TestFileUtils.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 973bc23 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java 
> 0c0fe95 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/RemoteHiveSparkClient.java 
> 2b06a04 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkUtilities.java 
> 5a6bef9 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java f983b82 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java 69f5465 
> 
> Diff: https://reviews.apache.org/r/50073/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aihua Xu
> 
>

Re: Review Request 50073: HIVE-14198: Refactor aux jar related code to make them more consistency

[Mohit Sabharwal]

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



LGTM. Few minor comments.


common/src/java/org/apache/hadoop/hive/common/FileUtils.java (line 866)
<https://reviews.apache.org/r/50073/#comment207984>

It'd be great to document this default assumption in the 
HiveConf.HIVEAUXJARS config description.



common/src/test/org/apache/hadoop/hive/common/TestFileUtils.java (lines 21 - 22)
<https://reviews.apache.org/r/50073/#comment207975>

nit: extra lines



common/src/test/org/apache/hadoop/hive/common/TestFileUtils.java (line 42)
<https://reviews.apache.org/r/50073/#comment207973>

nit: rename f -> tmpDir for readability



common/src/test/org/apache/hadoop/hive/common/TestFileUtils.java (line 45)
<https://reviews.apache.org/r/50073/#comment207974>

nit: rename to jarFile1



ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java 
(line 156)
<https://reviews.apache.org/r/50073/#comment207977>

To be safe:
SessionState.get() == null ? null : 
SessionState.get().getReloadableAuxJars(); ?



ql/src/java/org/apache/hadoop/hive/ql/exec/spark/RemoteHiveSparkClient.java 
(line 215)
<https://reviews.apache.org/r/50073/#comment207983>

To be safe:
SessionState.get() == null ? null : 
SessionState.get().getReloadableAuxJars(); ?


- Mohit Sabharwal


On July 15, 2016, 11:07 a.m., Aihua Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50073/
> ---
> 
> (Updated July 15, 2016, 11:07 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14198: Refactor aux jar related code to make them more consistency
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/common/FileUtils.java 
> d755798e341651422aedcfead84d0af69b314734 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 42f7d8829211e34e25e7a9e45657220cd648cc61 
>   common/src/test/org/apache/hadoop/hive/common/TestFileUtils.java 
> PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 
> 973bc23fb7fef833b33aaaea26901d7cdd8d6598 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/LocalHiveSparkClient.java 
> 0c0fe9513925beadea9c530e5dc79a332454397f 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/RemoteHiveSparkClient.java 
> 2b06a04907cec5b8dce72db169127eef21ba4b7d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/spark/SparkUtilities.java 
> 5a6bef96480a34af236074dda554920dd1757796 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 
> f983b820429fba531e1b3158c8f58ed1a1c12800 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java 
> 69f54651746e80a01401a6de723fac5c751f425b 
> 
> Diff: https://reviews.apache.org/r/50073/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aihua Xu
> 
>

Re: Review Request 49919: HIVE-14135 : beeline output not formatted correctly for large column widths

[Mohit Sabharwal]

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


Ship it!




Ship It!

- Mohit Sabharwal


On July 14, 2016, 11:12 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49919/
> ---
> 
> (Updated July 14, 2016, 11:12 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and 
> Thejas Nair.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14135 : beeline output not formatted correctly for large column widths
> 
> 
> Diffs
> -
> 
>   beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c 
>   beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 
> 5aaa38527734d46de037352ff51e54e0ae1cede0 
>   beeline/src/java/org/apache/hive/beeline/BufferedRows.java 
> 962c5319bb7e6e448979e1cef80a086cadd2ecc6 
>   beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49919/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 49919: HIVE-14135 : beeline output not formatted correctly for large column widths

[Mohit Sabharwal]

> On July 14, 2016, 9:57 p.m., Vihang Karajgaonkar wrote:
> > beeline/src/java/org/apache/hive/beeline/BufferedRows.java, line 76
> > <https://reviews.apache.org/r/49919/diff/2/?file=1442484#file1442484line76>
> >
> > Thanks for reviewing. I thought of adding it in the above loop like you 
> > mentioned, but then went against that for readability reasons since 
> > max.length should not be too big (it is numbers of columns in a row). I 
> > will add it now in the loop.
> > 
> > btw, If we have to add it in the loop it should be 
> > Math.min(Math.max(max[j], row.sizes[j] + 1), maxColumnWidth); Did you 
> > mean the same?

Ah, right, that should me min not max.


- Mohit


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


On July 12, 2016, 5:51 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49919/
> -------
> 
> (Updated July 12, 2016, 5:51 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and 
> Thejas Nair.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14135 : beeline output not formatted correctly for large column widths
> 
> 
> Diffs
> -
> 
>   beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c 
>   beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 
> 5aaa38527734d46de037352ff51e54e0ae1cede0 
>   beeline/src/java/org/apache/hive/beeline/BufferedRows.java 
> 962c5319bb7e6e448979e1cef80a086cadd2ecc6 
>   beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49919/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 49919: HIVE-14135 : beeline output not formatted correctly for large column widths

[Mohit Sabharwal]

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




beeline/src/java/org/apache/hive/beeline/BufferedRows.java (line 76)
<https://reviews.apache.org/r/49919/#comment207854>

wondering if this logic can be folded into the prior loop itself ?
 max((max[j], row.sizes[j] + 1)),maxColumnWidth) ?



beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java (line 1)
<https://reviews.apache.org/r/49919/#comment207855>

Missing Apache header



beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java (line 16)
<https://reviews.apache.org/r/49919/#comment207856>

nit: extra line


- Mohit Sabharwal


On July 12, 2016, 5:51 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49919/
> ---
> 
> (Updated July 12, 2016, 5:51 p.m.)
> 
> 
> Review request for hive, Mohit Sabharwal, Sergio Pena, Sahil Takiar, and 
> Thejas Nair.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-14135 : beeline output not formatted correctly for large column widths
> 
> 
> Diffs
> -
> 
>   beeline/pom.xml a720d0835314221ec3bd9e8d354d148498ff794c 
>   beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java 
> 5aaa38527734d46de037352ff51e54e0ae1cede0 
>   beeline/src/java/org/apache/hive/beeline/BufferedRows.java 
> 962c5319bb7e6e448979e1cef80a086cadd2ecc6 
>   beeline/src/test/org/apache/hive/beeline/TestBufferedRows.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49919/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Review Request 49739: HIVE-14187 : JDOPersistenceManager objects remain cached if MetaStoreClient#close is not called

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

Instead of relying the client to call close, automatically perform RawStore 
related cleanup at the server end via 
deleteContext() which gets called when the server detects a lost/closed 
connection.


Diffs
-

  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
 641abab71ad51748689a78e1e9ccb41aacdb9ace 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestRemoteHiveMetaStore.java
 6da516558f6cf9a643d33f55cce31def9b7abc91 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyClient.java
 2c6d56782219fba26a878497860e2b5e047beafa 
  
itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestSetUGIOnOnlyServer.java
 6c3fbf62c927644087478c4c7cce26cc144da501 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
f45b90d2f6bc9d853b895a837938a15f7d45a1e1 
  metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java 
44d73d482263bc7f3c210503ba9440a6cd793a72 

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


Testing
---

Added unit test


Thanks,

Mohit Sabharwal

[jira] [Created] (HIVE-14187) JDOPersistenceManager objects remain cached if MetaStoreClient#close is not called

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-14187:
--

 Summary: JDOPersistenceManager objects remain cached if 
MetaStoreClient#close is not called
 Key: HIVE-14187
 URL: https://issues.apache.org/jira/browse/HIVE-14187
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


JDOPersistenceManager objects are cached in JDOPersistenceManagerFactory by 
DataNuclues.

A new JDOPersistenceManager object gets created for every HMS thread
local since ObjectStore is a thread local.

In non-embedded metastore mode, JDOPersistenceManager associated with a thread 
only gets cleaned up if IMetaStoreClient#close is called by the client (which 
calls ObjectStore#shutdown which calls JDOPersistenceManager#close which in 
turn removes the object from cache in 
JDOPersistenceManagerFactory#releasePersistenceManager
https://github.com/datanucleus/datanucleus-api-jdo/blob/master/src/main/java/org/datanucleus/api/jdo/JDOPersistenceManagerFactory.java#L1271),
 i.e. the object will remain cached if client does not call close.

For example: If one interrupts out of hive CLI shell (instead of using 'exit;' 
command), SessionState#close does not get called, and hence 
IMetaStoreClient#close does not get called.

Instead of relying the client to call close, it's cleaner to automatically 
perform RawStore related cleanup at the server end via deleteContext() which 
gets called when the server detects a lost/closed connection.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Mohit Sabharwal]

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




common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 781)
<https://reviews.apache.org/r/48233/#comment203288>

"to the metastore" -> "from the metastore"

"for a given table" instead of "for each partitioned table"



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 1328)
<https://reviews.apache.org/r/48233/#comment203289>

Add the @Deprecated annotation for this config.

For the sake of completeness, we can say "Please use 
ConfVars.METASTORE_LIMIT_PARTITION_REQUEST in the metastore instead."


- Mohit Sabharwal


On June 16, 2016, 4:04 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48233/
> ---
> 
> (Updated June 16, 2016, 4:04 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
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 761dbb279fb196e2bf1e0e59824827a4504eb136 
>   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 
> da188d33d6194740ba9ecb37a6e533ecf1ec6906 
>   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 
> 
> Diff: https://reviews.apache.org/r/48233/diff/
> 
> 
> Testing
> ---
> 
> Waiting for HiveQA.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 48233: HIVE-13884: Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Mohit Sabharwal]

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

[jira] [Created] (HIVE-13894) Fix more json related JDK8 test failures Part 2

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13894:
--

 Summary: Fix more json related JDK8 test failures Part 2
 Key: HIVE-13894
 URL: https://issues.apache.org/jira/browse/HIVE-13894
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


After merge of java8 branch to master, some more json ordering related failures 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-13884) Disallow queries fetching more than a configured number of partitions in PartitionPruner

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13884:
--

 Summary: Disallow queries fetching more than a configured number 
of partitions in PartitionPruner
 Key: HIVE-13884
 URL: https://issues.apache.org/jira/browse/HIVE-13884
 Project: Hive
  Issue Type: Improvement
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


Currently the PartitionPruner requests either all partitions or partitions 
based on filter expression. In either scenarios, if the number of partitions 
accessed is large there can be significant memory pressure at the HMS server 
end.

We already have a config {{hive.limit.query.max.table.partition}} that enforces 
limits on number of partitions that may be scanned per operator. But this check 
happens after the PartitionPruner has already fetched all partitions.

We should add an option at PartitionPruner level to disallow queries that 
attempt to access number of partitions beyond a configurable limit.

Note that {{hive.mapred.mode=strict}} disallow queries without a partition 
filter in PartitionPruner, but this check accepts any query with a pruning 
condition, even if partitions fetched are large. In multi-tenant environments, 
admins could use more control w.r.t. number of partitions allowed based on HMS 
memory capacity.

One option is to have PartitionPruner first fetch the partition names (instead 
of partition specs) and throw an exception if number of partitions exceeds the 
configured value. Otherwise, fetch the partition specs.

Looks like the existing {{listPartitionNames}} call could be used if extended 
to take partition filter expressions like {{getPartitionsByExpr}} call does.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: [discuss] jdk8 support

[Mohit Sabharwal]

Update on moving Hive2 tests to JDK8: I've addressed almost all test
failures in HIVE-13547 on java8 branch. There is one remaining
open item (HIVE-13834) which is currently assigned. Given current
state of flaky test runs, there might be few more.

I will work with Sergio to merge the test fixes to master and switch
the Hive2 pre-commit job to use JDK8, hopefully sometime tomorrow.

After Hive2 tests switch, if your patch sees ordering related test
failures in pre-commit run, it's likely JDK version related and you'll
need to build & re-run the test using JDK8. Number of such tests
should be relatively small.

On Tue, Apr 19, 2016 at 10:43 AM, Mohit Sabharwal 
wrote:

> Created HIVE-13547 to track switching 2x tests to JDK8.
>
> On Wed, Apr 13, 2016 at 10:02 AM, Sergio Pena 
> wrote:
>
>> I agree with such change as JDK7 is not longer supported.
>>
>> Changes on Jenkins and Hive PTest shouldn't be hard. We just need to
>> replace the path from java7 to java8. But I think we should fix all JDK8
>> issues or most of them before doing the change or we will end up having a
>> lot of failures on all JIRAs running pre-commit tests.
>>
>> +1 with the change.
>>
>> On Mon, Apr 11, 2016 at 9:34 PM, Siddharth Seth  wrote:
>>
>> > Option 3 sounds good. I'd ideally like to make JDK8 the minimum
>> requirement
>> > soon as well.
>> >
>> > On Mon, Apr 11, 2016 at 4:59 PM, Szehon Ho  wrote:
>> >
>> > > Sounds like a good plan, +1
>> > >
>> > > On Mon, Apr 11, 2016 at 4:31 PM, Mohit Sabharwal 
>> > > wrote:
>> > >
>> > > > Thanks, Ashutosh. Makes sense to keep the source and target as 1.7
>> > since
>> > > > we're not using any JDK8 specific features yet. So, option (3)
>> > > essentially
>> > > > just means using JDK8 exclusively to build & test Hive2.
>> > > >
>> > > > On Sat, Apr 9, 2016 at 12:23 PM, Ashutosh Chauhan <
>> > hashut...@apache.org>
>> > > > wrote:
>> > > >
>> > > > > Hi Mohit,
>> > > > >
>> > > > > I also think option 3 makes sense. We should strive to keep test
>> > matrix
>> > > > > small so that we can do fast QA runs for dev patches.
>> > > > > We can just use jdk7 to build & test Hive1 and jdk8 to build &
>> test
>> > > > Hive2.
>> > > > > However, I am not sure of explicitly dropping support altogether
>> for
>> > > jdk7
>> > > > > on Hive2. We should make sure that in pom.xml java source & target
>> > > > > compatibility is still 1.7 (which already is the case currently)
>> so
>> > > that
>> > > > > Hive2 is still compatible with jdk7. Unit tests as I said we can
>> run
>> > on
>> > > > > jdk8.
>> > > > >
>> > > > > Thanks,
>> > > > > Ashutosh
>> > > > >
>> > > > > On Fri, Apr 8, 2016 at 4:34 PM, Mohit Sabharwal <
>> mo...@cloudera.com>
>> > > > > wrote:
>> > > > >
>> > > > > > Hi folks,
>> > > > > >
>> > > > > > Oracle EOL'ed (ended public updates) for Java 7 in April 2015.
>> > > > > > In order to support Java 8, we fixed quite a bunch of tests in
>> > > > > > HIVE-8607 (*) early last year. However, since our pre-commit
>> tests
>> > > run
>> > > > > > on JDK7 only, any JDK8 test failures are getting ignored. As a
>> > > result,
>> > > > > > the count has crept up
>> > > > > > <
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/HIVE-TRUNK-JAVA8/
>> > > > > > >
>> > > > > > back
>> > > > > > from zero to 125.
>> > > > > >
>> > > > > > Some options to address this:
>> > > > > >
>> > > > > > (1) Run pre-commit tests on both JDK7 and JDK8, in both 1.x
>> > > > > > and 2.x: This will further slow down the pre-commit run.
>> > > > > >
>> > > > > > (2) Alternate pre-commit test runs between JDK7 and JDK8 (in
>> > > > > > both 1.x and 2.x): It's a cheap h

[jira] [Created] (HIVE-13860) Fix more json related JDK8 test failures

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13860:
--

 Summary: Fix more json related JDK8 test failures
 Key: HIVE-13860
 URL: https://issues.apache.org/jira/browse/HIVE-13860
 Project: Hive
  Issue Type: Sub-task
  Components: Test
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal






--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

[Mohit Sabharwal]

e is a advantage in this use case, let's maintain the pattern. (Just 
like the queueConfig != null check elsewhere)

Same for other places where defaultString() is used.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 117)
<https://reviews.apache.org/r/47040/#comment197089>

wait, refreshDefaultQueue() just calls attemptSetScheduleForUser().

Can you just inline attemptSetScheduleForUser here with 
YarnConfiguration.DEFAULT_QUEUE_NAME param ? It's cleaner without the extra 
indirection.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 121)
<https://reviews.apache.org/r/47040/#comment197087>

Call this setJobQueueForUserInternal ?

attemptSetScheduleForUser seems confusing to me.
I thought we're just setting job queues and Yarn does the scheduling...

configuration -> conf



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 124)
<https://reviews.apache.org/r/47040/#comment197085>

remove extra line



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 125)
<https://reviews.apache.org/r/47040/#comment197086>

nit: cleaner with
if (queueConfig == null)
  return;



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 130)
<https://reviews.apache.org/r/47040/#comment197091>

We should make this LOG.info. There is nothing warn about here ...



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 137)
<https://reviews.apache.org/r/47040/#comment197079>

config for consistency



shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
 (line 1)
<https://reviews.apache.org/r/47040/#comment197105>

Add Apache License.


- Mohit Sabharwal


On May 11, 2016, 8:02 p.m., Reuben Kuhnert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47040/
> ---
> 
> (Updated May 11, 2016, 8:02 p.m.)
> 
> 
> Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: HIVE-13696
> https://issues.apache.org/jira/browse/HIVE-13696
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Ensure that jobs sent to YARN with impersonation off are correctly routed to 
> the proper queue based on fair-scheduler.xml. Monitor this file for changes 
> and validate that jobs can only be sent to queues authorized for the user.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> 3fecc5c4ca2a06a031c0c4a711fb49e757c49062 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 
> 926f6e883030b5a01d025994bd02c67f0f5a275c 
>   ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> a0015ebc655931f241b28c53fbb94cfe172841b1 
>   shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java 
> PRE-CREATION 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/SchedulerShim.java 
> 63803b8b0752745bd2fedaccc5d100befd97093b 
>   shims/scheduler/pom.xml b36c12325c588cdb609c6200b1edef73a2f79552 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
>  372244dc3c989d2a3ae2eb2bfb8cd0a235705e18 
>   
> shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
>  PRE-CREATION 
>   
> shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47040/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Reuben Kuhnert
> 
>

Re: Review Request 47204: HIVE-13549 : Remove jdk version specific out files from Hive2

[Mohit Sabharwal]

ava1.8.out 
10d780200944dd96052c8ef989a211e225dfc4f0 
  ql/src/test/results/clientpositive/tez/join0.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/tez/vector_cast_constant.q.java1.7.out 
420e7881636338d1a829d4c2609498e6864cc23d 
  ql/src/test/results/clientpositive/tez/vector_cast_constant.q.java1.8.out 
331edd0296c8f8d94b2ed88b3d9867eb2feb4995 
  ql/src/test/results/clientpositive/tez/vector_cast_constant.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/varchar_udf1.q.java1.7.out 
459d93b99d7c57108945db7d95bd612e33029900 
  ql/src/test/results/clientpositive/varchar_udf1.q.java1.8.out 
ace85682bd9fe659e475464330a58d2e0b38ef74 
  ql/src/test/results/clientpositive/varchar_udf1.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/vector_cast_constant.q.java1.7.out 
867dd4c96b9cd88a1fe6369f6307ab2d95ce5a84 
  ql/src/test/results/clientpositive/vector_cast_constant.q.java1.8.out 
789e6c2b82ffa473f2c8fd29ee9568f83243c2bf 
  ql/src/test/results/clientpositive/vector_cast_constant.q.out 
39ed1c85839c5b567bca603d335bf947b0a03b7f 

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


Testing
---


Thanks,

Mohit Sabharwal

Re: Review Request 47040: Monitor changes to FairScheduler.xml file and automatically update / validate jobs submitted to fair-scheduler

[Mohit Sabharwal]

 synchronized at all ? The event is not shared state, right ? 
Are we mutating any shared state in this method ?

Also, make it private ?



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 (line 1)
<https://reviews.apache.org/r/47040/#comment196844>

Missing Apache License Header



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 (line 11)
<https://reviews.apache.org/r/47040/#comment196845>

Remove and add javadoc relevant to the implementation.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 (line 29)
<https://reviews.apache.org/r/47040/#comment196846>

throw new RuntimeException

The exception could have been thrown for any unknown reason.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerQueueAllocator.java
 (line 41)
<https://reviews.apache.org/r/47040/#comment196847>

remove empty line.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 50)
<https://reviews.apache.org/r/47040/#comment196850>

why synchronized ?



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 53)
<https://reviews.apache.org/r/47040/#comment196849>

Change it to (Exception ex)



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 55)
<https://reviews.apache.org/r/47040/#comment196848>

Log exception in addition to the message.

i.e.

LOG.error(messge, ex)



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 60)
<https://reviews.apache.org/r/47040/#comment196851>

why synchronized ? Didn't see shared share getting modified anywhere...



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 114)
<https://reviews.apache.org/r/47040/#comment196852>

Important enough that this should be INFO level ?



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 121)
<https://reviews.apache.org/r/47040/#comment196853>

rename to getConfigForUser(...)

Also, not quiet sure why this needs to be synchronized.



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 134)
<https://reviews.apache.org/r/47040/#comment196854>

Instead of String.format, you can use slf4j.Logger format ("{}").

Log the exception as well, i.e. Log.warn(msg, ex)



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 143)
<https://reviews.apache.org/r/47040/#comment196855>

Instead of String.format, you can use slf4j.Logger format ("{}").

Log the exception as well, i.e. Log.warn(msg, ex)



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/FairSchedulerShim.java
 (line 163)
<https://reviews.apache.org/r/47040/#comment196856>

convert to javadoc



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 (line 1)
<https://reviews.apache.org/r/47040/#comment196857>

Missing Apache License Header



shims/scheduler/src/main/java/org/apache/hadoop/hive/schshim/QueueAllocator.java
 (line 10)
<https://reviews.apache.org/r/47040/#comment196858>

Remove and add relevant javadoc



shims/scheduler/src/test/java/org/apache/hadoop/hive/schshim/TestFairScheduler.java
 (line 1)
<https://reviews.apache.org/r/47040/#comment196859>

Missing Apache header


- Mohit Sabharwal


On May 10, 2016, 10:54 p.m., Reuben Kuhnert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47040/
> ---
> 
> (Updated May 10, 2016, 10:54 p.m.)
> 
> 
> Review request for hive, Lenni Kuff, Mohit Sabharwal, and Sergio Pena.
> 
> 
> Bugs: HIVE-13696
> https://issues.apache.org/jira/browse/HIVE-13696
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Ensure that jobs sent to YARN with impersonation off are correctly routed to 
> the proper queue based on fair-scheduler.xml. Monitor this file for changes 
> and validate that jobs can only be sent to queues authorized for the user.
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java 926f6e8 
>   ql/src/java/org/apache/hadoop/hive/ql/session/YarnFairScheduling.java 
> PRE-CREATION 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> a0015eb 
>   shims/common/src/main/java/org/apache/hadoop/fs/FileSystemWatcher.java 
> PRE-CREATION 
>   shims/common/src/main/java/org/apache/hadoop/hive/shi

Review Request 47204: HIVE-13549 : Remove jdk version specific out files from Hive2

[Mohit Sabharwal]

E-CREATION 
  ql/src/test/results/clientpositive/tez/vector_cast_constant.q.java1.7.out 
420e7881636338d1a829d4c2609498e6864cc23d 
  ql/src/test/results/clientpositive/tez/vector_cast_constant.q.java1.8.out 
331edd0296c8f8d94b2ed88b3d9867eb2feb4995 
  ql/src/test/results/clientpositive/tez/vector_cast_constant.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/varchar_udf1.q.java1.7.out 
459d93b99d7c57108945db7d95bd612e33029900 
  ql/src/test/results/clientpositive/varchar_udf1.q.java1.8.out 
ace85682bd9fe659e475464330a58d2e0b38ef74 
  ql/src/test/results/clientpositive/varchar_udf1.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/vector_cast_constant.q.java1.7.out 
867dd4c96b9cd88a1fe6369f6307ab2d95ce5a84 
  ql/src/test/results/clientpositive/vector_cast_constant.q.java1.8.out 
789e6c2b82ffa473f2c8fd29ee9568f83243c2bf 
  ql/src/test/results/clientpositive/vector_cast_constant.q.out 
39ed1c85839c5b567bca603d335bf947b0a03b7f 

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


Testing
---


Thanks,

Mohit Sabharwal

[jira] [Created] (HIVE-13657) Spark driver stderr logs should appear in hive client logs

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13657:
--

 Summary: Spark driver stderr logs should appear in hive client logs
 Key: HIVE-13657
 URL: https://issues.apache.org/jira/browse/HIVE-13657
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


Currently, spark driver exceptions are not getting logged in beeline. Instead, 
the users sees the not-so-useful: 
{code}
ERROR : Failed to execute spark task, with exception 
'org.apache.hadoop.hive.ql.metadata.HiveException(Failed to create spark 
client.)'

{code}

The user has to look at HS2 logs to discover the root cause:
{code}
2015-04-01 11:33:16,048 INFO org.apache.hive.spark.client.SparkClientImpl: 
15/04/01 11:33:16 WARN UserGroupInformation: PriviledgedActionException as:foo 
(auth:PROXY) via hive (auth:SIMPLE) 
cause:org.apache.hadoop.security.AccessControlException: Permission denied: 
user=foo, access=WRITE, inode="/user":hdfs:supergroup:drwxr-xr-x
...
{code}

We should surface these critical errors in hive client.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-13587) Set Hive pom to use Hadoop 2.6.1

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13587:
--

 Summary: Set Hive pom to use Hadoop 2.6.1
 Key: HIVE-13587
 URL: https://issues.apache.org/jira/browse/HIVE-13587
 Project: Hive
  Issue Type: Sub-task
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


A number of the JDK8 unit test failures are due to HADOOP-10786, fixed in 
Hadoop 2.6.1. 

* TestMiniHiveKdc.testLogin
* TestHiveAuthFactory.testStartTokenManagerForDBTokenStore
* TestHiveAuthFactory.testStartTokenManagerForMemoryTokenStore

i.e. Hive under Kerberos is broken in Java8 unless we move dependency to hadoop 
2.6.1



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-13585) Add counter metric for direct sql failures

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13585:
--

 Summary: Add counter metric for direct sql failures
 Key: HIVE-13585
 URL: https://issues.apache.org/jira/browse/HIVE-13585
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


In case of direct sql failure, metastore query falls back to DataNucleus. It'd 
be good to record how often this happens as a metrics counter.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-13549) Remove jdk version specific out files from Hive2

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13549:
--

 Summary: Remove jdk version specific out files from Hive2
 Key: HIVE-13549
 URL: https://issues.apache.org/jira/browse/HIVE-13549
 Project: Hive
  Issue Type: Sub-task
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal






--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: [discuss] jdk8 support

[Mohit Sabharwal]

Created HIVE-13547 to track switching 2x tests to JDK8.

On Wed, Apr 13, 2016 at 10:02 AM, Sergio Pena 
wrote:

> I agree with such change as JDK7 is not longer supported.
>
> Changes on Jenkins and Hive PTest shouldn't be hard. We just need to
> replace the path from java7 to java8. But I think we should fix all JDK8
> issues or most of them before doing the change or we will end up having a
> lot of failures on all JIRAs running pre-commit tests.
>
> +1 with the change.
>
> On Mon, Apr 11, 2016 at 9:34 PM, Siddharth Seth  wrote:
>
> > Option 3 sounds good. I'd ideally like to make JDK8 the minimum
> requirement
> > soon as well.
> >
> > On Mon, Apr 11, 2016 at 4:59 PM, Szehon Ho  wrote:
> >
> > > Sounds like a good plan, +1
> > >
> > > On Mon, Apr 11, 2016 at 4:31 PM, Mohit Sabharwal 
> > > wrote:
> > >
> > > > Thanks, Ashutosh. Makes sense to keep the source and target as 1.7
> > since
> > > > we're not using any JDK8 specific features yet. So, option (3)
> > > essentially
> > > > just means using JDK8 exclusively to build & test Hive2.
> > > >
> > > > On Sat, Apr 9, 2016 at 12:23 PM, Ashutosh Chauhan <
> > hashut...@apache.org>
> > > > wrote:
> > > >
> > > > > Hi Mohit,
> > > > >
> > > > > I also think option 3 makes sense. We should strive to keep test
> > matrix
> > > > > small so that we can do fast QA runs for dev patches.
> > > > > We can just use jdk7 to build & test Hive1 and jdk8 to build & test
> > > > Hive2.
> > > > > However, I am not sure of explicitly dropping support altogether
> for
> > > jdk7
> > > > > on Hive2. We should make sure that in pom.xml java source & target
> > > > > compatibility is still 1.7 (which already is the case currently) so
> > > that
> > > > > Hive2 is still compatible with jdk7. Unit tests as I said we can
> run
> > on
> > > > > jdk8.
> > > > >
> > > > > Thanks,
> > > > > Ashutosh
> > > > >
> > > > > On Fri, Apr 8, 2016 at 4:34 PM, Mohit Sabharwal <
> mo...@cloudera.com>
> > > > > wrote:
> > > > >
> > > > > > Hi folks,
> > > > > >
> > > > > > Oracle EOL'ed (ended public updates) for Java 7 in April 2015.
> > > > > > In order to support Java 8, we fixed quite a bunch of tests in
> > > > > > HIVE-8607 (*) early last year. However, since our pre-commit
> tests
> > > run
> > > > > > on JDK7 only, any JDK8 test failures are getting ignored. As a
> > > result,
> > > > > > the count has crept up
> > > > > > <
> > > > > >
> > > > >
> > > >
> > >
> >
> http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/HIVE-TRUNK-JAVA8/
> > > > > > >
> > > > > > back
> > > > > > from zero to 125.
> > > > > >
> > > > > > Some options to address this:
> > > > > >
> > > > > > (1) Run pre-commit tests on both JDK7 and JDK8, in both 1.x
> > > > > > and 2.x: This will further slow down the pre-commit run.
> > > > > >
> > > > > > (2) Alternate pre-commit test runs between JDK7 and JDK8 (in
> > > > > > both 1.x and 2.x): It's a cheap hack. More messy, though failures
> > > > > > won't get ignored.
> > > > > >
> > > > > > (3) Drop support for JDK7 in Hive 2.x, i.e. build and test Hive
> 2.x
> > > on
> > > > > > JDK8 only. For Hive 1.x, continue running JDK7 pre-commit tests.
> > > > > > The pros are:
> > > > > > - Simple test matrix.
> > > > > > - No need to slow down test run or maintain version specific
> golden
> > > > files
> > > > > > (**).
> > > > > > - 2.x looks like the logical place to move to JDK8.
> > > > > > - Users transitioning to JDK8 for all other services do not have
> to
> > > > > > maintain
> > > > > > multiple java versions on the cluster.
> > > > > >
> > > > > > Option (3) looks most attractive to me.
> > > > > >
> > > > > > Moving to JDK8 also lines us up better for Java 9, which is on
> the
> > > > > > horizon (Oracle will end public updates for Java 8 in Sep 2017)
> > > > > >
> > > > > > Around 100 of the latest crop of failures are due to one cause
> > > > > > (HIVE-13409).
> > > > > > I can take a pass at triaging the rest if there is consensus
> around
> > > > what
> > > > > > to do overall.
> > > > > >
> > > > > > Thanks,
> > > > > > Mohit
> > > > > >
> > > > > > (*) Most test failures are due to JDK8 using a different hash
> > > function
> > > > > for
> > > > > > HashMap  compared to JDK7. This results in (mostly benign, but
> hard
> > > > > > to debug) ordering differences in q-file output related to
> > > > serialization
> > > > > > order of map entries, numbering of stages in query plan, etc.
> > > > > >
> > > > > > (**) In some cases, hash function related ordering differences
> > > > originate
> > > > > > in external libraries like Avro, antlr, json ,Thrift's map, etc.
> > for
> > > > > which
> > > > > > code
> > > > > > changes are either more involved or led to more test failures.
> To
> > > > > address
> > > > > > this,
> > > > > > we added support for version specific golden files (HIVE-9109).
> > > > > Currently,
> > > > > > there
> > > > > > are ~40 golden files with ".java1.8.out" extension.
> > > > > >
> > > > >
> > > >
> > >
> >
>

[jira] [Created] (HIVE-13547) Switch Hive2 tests to JDK8

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13547:
--

 Summary: Switch Hive2 tests to JDK8
 Key: HIVE-13547
 URL: https://issues.apache.org/jira/browse/HIVE-13547
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


Tracks switching Hive2 tests to JDK8 as discussed on dev list:
http://mail-archives.apache.org/mod_mbox/hive-dev/201604.mbox/%3ccabjbx5mckfczpxpkr9kvkbaenhwvymbu_mwyfmgfs9snzni...@mail.gmail.com%3E



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: [discuss] jdk8 support

[Mohit Sabharwal]

Thanks, Ashutosh. Makes sense to keep the source and target as 1.7 since
we're not using any JDK8 specific features yet. So, option (3) essentially
just means using JDK8 exclusively to build & test Hive2.

On Sat, Apr 9, 2016 at 12:23 PM, Ashutosh Chauhan 
wrote:

> Hi Mohit,
>
> I also think option 3 makes sense. We should strive to keep test matrix
> small so that we can do fast QA runs for dev patches.
> We can just use jdk7 to build & test Hive1 and jdk8 to build & test Hive2.
> However, I am not sure of explicitly dropping support altogether for jdk7
> on Hive2. We should make sure that in pom.xml java source & target
> compatibility is still 1.7 (which already is the case currently) so that
> Hive2 is still compatible with jdk7. Unit tests as I said we can run on
> jdk8.
>
> Thanks,
> Ashutosh
>
> On Fri, Apr 8, 2016 at 4:34 PM, Mohit Sabharwal 
> wrote:
>
> > Hi folks,
> >
> > Oracle EOL'ed (ended public updates) for Java 7 in April 2015.
> > In order to support Java 8, we fixed quite a bunch of tests in
> > HIVE-8607 (*) early last year. However, since our pre-commit tests run
> > on JDK7 only, any JDK8 test failures are getting ignored. As a result,
> > the count has crept up
> > <
> >
> http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/HIVE-TRUNK-JAVA8/
> > >
> > back
> > from zero to 125.
> >
> > Some options to address this:
> >
> > (1) Run pre-commit tests on both JDK7 and JDK8, in both 1.x
> > and 2.x: This will further slow down the pre-commit run.
> >
> > (2) Alternate pre-commit test runs between JDK7 and JDK8 (in
> > both 1.x and 2.x): It's a cheap hack. More messy, though failures
> > won't get ignored.
> >
> > (3) Drop support for JDK7 in Hive 2.x, i.e. build and test Hive 2.x on
> > JDK8 only. For Hive 1.x, continue running JDK7 pre-commit tests.
> > The pros are:
> > - Simple test matrix.
> > - No need to slow down test run or maintain version specific golden files
> > (**).
> > - 2.x looks like the logical place to move to JDK8.
> > - Users transitioning to JDK8 for all other services do not have to
> > maintain
> > multiple java versions on the cluster.
> >
> > Option (3) looks most attractive to me.
> >
> > Moving to JDK8 also lines us up better for Java 9, which is on the
> > horizon (Oracle will end public updates for Java 8 in Sep 2017)
> >
> > Around 100 of the latest crop of failures are due to one cause
> > (HIVE-13409).
> > I can take a pass at triaging the rest if there is consensus around what
> > to do overall.
> >
> > Thanks,
> > Mohit
> >
> > (*) Most test failures are due to JDK8 using a different hash function
> for
> > HashMap  compared to JDK7. This results in (mostly benign, but hard
> > to debug) ordering differences in q-file output related to serialization
> > order of map entries, numbering of stages in query plan, etc.
> >
> > (**) In some cases, hash function related ordering differences originate
> > in external libraries like Avro, antlr, json ,Thrift's map, etc. for
> which
> > code
> > changes are either more involved or led to more test failures.  To
> address
> > this,
> > we added support for version specific golden files (HIVE-9109).
> Currently,
> > there
> > are ~40 golden files with ".java1.8.out" extension.
> >
>

[discuss] jdk8 support

[Mohit Sabharwal]

Hi folks,

Oracle EOL'ed (ended public updates) for Java 7 in April 2015.
In order to support Java 8, we fixed quite a bunch of tests in
HIVE-8607 (*) early last year. However, since our pre-commit tests run
on JDK7 only, any JDK8 test failures are getting ignored. As a result,
the count has crept up

back
from zero to 125.

Some options to address this:

(1) Run pre-commit tests on both JDK7 and JDK8, in both 1.x
and 2.x: This will further slow down the pre-commit run.

(2) Alternate pre-commit test runs between JDK7 and JDK8 (in
both 1.x and 2.x): It's a cheap hack. More messy, though failures
won't get ignored.

(3) Drop support for JDK7 in Hive 2.x, i.e. build and test Hive 2.x on
JDK8 only. For Hive 1.x, continue running JDK7 pre-commit tests.
The pros are:
- Simple test matrix.
- No need to slow down test run or maintain version specific golden files
(**).
- 2.x looks like the logical place to move to JDK8.
- Users transitioning to JDK8 for all other services do not have to
maintain
multiple java versions on the cluster.

Option (3) looks most attractive to me.

Moving to JDK8 also lines us up better for Java 9, which is on the
horizon (Oracle will end public updates for Java 8 in Sep 2017)

Around 100 of the latest crop of failures are due to one cause
(HIVE-13409).
I can take a pass at triaging the rest if there is consensus around what
to do overall.

Thanks,
Mohit

(*) Most test failures are due to JDK8 using a different hash function for
HashMap  compared to JDK7. This results in (mostly benign, but hard
to debug) ordering differences in q-file output related to serialization
order of map entries, numbering of stages in query plan, etc.

(**) In some cases, hash function related ordering differences originate
in external libraries like Avro, antlr, json ,Thrift's map, etc. for which
code
changes are either more involved or led to more test failures.  To address
this,
we added support for version specific golden files (HIVE-9109). Currently,
there
are ~40 golden files with ".java1.8.out" extension.

[jira] [Created] (HIVE-13409) Fix JDK8 test failures related to COLUMN_STATS_ACCURATE

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13409:
--

 Summary: Fix JDK8 test failures related to COLUMN_STATS_ACCURATE
 Key: HIVE-13409
 URL: https://issues.apache.org/jira/browse/HIVE-13409
 Project: Hive
  Issue Type: Bug
  Components: Tests
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


126 failures have crept into JDK8 tests since we resolved HIVE-8607

http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/HIVE-TRUNK-JAVA8/

Majority relate to the ordering of a "COLUMN_STATS_ACCURATE" partition property.

Looks like a simple fix, use ordered map in 
HiveStringUtils.getPropertiesExplain()



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Review Request 45525: HIVE-12484 : Show meta operations on HS2 web UI

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

HIVE-12484 : Show meta operations on HS2 web UI

Expands display availablity to meta operations
as well as "set" ("command") operations. 

Moves OperationDisplay to Operation level.

Added getDisplayQuery() method for all meta 
operations to specify the query string to 
display in the UI.


Diffs
-

  
itests/hive-unit/src/test/java/org/apache/hive/service/cli/session/TestQueryDisplay.java
 418f71eb87cdd519677b2f5a59c67099f704ec80 
  ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java 
d582bc063fc150002a01d63451ae6632fca29ac1 
  service/src/jamon/org/apache/hive/tmpl/QueryProfileTmpl.jamon 
8d51a73f7ac6c5eb5fe44c5525ed840b641c5c6b 
  
service/src/java/org/apache/hive/service/cli/operation/GetCatalogsOperation.java
 8868ec18e0f50f6a434eb0498e4a923b0a151b97 
  
service/src/java/org/apache/hive/service/cli/operation/GetColumnsOperation.java 
35b6c5209025a0e9b5c3732493afd9d30d4f4a1f 
  
service/src/java/org/apache/hive/service/cli/operation/GetFunctionsOperation.java
 8db2e62604345a2d3b9193c497080f3f2977f2ac 
  
service/src/java/org/apache/hive/service/cli/operation/GetSchemasOperation.java 
d6f6280f1c398fa0239cb52070ff8c40168215bf 
  
service/src/java/org/apache/hive/service/cli/operation/GetTableTypesOperation.java
 a09b39a4e0855152925085e46b2e0a703b32de7f 
  
service/src/java/org/apache/hive/service/cli/operation/GetTablesOperation.java 
740b85158a6008ecece592d72cd743d8f5a433f4 
  
service/src/java/org/apache/hive/service/cli/operation/GetTypeInfoOperation.java
 2a0fec27715daba4307f661a6f65c3cd4d43c975 
  
service/src/java/org/apache/hive/service/cli/operation/HiveCommandOperation.java
 f5a9771449a7cc1fb5ac5c0b6f2fd6ab032a2162 
  service/src/java/org/apache/hive/service/cli/operation/MetadataOperation.java 
285b4f94ec828a357da4c2e7dc96c64bd459ff80 
  service/src/java/org/apache/hive/service/cli/operation/Operation.java 
d9a273b1b95adf7a6a68fa013042da2408904d64 
  service/src/java/org/apache/hive/service/cli/operation/OperationDisplay.java 
PRE-CREATION 
  
service/src/java/org/apache/hive/service/cli/operation/OperationDisplayCache.java
 PRE-CREATION 
  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
56a9c18bc118b8d67f87e5500ec7d4442a90a869 
  service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 
04d816a72afcadbff26f5eb5d68003a7344e31c9 
  
service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplay.java 
d2ca1e7b79e9722ad0ce990e96bf3fc9297ca7fd 
  
service/src/java/org/apache/hive/service/cli/operation/SQLOperationDisplayCache.java
 4a33d37a88a7216d832a5b9ab4b7534091129be7 
  service/src/java/org/apache/hive/service/servlet/QueryProfileServlet.java 
8fa447a386c5a7a000b04453618f502e1681364e 
  service/src/resources/hive-webapps/hiveserver2/hiveserver2.jsp 
8b46550b603a51ba6143c36746029475d7e11a40 

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


Testing
---

Tested with different sql, metadata and set operations.
Added tests to TestQueryDisplay


Thanks,

Mohit Sabharwal

Review Request 43785: HIVE-13099: Non-SQLOperations lead to Web UI NPE

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

To support display of live operations in the WebUI, we record SQLOperations (in 
{{liveSqlOperations}}).

However, to support historic operations, we save all operations in 
{{historicSqlOperations}}, including non-SQLOperations which do not have 
display entries in liveSqlOperations.

This leads to a race condition depending on whether sessions use non-sql 
operations. Reproduce-able by issuing a 'set' operation.


Diffs
-

  service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
96c01de40fa581f1583fc3f9a93a2c71355ebc04 

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


Testing
---


Thanks,

Mohit Sabharwal

[jira] [Created] (HIVE-13099) Non-SQLOperations lead to Web UI NPE

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13099:
--

 Summary: Non-SQLOperations lead to Web UI NPE
 Key: HIVE-13099
 URL: https://issues.apache.org/jira/browse/HIVE-13099
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


To support display of live operations in the WebUI, we record SQLOperations (in 
{{liveSqlOperations}}). 

However, to support historic operations, we save all operations in 
{{historicSqlOperations}}, including non-SQLOperations which do not have 
display entries in liveSqlOperations.

This leads to a race condition depending on whether sessions use non-sql 
operations. Reproduce-able by issuing a 'set' operation.
{code}
java.lang.NullPointerException
at 
org.apache.hive.generated.hiveserver2.hiveserver2_jsp._jspService(hiveserver2_jsp.java:131)
at org.apache.jasper.runtime.HttpJspBase.service(HttpJspBase.java:98)
{code}

We should save only SQLOperations in historicSqlOperations.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Review Request 43553: HIVE-13057: Remove duplicate copies of TableDesc property values in PartitionDesc

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

For a partitioned table, each PartitionDesc has a copy of corresponding 
TableDesc.

While TableDesc is mutable and hence cannot be interned, it's property values 
can be.

For a simple select on a table with 100K partitions, this cut total number of 
String instances by ~65%.

Most replicated strings were location, serde, input/output format, column, 
types, table name, etc.


Diffs
-

  ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java 
b032349b7faf7026928bea90a6ded29eeb2a502c 

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


Testing
---


Thanks,

Mohit Sabharwal

[jira] [Created] (HIVE-13057) Remove duplicate copies of TableDesc property values in PartitionDesc

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13057:
--

 Summary: Remove duplicate copies of TableDesc property values in 
PartitionDesc
 Key: HIVE-13057
 URL: https://issues.apache.org/jira/browse/HIVE-13057
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


For a partitioned table, each PartitionDesc has a copy of corresponding 
TableDesc.

While TableDesc is mutable and hence cannot be interned, it's property values 
can be.

For a simple select on a table with 100K partitions, this cut total number of 
String instances by ~65%.

Most replicated strings were location, serde, input/output format, column, 
types, table name, etc.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-13047) Disabling Web UI leads to NullPointerException

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13047:
--

 Summary: Disabling Web UI leads to NullPointerException
 Key: HIVE-13047
 URL: https://issues.apache.org/jira/browse/HIVE-13047
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


Disabling the Web UI or it's historical query display feature can lead to 
NullPointerException since {{historicSqlOperations}} is unintialized.

For ex: If hive.server2.webui.port is set to 0

{code}
Caused by: java.lang.NullPointerException: null
at 
org.apache.hive.service.cli.operation.OperationManager.removeOperation(OperationManager.java:212)
at 
org.apache.hive.service.cli.operation.OperationManager.closeOperation(OperationManager.java:240)
at 
org.apache.hive.service.cli.session.HiveSessionImpl.closeOperation(HiveSessionImpl.java:727)
at 
org.apache.hive.service.cli.CLIService.closeOperation(CLIService.java:408)
at 
org.apache.hive.service.cli.thrift.ThriftCLIService.CloseOperation(ThriftCLIService.java:664)
at 
org.apache.hive.service.cli.thrift.TCLIService$Processor$CloseOperation.getResult(TCLIService.java:1513)
at 
org.apache.hive.service.cli.thrift.TCLIService$Processor$CloseOperation.getResult(TCLIService.java:1498)
at org.apache.thrift.ProcessFunction.process(ProcessFunction.java:39)
at org.apache.thrift.TBaseProcessor.process(TBaseProcessor.java:39)
at 
org.apache.hive.service.auth.TSetIpAddressProcessor.process(TSetIpAddressProcessor.java:56)
at 
org.apache.thrift.server.TThreadPoolServer$WorkerProcess.run(TThreadPoolServer.java:285)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
at java.lang.Thread.run(Thread.java:745)
{code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-13045) move guava dependency back to 14 after HIVE-12952

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-13045:
--

 Summary: move guava dependency back to 14 after HIVE-12952
 Key: HIVE-13045
 URL: https://issues.apache.org/jira/browse/HIVE-13045
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


HIVE-12952 removed usage of EvictingQueue, so we don't need to up dependency to 
guava 15 at this point - avoid version related conflicts with clients if we can 
avoid it.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-12966) Change some ZooKeeperHiveLockManager logs to debug

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-12966:
--

 Summary: Change some ZooKeeperHiveLockManager logs to debug
 Key: HIVE-12966
 URL: https://issues.apache.org/jira/browse/HIVE-12966
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


{{ZooKeeperHiveLockManager}} prints a info level log every time it is acquiring 
or releasing a lock. For a table with 10K partitions, that's 20K+ log lines.





--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-12949) Fix description of hive.server2.logging.operation.level

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-12949:
--

 Summary: Fix description of hive.server2.logging.operation.level
 Key: HIVE-12949
 URL: https://issues.apache.org/jira/browse/HIVE-12949
 Project: Hive
  Issue Type: Bug
  Components: Configuration
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


Description of {{hive.server2.logging.operation.level}} states that this 
property is available to set at session level. This is not correct. The 
property is only settable at HS2 level currently.

A log4j appender is created just once currently - based on logging level 
specified by this config, at OperationManager init time in 
initOperationLogCapture(). And there is only one instance of OperationManager.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 42777: HIVE-12550 : Cache and display last N completed queries in HS2 WebUI

[Mohit Sabharwal]

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


Ship it!




Ship It!

- Mohit Sabharwal


On Jan. 27, 2016, 7:38 p.m., Szehon Ho wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42777/
> ---
> 
> (Updated Jan. 27, 2016, 7:38 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-12550
> https://issues.apache.org/jira/browse/HIVE-12550
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Use a Guava 'EvictingQueue' to cache finished SQLOperation info.  Bump up 
> Guava version to get this.  The information is saved when the SQLOperation is 
> closed.
> 
> Also fix a small discrepancy where Elapsed Time (existing field) was 
> comparing against lastAccessTime , which gets updated whenever state changes 
> and some other places.  Chaged to compare against operation's start time.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 97fe7bc 
>   pom.xml 2066518 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 75187cf 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 
> 113eddf 
>   
> service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
> 92135cd 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 
> c8a69b9 
>   
> service/src/java/org/apache/hive/service/cli/operation/SQLOperationInfo.java 
> PRE-CREATION 
>   service/src/resources/hive-webapps/hiveserver2/hiveserver2.jsp a91b008 
> 
> Diff: https://reviews.apache.org/r/42777/diff/
> 
> 
> Testing
> ---
> 
> Manual testing, with screenshot attached.
> 
> 
> Thanks,
> 
> Szehon Ho
> 
>

Re: Review Request 42777: HIVE-12550 : Cache and display last N completed queries in HS2 WebUI

[Mohit Sabharwal]

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



LGTM. Few minor comments.


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 1906)
<https://reviews.apache.org/r/42777/#comment177507>

nit: "max old" -> "maximum number of past operations to show in HiverSever2 
WebUI."



service/src/java/org/apache/hive/service/cli/operation/Operation.java (line 94)
<https://reviews.apache.org/r/42777/#comment177508>

nit: beginTime = lastAccessTime;



service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
(line 71)
<https://reviews.apache.org/r/42777/#comment177510>

extra line



service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
(line 341)
<https://reviews.apache.org/r/42777/#comment177506>

check for getSQLOperationInfo() returning null here.


- Mohit Sabharwal


On Jan. 26, 2016, 10:26 p.m., Szehon Ho wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42777/
> ---
> 
> (Updated Jan. 26, 2016, 10:26 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-12550
> https://issues.apache.org/jira/browse/HIVE-12550
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Use a Guava 'EvictingQueue' to cache finished SQLOperation info.  Bump up 
> Guava version to get this.  The information is saved when the SQLOperation is 
> closed.
> 
> Also fix a small discrepancy where Elapsed Time (existing field) was 
> comparing against lastAccessTime , which gets updated whenever state changes 
> and some other places.  Chaged to compare against operation's start time.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 97fe7bc 
>   pom.xml 2066518 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 75187cf 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 
> 113eddf 
>   
> service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
> 92135cd 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 
> c8a69b9 
>   
> service/src/java/org/apache/hive/service/cli/operation/SQLOperationInfo.java 
> PRE-CREATION 
>   service/src/resources/hive-webapps/hiveserver2/hiveserver2.jsp a91b008 
> 
> Diff: https://reviews.apache.org/r/42777/diff/
> 
> 
> Testing
> ---
> 
> Manual testing, with screenshot attached.
> 
> 
> Thanks,
> 
> Szehon Ho
> 
>

[jira] [Created] (HIVE-12705) Java8 Jenkins jobs failing with "Error executing HIVE-TRUNK-JAVA8-*"

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-12705:
--

 Summary: Java8 Jenkins jobs failing with "Error executing 
HIVE-TRUNK-JAVA8-*"
 Key: HIVE-12705
 URL: https://issues.apache.org/jira/browse/HIVE-12705
 Project: Hive
  Issue Type: Bug
  Components: Hive
Reporter: Mohit Sabharwal


http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/HIVE-TRUNK-JAVA8/

ex: 
http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/HIVE-TRUNK-JAVA8/144/console

{code}
+ java -cp 'target/hive-ptest-1.0-classes.jar:target/lib/*' 
org.apache.hive.ptest.api.client.PTestClient --endpoint 
http://ec2-174-129-184-35.compute-1.amazonaws.com:8181/hive-ptest-1.0/ 
--logsEndpoint http://ec2-174-129-184-35.compute-1.amazonaws.com/logs/ 
--command testStart --profile trunkjava8-mr2 --password c0mmit --outputDir 
target/ --testHandle HIVE-TRUNK-JAVA8-144
2015-12-16 09:56:47,201 ERROR TestExecutor.run:138 Error executing 
HIVE-TRUNK-JAVA8-144 java.lang.NullPointerException: branch
at 
com.google.common.base.Preconditions.checkNotNull(Preconditions.java:209)
at 
org.apache.hive.ptest.execution.conf.TestConfiguration.(TestConfiguration.java:101)
at 
org.apache.hive.ptest.execution.conf.TestConfiguration.fromInputStream(TestConfiguration.java:276)
at 
org.apache.hive.ptest.execution.conf.TestConfiguration.fromFile(TestConfiguration.java:284)
at 
org.apache.hive.ptest.api.server.TestExecutor.run(TestExecutor.java:110)
{code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-12670) Fix tests failing due to invalid ConnectionDriverName

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-12670:
--

 Summary: Fix tests failing due to invalid ConnectionDriverName
 Key: HIVE-12670
 URL: https://issues.apache.org/jira/browse/HIVE-12670
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


Some unit tests fail when run outside the ptest environment (i.e. when run 
individually on the local box like mvn test -Dtest=TestSessionHooks) with the 
following error:

{code}
Caused by: org.datanucleus.exceptions.NucleusException: Attempt to invoke the 
"BONECP" plugin to create a ConnectionPool gave an error : The specified 
datastore driver ("hive-site.xml") was not found in the CLASSPATH. Please check 
your CLASSPATH specification, and the name of the driver.
{code}

This is because to support TestHiveConf, we override 
{{javax.jdo.option.ConnectionDriverName}} in  test hive-site file 
(common/src/test/resources/hive-site.xml). However, this override gets applied 
for all tests. The overriden value is invalid, which causes other tests that 
attempt to initialize CliService to fail.

Instead, we should use a property exclusively used for testing like 
{{hive.test.dummystats.aggregator}} so that overriding it does not affect other 
tests.

Not sure why these tests pass in ptest, presumably because some other test that 
comes before overrides {{javax.jdo.option.ConnectionDriverName}} to a sensible 
value.

Tests failing:

TestSessionHooks
TestPlainSaslHelper 
TestSessionGlobalInitFile



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 41062: HIVE-12485 Secure HS2 web UI with kerberos

[Mohit Sabharwal]

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

Ship it!


LGTM

- Mohit Sabharwal


On Dec. 9, 2015, 4:35 p.m., Jimmy Xiang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41062/
> ---
> 
> (Updated Dec. 9, 2015, 4:35 p.m.)
> 
> 
> Review request for hive, Szehon Ho and Xuefu Zhang.
> 
> 
> Bugs: HIVE-12485
> https://issues.apache.org/jira/browse/HIVE-12485
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Added an AuthenticationFilter to secure the HS2 web ui with kerberos
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d52f994 
>   common/src/java/org/apache/hive/http/HttpServer.java 4b0ed68 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java cad541a 
> 
> Diff: https://reviews.apache.org/r/41062/diff/
> 
> 
> Testing
> ---
> 
> Manually tested it locally.
> 
> 
> Thanks,
> 
> Jimmy Xiang
> 
>

Re: Review Request 41062: HIVE-12485 Secure HS2 web UI with kerberos

[Mohit Sabharwal]

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



common/src/java/org/apache/hive/http/HttpServer.java (line 93)
<https://reviews.apache.org/r/41062/#comment168907>

should this be conditional on the cluster itself being kerberized ?

i.e.
if (UserGroupInformation.isSecurityEnabled()) {
}


- Mohit Sabharwal


On Dec. 8, 2015, 12:05 a.m., Jimmy Xiang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41062/
> ---
> 
> (Updated Dec. 8, 2015, 12:05 a.m.)
> 
> 
> Review request for hive, Szehon Ho and Xuefu Zhang.
> 
> 
> Bugs: HIVE-12485
> https://issues.apache.org/jira/browse/HIVE-12485
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Added an AuthenticationFilter to secure the HS2 web ui with kerberos
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d52f994 
>   common/src/java/org/apache/hive/http/HttpServer.java 4b0ed68 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java cad541a 
> 
> Diff: https://reviews.apache.org/r/41062/diff/
> 
> 
> Testing
> ---
> 
> Manually tested it locally.
> 
> 
> Thanks,
> 
> Jimmy Xiang
> 
>

Re: Review Request 40948: HIVE-12499 : Add HMS metrics for number of tables and partitions

[Mohit Sabharwal]

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


just passing through...


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 705)
<https://reviews.apache.org/r/40948/#comment168426>

Nit: add time unit to descreiption



metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java (line 
419)
<https://reviews.apache.org/r/40948/#comment168431>

Seems like lot of INFO logging since it's every 5 minutes, no ? Make it 
DEBUG ? same for below..


- Mohit Sabharwal


On Dec. 4, 2015, 1:51 a.m., Szehon Ho wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40948/
> ---
> 
> (Updated Dec. 4, 2015, 1:51 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-12499
> https://issues.apache.org/jira/browse/HIVE-12499
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Add separate timer thread that polls for count of database, table, partition 
> entries to publish as metrics, the period is configurable.  Delay in getting 
> exact number should be ok as this is for monitoring.
> 
> Implemented for HBase and DB metastores.
> 
> 
> Diffs
> -
> 
>   
> common/src/java/org/apache/hadoop/hive/common/metrics/common/MetricsConstant.java
>  95e2bcf 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4d881ba 
>   common/src/test/org/apache/hadoop/hive/common/metrics/MetricsTestUtils.java 
> fd420f7 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetaStoreMetrics.java
>  f571c7c 
>   
> itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/hbase/TestHBaseMetastoreMetrics.java
>  PRE-CREATION 
>   metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 
> 00602e1 
>   metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 
> 1c0ab6d 
>   metastore/src/java/org/apache/hadoop/hive/metastore/RawStore.java 5b36b03 
>   
> metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseReadWrite.java 
> 2fb3e8f 
>   metastore/src/java/org/apache/hadoop/hive/metastore/hbase/HBaseStore.java 
> 98e6c75 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  9a1d159 
>   
> metastore/src/test/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  8dde0af 
> 
> Diff: https://reviews.apache.org/r/40948/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests for HBase and Db metastores.
> 
> 
> Thanks,
> 
> Szehon Ho
> 
>

Re: Review Request 40898: HIVE-12431: Support timeout for compile lock

[Mohit Sabharwal]

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

(Updated Dec. 3, 2015, 11:04 p.m.)


Review request for hive.


Changes
---

Incorp fb.


Summary (updated)
-

HIVE-12431: Support timeout for compile lock


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


Repository: hive-git


Description (updated)
---

HIVE-12431: Support timeout for compile lock

When compile lock is configured, a long-compiling request will block 
remaining sessions or concurrent requests for a given session indefinitely. 

This patch allows the user to configure the maximum time a request will wait
to acquire the compile lock.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
7f9607129eb1f5f43e8a728cf7d2a56c1ed5af49 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
62b608cbf53c371d1743df40988daf85f76a0867 
  ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java 
8a47605630066e39272f506c6e309b108b8455dd 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java 
d90002bd16e46b5ce970d4c6c544a9c7605328d1 

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


Testing
---

TestEmbeddedThriftBinaryCLIService#testGlobalCompileLockTimeout


Thanks,

Mohit Sabharwal

Re: Review Request 40898: HIVE-12431: Support timeout for global compile lock

[Mohit Sabharwal]

> On Dec. 3, 2015, 5:40 p.m., Lenni Kuff wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 1849
> > <https://reviews.apache.org/r/40898/diff/2/?file=1152868#file1152868line1849>
> >
> > Curious - does it make sense to only apply this to the "global" compile 
> > lock? Wouldn't this also be applicable for the session-level compile lock?

Yeah, makes sense. Changed it to apply to either case.


> On Dec. 3, 2015, 5:40 p.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 139
> > <https://reviews.apache.org/r/40898/diff/2/?file=1152869#file1152869line139>
> >
> > Is there a better way we can add in these test hook (create a mock 
> > driver or something)?

Mocking the driver to test concurrent requests is a bit tricky. Cleaned this up 
by adding a compile time hook instead as suggesed by Sergey.


> On Dec. 3, 2015, 5:40 p.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 1265
> > <https://reviews.apache.org/r/40898/diff/2/?file=1152869#file1152869line1265>
> >
> > Add a comment to this function to describe what it does and info about 
> > the return value.
> > 
> > Maybe rename to "tryAcquireCompileLock"?

Fixed.


> On Dec. 3, 2015, 5:40 p.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 1277
> > <https://reviews.apache.org/r/40898/diff/2/?file=1152869#file1152869line1277>
> >
> > Should this be INFO level? It might be useful to log the query along 
> > with this message for debug purposes.

Done.


> On Dec. 3, 2015, 5:40 p.m., Lenni Kuff wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/Driver.java, line 1280
> > <https://reviews.apache.org/r/40898/diff/2/?file=1152869#file1152869line1280>
> >
> > Can we include the query text here?

Done.


- Mohit


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


On Dec. 3, 2015, 8:14 a.m., Mohit Sabharwal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40898/
> ---
> 
> (Updated Dec. 3, 2015, 8:14 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-12431
> https://issues.apache.org/jira/browse/HIVE-12431
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-12431: Support timeout for global compile lock
> 
> When global (HS2-wide) compile lock is configured, a long-compiling request 
> will block remaining sessions indefinitely. 
> 
> This patch allows the user to configure the maximum time a request will wait
> to acquire the compile lock. Note that this configuration does not apply when
> session scoped compile locking is configured.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 7f9607129eb1f5f43e8a728cf7d2a56c1ed5af49 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
> 62b608cbf53c371d1743df40988daf85f76a0867 
>   ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java 
> 8a47605630066e39272f506c6e309b108b8455dd 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java 
> d90002bd16e46b5ce970d4c6c544a9c7605328d1 
> 
> Diff: https://reviews.apache.org/r/40898/diff/
> 
> 
> Testing
> ---
> 
> TestEmbeddedThriftBinaryCLIService#testGlobalCompileLockTimeout
> 
> 
> Thanks,
> 
> Mohit Sabharwal
> 
>

Re: Review Request 40898: HIVE-12431: Support timeout for global compile lock

[Mohit Sabharwal]

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

(Updated Dec. 3, 2015, 8:14 a.m.)


Review request for hive.


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


Repository: hive-git


Description
---

HIVE-12431: Support timeout for global compile lock

When global (HS2-wide) compile lock is configured, a long-compiling request 
will block remaining sessions indefinitely. 

This patch allows the user to configure the maximum time a request will wait
to acquire the compile lock. Note that this configuration does not apply when
session scoped compile locking is configured.


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
7f9607129eb1f5f43e8a728cf7d2a56c1ed5af49 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
62b608cbf53c371d1743df40988daf85f76a0867 
  ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java 
8a47605630066e39272f506c6e309b108b8455dd 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java 
d90002bd16e46b5ce970d4c6c544a9c7605328d1 

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


Testing
---

TestEmbeddedThriftBinaryCLIService#testGlobalCompileLockTimeout


Thanks,

Mohit Sabharwal

Review Request 40898: HIVE-12431: Support timeout for global compile lock

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

HIVE-12431: Support timeout for global compile lock

When global (HS2-wide) compile lock is configured, a long-compiling request 
will block remaining sessions indefinitely. 

This patch allows the user to configure the maximum time a request will wait
to acquire the compile lock. Note that this configuration does not apply when
session scoped compile locking is configured.


Diffs
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
7f9607129eb1f5f43e8a728cf7d2a56c1ed5af49 
  ql/src/java/org/apache/hadoop/hive/ql/Driver.java 
62b608cbf53c371d1743df40988daf85f76a0867 
  ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java 
8a47605630066e39272f506c6e309b108b8455dd 
  service/src/test/org/apache/hive/service/cli/CLIServiceTest.java 
d90002bd16e46b5ce970d4c6c544a9c7605328d1 

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


Testing
---

TestEmbeddedThriftBinaryCLIService#testGlobalCompileLockTimeout


Thanks,

Mohit Sabharwal

Re: Review Request 40833: HIVE-12471 Secure HS2 web UI with SSL

[Mohit Sabharwal]

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

Ship it!


The conditional is reverse, otherwise lgtm


common/src/java/org/apache/hive/http/HttpServer.java (line 274)
<https://reviews.apache.org/r/40833/#comment168177>

(!b.useSSL) ?


- Mohit Sabharwal


On Dec. 2, 2015, 8:09 p.m., Jimmy Xiang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40833/
> ---
> 
> (Updated Dec. 2, 2015, 8:09 p.m.)
> 
> 
> Review request for hive, Lefty Leverenz, Szehon Ho, and Xuefu Zhang.
> 
> 
> Bugs: HIVE-12471
> https://issues.apache.org/jira/browse/HIVE-12471
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This patch changed the HttpServer construction to use builder which is 
> flexible. It addes 3 configurations for web UI SSL support. By default, it is 
> disabled.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 7f96071 
>   common/src/java/org/apache/hive/http/HttpServer.java 1ff8d7c 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 204eb5a 
> 
> Diff: https://reviews.apache.org/r/40833/diff/
> 
> 
> Testing
> ---
> 
> Tested web UI with and without SSL, both work as expected.
> 
> 
> Thanks,
> 
> Jimmy Xiang
> 
>

Re: Review Request 40833: HIVE-12471 Secure HS2 web UI with SSL

[Mohit Sabharwal]

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


LGTM. Thanks for adding the Builder! Couple of nits.


common/src/java/org/apache/hive/http/HttpServer.java (line 268)
<https://reviews.apache.org/r/40833/#comment168072>

Nit: b.keyStorePath to determine ssl is not enabled looks bit unclean. 
Maybe add a useSsl boolean to the builder ?



common/src/java/org/apache/hive/http/HttpServer.java (line 290)
<https://reviews.apache.org/r/40833/#comment168073>

Add back the comment 

/* Set servlet context attribute that can be used in jsp */

It wasn't clear to me this is related to jsp



common/src/java/org/apache/hive/http/HttpServer.java (line 299)
<https://reviews.apache.org/r/40833/#comment168071>

Any reason why it shouldn't just default to what's specified in 
HiveConf.HIVE_SERVER2_WEBUI_MAX_THREADS (which is 50) ?


- Mohit Sabharwal


On Dec. 1, 2015, 6:50 p.m., Jimmy Xiang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40833/
> ---
> 
> (Updated Dec. 1, 2015, 6:50 p.m.)
> 
> 
> Review request for hive, Lefty Leverenz, Szehon Ho, and Xuefu Zhang.
> 
> 
> Bugs: HIVE-12471
> https://issues.apache.org/jira/browse/HIVE-12471
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> This patch changed the HttpServer construction to use builder which is 
> flexible. It addes 3 configurations for web UI SSL support. By default, it is 
> disabled.
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 9e805bd 
>   common/src/java/org/apache/hive/http/HttpServer.java 1ff8d7c 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 204eb5a 
> 
> Diff: https://reviews.apache.org/r/40833/diff/
> 
> 
> Testing
> ---
> 
> Tested web UI with and without SSL, both work as expected.
> 
> 
> Thanks,
> 
> Jimmy Xiang
> 
>

[jira] [Created] (HIVE-12561) Add sentry jars to HS2 & HMS classpath

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-12561:
--

 Summary: Add sentry jars to HS2 & HMS classpath
 Key: HIVE-12561
 URL: https://issues.apache.org/jira/browse/HIVE-12561
 Project: Hive
  Issue Type: Improvement
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal
Priority: Minor


If SENTRY_HOME is either set or can be detected at conventional location, add 
relevant jars to HS2 and HMS classpath.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-12512) Include driver logs in execution-level Operation logs

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-12512:
--

 Summary: Include driver logs in execution-level Operation logs
 Key: HIVE-12512
 URL: https://issues.apache.org/jira/browse/HIVE-12512
 Project: Hive
  Issue Type: Bug
  Components: Logging
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal
Priority: Minor


When {{hive.server2.logging.operation.level}} is set to {{EXECUTION}} 
(default),  operation logs do not include Driver logs, which contain useful 
info like total number of jobs launched, stage getting executed, etc. that help 
track high-level progress. It only adds a few more lines to the output.

{code}
15/11/24 14:09:12 INFO ql.Driver: Semantic Analysis Completed
15/11/24 14:09:12 INFO ql.Driver: Starting 
command(queryId=hive_20151124140909_e8cbb9bd-bac0-40b2-83d0-382de25b80d1): 
select count(*) from sample_08
15/11/24 14:09:12 INFO ql.Driver: Query ID = 
hive_20151124140909_e8cbb9bd-bac0-40b2-83d0-382de25b80d1
15/11/24 14:09:12 INFO ql.Driver: Total jobs = 1
...
15/11/24 14:09:40 INFO ql.Driver: MapReduce Jobs Launched:
15/11/24 14:09:40 INFO ql.Driver: Stage-Stage-1: Map: 1  Reduce: 1   Cumulative 
CPU: 3.58 sec   HDFS Read: 52956 HDFS Write: 4 SUCCESS
15/11/24 14:09:40 INFO ql.Driver: Total MapReduce CPU Time Spent: 3 seconds 580 
msec
15/11/24 14:09:40 INFO ql.Driver: OK
{code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session

[Mohit Sabharwal]

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

Ship it!


Ship It!

- Mohit Sabharwal


On Nov. 20, 2015, 8:39 p.m., Aihua Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40549/
> ---
> 
> (Updated Nov. 20, 2015, 8:39 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-12456: QueryId can't be stored in the configuration of the SessionState 
> since multiple queries can run in a single session
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4f8209a 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  3f2de10 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 
> d13415e 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 
> 8b42265 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 2d784f0 
> 
> Diff: https://reviews.apache.org/r/40549/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aihua Xu
> 
>

Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session

[Mohit Sabharwal]

> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 
> > 56
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line56>
> >
> > what does MDC stand for ?
> 
> Aihua Xu wrote:
> MDC is log4j term. Stands for Mapped diagnostics context and that is how 
> we used for sessionId and queryId.

I'd vote to call it QUERYID_LOG_KEY or something to make it more readable.


> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 
> > 87
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line87>
> >
> > is this conditional needed ?
> 
> Aihua Xu wrote:
> Yeah. It's needed. So it means we only overwrite when the passed in 
> confOverlay is not null. Originally I was also confused and thought it's the 
> class memember but actually it's the passed in parameter.

Ok. 

In the current code: 
  If param confOverlay == null, this.confOverlay is null (default)
  if param confOverlay != null, this.confOverlay = confOverlay 
So, the conditional is redundant, right ?


- Mohit


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


On Nov. 20, 2015, 4:56 p.m., Aihua Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40549/
> ---
> 
> (Updated Nov. 20, 2015, 4:56 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-12456: QueryId can't be stored in the configuration of the SessionState 
> since multiple queries can run in a single session
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 4f8209ad90841740c447534f7902d49c8a8d2038 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  3f2de108f069a815a46d49498238bb8197263d93 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 
> d13415e820e67a027d63ba994c05db18b613afc1 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 
> 8b4226522f1ff6c5c777fe372490f0fd157a3b2b 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 2d784f0d978c04ce674dc06b1adc0cf3d8d5f984 
> 
> Diff: https://reviews.apache.org/r/40549/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aihua Xu
> 
>

Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session

[Mohit Sabharwal]

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


LGTM. Couple of questions.


service/src/java/org/apache/hive/service/cli/operation/Operation.java (line 56)
<https://reviews.apache.org/r/40549/#comment166493>

what does MDC stand for ?



service/src/java/org/apache/hive/service/cli/operation/Operation.java (lines 56 
- 57)
<https://reviews.apache.org/r/40549/#comment166496>

why make these public ... only accessed in this class.



service/src/java/org/apache/hive/service/cli/operation/Operation.java (line 87)
<https://reviews.apache.org/r/40549/#comment166497>

is this conditional needed ?


- Mohit Sabharwal


On Nov. 20, 2015, 4:56 p.m., Aihua Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40549/
> ---
> 
> (Updated Nov. 20, 2015, 4:56 p.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-12456: QueryId can't be stored in the configuration of the SessionState 
> since multiple queries can run in a single session
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 
> 4f8209ad90841740c447534f7902d49c8a8d2038 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  3f2de108f069a815a46d49498238bb8197263d93 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 
> d13415e820e67a027d63ba994c05db18b613afc1 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 
> 8b4226522f1ff6c5c777fe372490f0fd157a3b2b 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 2d784f0d978c04ce674dc06b1adc0cf3d8d5f984 
> 
> Diff: https://reviews.apache.org/r/40549/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aihua Xu
> 
>

Re: Review Request 40500: HIVE-12338 Add webui to HiveServer2

[Mohit Sabharwal]

> On Nov. 20, 2015, 4:53 a.m., Mohit Sabharwal wrote:
> > LGTM. This patch only includes SQLOperations. Are we planning to add 
> > metadata operations as well ? (so we can capture jdbc clients and Hue usage 
> > as well...)
> 
> Jimmy Xiang wrote:
> Thanks a lot for the review. I was thinking about showing meta operations 
> in HMS web UI. Should we also show them here?

I guess at some point it'd be good to have it in HS2 UI as well so we can get 
the end-to-end picture (That way we can show all running operations for a given 
session, not just the queries). We could do this as an enhancement at a later 
point.


- Mohit


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


On Nov. 19, 2015, 8:53 p.m., Jimmy Xiang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40500/
> ---
> 
> (Updated Nov. 19, 2015, 8:53 p.m.)
> 
> 
> Review request for hive, Szehon Ho and Xuefu Zhang.
> 
> 
> Bugs: HIVE-12338
> https://issues.apache.org/jira/browse/HIVE-12338
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Added web UI to HS2. The UI is similar to those for other Hadoop components.
> The default web UI port is set to 10002, which is configurable. It can be 
> disabled. Currently it shows active sessions and queries. It can also access 
> locals, metrics, and configuration.
> 
> 
> Diffs
> -
> 
>   common/pom.xml cd14581 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 2185f85 
>   common/src/java/org/apache/hive/http/AdminAuthorizedServlet.java 
> PRE-CREATION 
>   common/src/java/org/apache/hive/http/ConfServlet.java PRE-CREATION 
>   common/src/java/org/apache/hive/http/HttpServer.java PRE-CREATION 
>   common/src/java/org/apache/hive/http/JMXJsonServlet.java PRE-CREATION 
>   pom.xml c6df4a5 
>   service/pom.xml afa52cf 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 
> d13415e 
>   
> service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
> b0bd351 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 
> 8b42265 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionBase.java 
> 1ab5652 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 2d784f0 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
> d11cf3d 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java b30b6a2 
>   service/src/resources/hive-webapps/hiveserver2/hiveserver2.jsp PRE-CREATION 
>   service/src/resources/hive-webapps/hiveserver2/index.html PRE-CREATION 
>   service/src/resources/hive-webapps/static/css/bootstrap-theme.min.css 
> PRE-CREATION 
>   service/src/resources/hive-webapps/static/css/bootstrap.min.css 
> PRE-CREATION 
>   service/src/resources/hive-webapps/static/css/hive.css PRE-CREATION 
>   
> service/src/resources/hive-webapps/static/fonts/glyphicons-halflings-regular.eot
>  PRE-CREATION 
>   
> service/src/resources/hive-webapps/static/fonts/glyphicons-halflings-regular.svg
>  PRE-CREATION 
>   
> service/src/resources/hive-webapps/static/fonts/glyphicons-halflings-regular.ttf
>  PRE-CREATION 
>   
> service/src/resources/hive-webapps/static/fonts/glyphicons-halflings-regular.woff
>  PRE-CREATION 
>   service/src/resources/hive-webapps/static/hive_logo.jpeg PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40500/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jimmy Xiang
> 
>

Re: Review Request 40500: HIVE-12338 Add webui to HiveServer2

[Mohit Sabharwal]

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


LGTM. This patch only includes SQLOperations. Are we planning to add metadata 
operations as well ? (so we can capture jdbc clients and Hue usage as well...)


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 1856)
<https://reviews.apache.org/r/40500/#comment166319>

Th -> The



service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
(line 65)
<https://reviews.apache.org/r/40500/#comment166342>

any reason we only want sqloperations ?


- Mohit Sabharwal


On Nov. 19, 2015, 8:53 p.m., Jimmy Xiang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40500/
> ---
> 
> (Updated Nov. 19, 2015, 8:53 p.m.)
> 
> 
> Review request for hive, Szehon Ho and Xuefu Zhang.
> 
> 
> Bugs: HIVE-12338
> https://issues.apache.org/jira/browse/HIVE-12338
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> Added web UI to HS2. The UI is similar to those for other Hadoop components.
> The default web UI port is set to 10002, which is configurable. It can be 
> disabled. Currently it shows active sessions and queries. It can also access 
> locals, metrics, and configuration.
> 
> 
> Diffs
> -
> 
>   common/pom.xml cd14581 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 2185f85 
>   common/src/java/org/apache/hive/http/AdminAuthorizedServlet.java 
> PRE-CREATION 
>   common/src/java/org/apache/hive/http/ConfServlet.java PRE-CREATION 
>   common/src/java/org/apache/hive/http/HttpServer.java PRE-CREATION 
>   common/src/java/org/apache/hive/http/JMXJsonServlet.java PRE-CREATION 
>   pom.xml c6df4a5 
>   service/pom.xml afa52cf 
>   service/src/java/org/apache/hive/service/cli/operation/Operation.java 
> d13415e 
>   
> service/src/java/org/apache/hive/service/cli/operation/OperationManager.java 
> b0bd351 
>   service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 
> 8b42265 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionBase.java 
> 1ab5652 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 
> 2d784f0 
>   service/src/java/org/apache/hive/service/cli/session/SessionManager.java 
> d11cf3d 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java b30b6a2 
>   service/src/resources/hive-webapps/hiveserver2/hiveserver2.jsp PRE-CREATION 
>   service/src/resources/hive-webapps/hiveserver2/index.html PRE-CREATION 
>   service/src/resources/hive-webapps/static/css/bootstrap-theme.min.css 
> PRE-CREATION 
>   service/src/resources/hive-webapps/static/css/bootstrap.min.css 
> PRE-CREATION 
>   service/src/resources/hive-webapps/static/css/hive.css PRE-CREATION 
>   
> service/src/resources/hive-webapps/static/fonts/glyphicons-halflings-regular.eot
>  PRE-CREATION 
>   
> service/src/resources/hive-webapps/static/fonts/glyphicons-halflings-regular.svg
>  PRE-CREATION 
>   
> service/src/resources/hive-webapps/static/fonts/glyphicons-halflings-regular.ttf
>  PRE-CREATION 
>   
> service/src/resources/hive-webapps/static/fonts/glyphicons-halflings-regular.woff
>  PRE-CREATION 
>   service/src/resources/hive-webapps/static/hive_logo.jpeg PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40500/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jimmy Xiang
> 
>

[jira] [Created] (HIVE-11677) Access to opHandleSet in HiveSession should be synchronized

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-11677:
--

 Summary: Access to opHandleSet in HiveSession should be 
synchronized
 Key: HIVE-11677
 URL: https://issues.apache.org/jira/browse/HIVE-11677
 Project: Hive
  Issue Type: Bug
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


In the scenario where multiple threads share the same session, reading/writing 
to HiveSessionImpl.opHandleSet can lead to a race condition. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-11534) Improve validateTableCols error message

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-11534:
--

 Summary: Improve validateTableCols error message
 Key: HIVE-11534
 URL: https://issues.apache.org/jira/browse/HIVE-11534
 Project: Hive
  Issue Type: Improvement
  Components: Hive
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal
Priority: Minor


For tables created without column definition in the DDL (but referencing the 
schema in the underlying file format like Avro), ObjectStore.validateTableCols 
throws an exception that doesn't include the table and db name.  This makes it 
tedious to lookup table name in schema files.

Example:
{code}
ERROR org.apache.hadoop.hive.metastore.ObjectStore: Error retrieving statistics 
via jdo
MetaException(message:Column wpp_mbrshp_hix_ik doesn't exist.)
at 
org.apache.hadoop.hive.metastore.ObjectStore.validateTableCols(ObjectStore.java:6061)
at 
org.apache.hadoop.hive.metastore.ObjectStore.getMTableColumnStatistics(ObjectStore.java:6012)
at 
org.apache.hadoop.hive.metastore.ObjectStore.access$1000(ObjectStore.java:160)
at 
org.apache.hadoop.hive.metastore.ObjectStore$6.getJdoResult(ObjectStore.java:6084)
at 
org.apache.hadoop.hive.metastore.ObjectStore$6.getJdoResult(ObjectStore.java:6076)
{code}

We should add database and the table name to the error message.




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (HIVE-11133) Support hive.explain.user for Spark

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-11133:
--

 Summary: Support hive.explain.user for Spark
 Key: HIVE-11133
 URL: https://issues.apache.org/jira/browse/HIVE-11133
 Project: Hive
  Issue Type: Sub-task
  Components: Spark
Reporter: Mohit Sabharwal


User friendly explain output ({{set hive.explain.user=true}}) should support 
Spark as well. 

Once supported, we should also enable related q-tests like {{explainuser_1.q}}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Review Request 35897: HIVE-11032: Enable more tests for grouping by skewed data [Spark Branch]

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

HIVE-11032: Enable more tests for grouping by skewed data [Spark Branch]

All tests with hive.groupby.skewindata set pass with Spark except 
explainuser_1.q, 
which has set hive.explain.user=true; set and this feature (user friendly 
explain output) 
is not supported for Spark yet.

git grep "hive.groupby.skewindata" | grep ".q:" | perl -pe 's@:set 
hive.groupby.skewindata.*@@g' | sort


Diffs
-

  itests/src/test/resources/testconfiguration.properties 
7b7559a9590803f2528a24180a962013881cba1b 
  ql/src/test/results/clientnegative/spark/groupby2_multi_distinct.q.out 
PRE-CREATION 
  
ql/src/test/results/clientnegative/spark/groupby3_map_skew_multi_distinct.q.out 
PRE-CREATION 
  ql/src/test/results/clientnegative/spark/groupby3_multi_distinct.q.out 
PRE-CREATION 
  ql/src/test/results/clientnegative/spark/groupby_grouping_sets7.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby1_map.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby1_map_nomap.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby1_map_skew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby1_noskew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby2_map.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby2_map_multi_distinct.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby2_map_skew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby2_noskew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby2_noskew_multi_distinct.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby4_map.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby4_map_skew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby4_noskew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby5.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby5_map.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby5_map_skew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby5_noskew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby6.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby6_map.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby6_map_skew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby6_noskew.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby_grouping_id2.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby_ppr_multi_distinct.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/spark/groupby_resolution.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/spark/nullgroup.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/nullgroup2.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/nullgroup4.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/nullgroup4_multi_distinct.q.out 
PRE-CREATION 
  ql/src/test/results/clientpositive/spark/temp_table_gb1.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/udaf_collect_set.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/udf_max.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/udf_min.q.out PRE-CREATION 
  ql/src/test/results/clientpositive/spark/udf_percentile.q.out PRE-CREATION 

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


Testing
---

cat postiveqtests.txt | xargs -n 20 | perl -pe 's@ @,@g' | while read -r line; 
do command mvn test -Dtest=TestSparkCliDriver -Phadoop-2 -Dqfile=$line 
-Dtest.output.overwrite=true; done

cat negativeqtests.txt | xargs -n 20 | perl -pe 's@ @,@g' | while read -r line; 
do command mvn test -Dtest=TestNegativeSparkCliDriver -Phadoop-2 -Dqfile=$line 
-Dtest.output.overwrite=true; done


Thanks,

Mohit Sabharwal

Review Request 35846: HIVE-11099: Add support for running negative q-tests [Spark Branch]

[Mohit Sabharwal]

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

Review request for hive.


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


Repository: hive-git


Description
---

HIVE-11099: Add support for running negative q-tests [Spark Branch]

Add support for TestSparkNegativeCliDriver and 
TestMiniSparkOnYarnNegativeCliDriver for negative q-tests

Adding empty spark.query.negative.files and 
miniSparkOnYarn.query.negative.files lists in 
testconfiguration.properties. These lists can be populated as 
we enable negative q-tests. HIVE-11032 is a follow-up
patch which will enable some of these tests


Diffs
-

  itests/qtest-spark/pom.xml dcb76230584857294e0d7346da8e7353084ea3ae 
  itests/src/test/resources/testconfiguration.properties 
7b7559a9590803f2528a24180a962013881cba1b 

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


Testing
---

Tested with a negative q-test:
mvn test -Dmodule=ql -Dtest=TestSparkNegativeCliDriver 
-Dqfile=groupby2_map_skew_multi_distinct.q -Phadoop-2


Thanks,

Mohit Sabharwal

[jira] [Created] (HIVE-11099) Add support for running negative q-tests [Spark Branch]

[Mohit Sabharwal (JIRA)]

Mohit Sabharwal created HIVE-11099:
--

 Summary: Add support for running negative q-tests [Spark Branch]
 Key: HIVE-11099
 URL: https://issues.apache.org/jira/browse/HIVE-11099
 Project: Hive
  Issue Type: Sub-task
  Components: Spark
Reporter: Mohit Sabharwal
Assignee: Mohit Sabharwal


Add support for TestSparkNegativeCliDriver TestMiniSparkOnYarnNegativeCliDriver 
to negative q-tests



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Re: Review Request 32489: HIVE-9518 Implement MONTHS_BETWEEN aligned with Oracle one

[Mohit Sabharwal]

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



ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFMonthsBetween.java
<https://reviews.apache.org/r/32489/#comment127019>

nit: cleaner to just say (31 * 24 * 60 * 60)


- Mohit Sabharwal


On March 31, 2015, 8:08 p.m., Alexander Pivovarov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32489/
> ---
> 
> (Updated March 31, 2015, 8:08 p.m.)
> 
> 
> Review request for hive and Jason Dere.
> 
> 
> Bugs: HIVE-9518
> https://issues.apache.org/jira/browse/HIVE-9518
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-9518 Implement MONTHS_BETWEEN aligned with Oracle one
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 
> 2476e832b8b7101971ea2226368aa82633b7e7d1 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 
> ce981232382e993c7c9d640efe9b2d21f70a0ed4 
>   
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFMonthsBetween.java
>  PRE-CREATION 
>   
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFMonthsBetween.java
>  PRE-CREATION 
>   ql/src/test/queries/clientpositive/udf_months_between.q PRE-CREATION 
>   ql/src/test/results/clientpositive/show_functions.q.out 
> 22091d06241218a5c0ee21d6ee6be00a71706971 
>   ql/src/test/results/clientpositive/udf_months_between.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32489/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Pivovarov
> 
>

Re: Review Request 32489: HIVE-9518 Implement MONTHS_BETWEEN aligned with Oracle one

[Mohit Sabharwal]

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



ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java
<https://reviews.apache.org/r/32489/#comment126766>

based on your comment, don't you want to return null for dataStr.length() > 
10 ?

As it stands now, long date+time string will get converted using: 

   Object writableValue = converters[i].convert(obj);
Timestamp ts = ((TimestampWritable) writableValue).getTimestamp();
return ts;

Is that your intention?



ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java
<https://reviews.apache.org/r/32489/#comment126767>

based on your comment, don't you want to return null for dataStr.length() > 
10 ?

As it stands now, long date+time string will get converted using: 

   Object writableValue = converters[i].convert(obj);
Timestamp ts = ((TimestampWritable) writableValue).getTimestamp();
return ts;

Is that your intention?


- Mohit Sabharwal


On March 27, 2015, 6:35 p.m., Alexander Pivovarov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32489/
> ---
> 
> (Updated March 27, 2015, 6:35 p.m.)
> 
> 
> Review request for hive and Jason Dere.
> 
> 
> Bugs: HIVE-9518
> https://issues.apache.org/jira/browse/HIVE-9518
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-9518 Implement MONTHS_BETWEEN aligned with Oracle one
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 
> 2476e832b8b7101971ea2226368aa82633b7e7d1 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 
> ce981232382e993c7c9d640efe9b2d21f70a0ed4 
>   
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFMonthsBetween.java
>  PRE-CREATION 
>   
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFMonthsBetween.java
>  PRE-CREATION 
>   ql/src/test/queries/clientpositive/udf_months_between.q PRE-CREATION 
>   ql/src/test/results/clientpositive/show_functions.q.out 
> 22091d06241218a5c0ee21d6ee6be00a71706971 
>   ql/src/test/results/clientpositive/udf_months_between.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32489/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Pivovarov
> 
>

Re: Review Request 32489: HIVE-9518 Implement MONTHS_BETWEEN aligned with Oracle one

[Mohit Sabharwal]

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


LGTM. Some nits.


ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFMonthsBetween.java
<https://reviews.apache.org/r/32489/#comment126512>

static?



ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java
<https://reviews.apache.org/r/32489/#comment126504>

same as PrimitiveGrouping.getPrimitiveGroup(inputTypes[i]) == STRING_GROUP ?



ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java
<https://reviews.apache.org/r/32489/#comment126505>

19...a comment on where this comes from would be great


- Mohit Sabharwal


On March 27, 2015, 12:58 a.m., Alexander Pivovarov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32489/
> ---
> 
> (Updated March 27, 2015, 12:58 a.m.)
> 
> 
> Review request for hive and Jason Dere.
> 
> 
> Bugs: HIVE-9518
> https://issues.apache.org/jira/browse/HIVE-9518
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-9518 Implement MONTHS_BETWEEN aligned with Oracle one
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java 
> 2476e832b8b7101971ea2226368aa82633b7e7d1 
>   ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDF.java 
> ce981232382e993c7c9d640efe9b2d21f70a0ed4 
>   
> ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFMonthsBetween.java
>  PRE-CREATION 
>   
> ql/src/test/org/apache/hadoop/hive/ql/udf/generic/TestGenericUDFMonthsBetween.java
>  PRE-CREATION 
>   ql/src/test/queries/clientpositive/udf_months_between.q PRE-CREATION 
>   ql/src/test/results/clientpositive/show_functions.q.out 
> 22091d06241218a5c0ee21d6ee6be00a71706971 
>   ql/src/test/results/clientpositive/udf_months_between.q.out PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32489/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Pivovarov
> 
>