Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-03-19 Thread Daniel Dai 

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


Ship it!




+1, let's get this in. We can open new tickets for any remaining review 
Alexander has.

- Daniel Dai


On March 9, 2018, 9 p.m., Vaibhav Gumashta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65634/
> ---
> 
> (Updated March 9, 2018, 9 p.m.)
> 
> 
> Review request for hive, Alexander Kolbasov, Daniel Dai, and Thejas Nair.
> 
> 
> Bugs: HIVE-18264
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  a3725c5395 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 86c9c2b33c 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  ac71d0882f 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  7b44df4128 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  f500d63725 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
>  f0f650ddcf 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  0d132f2074 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
>  32ea17495f 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  50f873a013 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  75ea8c4a77 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  207d842f94 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
>  ab6feb6f0b 
>   standalone-metastore/src/test/resources/log4j2.properties 365687e1c9 
> 
> 
> Diff: https://reviews.apache.org/r/65634/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-03-09 Thread Vaibhav Gumashta 

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

(Updated March 9, 2018, 9 p.m.)


Review request for hive, Daniel Dai and Thejas Nair.


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


Repository: hive-git


Description
---

https://issues.apache.org/jira/browse/HIVE-18264


Diffs (updated)
-

  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 a3725c5395 
  service/src/java/org/apache/hive/service/server/HiveServer2.java 86c9c2b33c 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 ac71d0882f 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 7b44df4128 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
 f500d63725 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
 f0f650ddcf 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
 0d132f2074 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
 32ea17495f 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
 50f873a013 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 75ea8c4a77 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 207d842f94 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
 ab6feb6f0b 
  standalone-metastore/src/test/resources/log4j2.properties 365687e1c9 


Diff: https://reviews.apache.org/r/65634/diff/5/

Changes: https://reviews.apache.org/r/65634/diff/4-5/


Testing
---


Thanks,

Vaibhav Gumashta

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-03-09 Thread Vaibhav Gumashta 


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
> > Lines 63 (patched)
> > 
> >
> > It would be cleaner and easier to read to rewrite this as
> > 
> > ```
> >   public static String buildKey(List partVals) {
> > if (partVals == null || partVals.isEmpty()) {
> >   return "";
> > }
> > return String.join(delimit, partVals);
> >   }
> > ```

I have made the change based on your suggestion, but I think it's just a 
preference of style.


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
> > Lines 70 (patched)
> > 
> >
> > 1) Please add Javadoc comment, explaining what this function does.
> > 2) Is overloading really useful here?

I agree, makes sense to have a new method than overload since the params don't 
really convey the intent if you overlook the param name. I'll make changes to 
other methods as well.


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
> > Lines 71 (patched)
> > 
> >
> > why not just 
> > 
> > `return buildKey(partVals) + delimit + colName`
> > 
> > can colName be empty here or not?

colName won't be empty here.


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
> > Lines 128 (patched)
> > 
> >
> > 1) Please add units in the name and use constant for the default value.
> > 2) Please document what is `cacheRefreshPeriod`.

It is already documented as part of the Conf class. Adding the same note here.


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
> > Line 226 (original), 140 (patched)
> > 
> >
> > Please document this method - in particular how does it gets cache 
> > implementaiton from config.

Actually I don't think this method is needed now (have removed it). This was 
introduced in HIVE-17629, when prewarm was a blocking call. In this patch, I 
have made it non-blocking and it runs in a background thread so that metastore 
can remain usable during cache prewarm.


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
> > Line 262 (original), 175 (patched)
> > 
> >
> > This doesn't look correct:
> > 
> > 1) initBlackListWhiteList() will not update any existing whitelist or 
> > blacklist, only add one if it wasn't there.
> > 2) initBlackListWhiteList() is calling 
> > `Collections.reverse(blacklistPatterns)` which doesn't make sense when 
> > configuration is set to a new value.

This is picked once only when the metastore server is started and then used 
thereafter. Removed Collections.reverse.


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
> > Lines 176 (patched)
> > 
> >
> > Looks like every time someone calls setConf() a new thread is started - 
> > isn't it a threda leak?
> > In general it isn't a good practice to add such side-effects for config 
> > changes like setConf - it is better to explicitly call a method which will 
> > do whatever is needed after conf update.

No, a new thread is started only when cacheUpdateMaster is null or from the 
Unit tests (where we explicitly try to control start and stop, and where the 
thread dies after one run).


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
> > Line 266 (original), 202 (patched)
> > 
> >
> > Please document this method. Among other things - can prewarm() be 
> > called multiple times? If not, should it be somehow enforced?

Have enforced that. Currently it was being called just once from the background 
thread.


> On March 2, 2018, 7:54 a.m., Alexander Kolbasov wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
> > Line 271 (original), 207

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-03-01 Thread Alexander Kolbasov 

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



Changes are pretty big, I didn't go through all of them 0 some comments below.


standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
Lines 62 (patched)


Please add Javadoc comment, explaining what this function does.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
Lines 63 (patched)


It would be cleaner and easier to read to rewrite this as

```
  public static String buildKey(List partVals) {
if (partVals == null || partVals.isEmpty()) {
  return "";
}
return String.join(delimit, partVals);
  }
```



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
Lines 70 (patched)


1) Please add Javadoc comment, explaining what this function does.
2) Is overloading really useful here?



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
Lines 71 (patched)


why not just 

`return buildKey(partVals) + delimit + colName`

can colName be empty here or not?



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
Line 62 (original), 75 (patched)


This method is never used



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Lines 128 (patched)


1) Please add units in the name and use constant for the default value.
2) Please document what is `cacheRefreshPeriod`.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 223 (original), 137 (patched)


Why do you need an empty public constructor?



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 226 (original), 140 (patched)


Please document this method - in particular how does it gets cache 
implementaiton from config.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 236 (original), 150 (patched)


Please used internal formatting for LOG:

LOG.debug("CachedStore is not enabled; using {}", clazzName)



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 237 (original), 151 (patched)


This return is not needed.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 259 (original), 172 (patched)


Can this be an else part of the prior if?



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 262 (original), 175 (patched)


This doesn't look correct:

1) initBlackListWhiteList() will not update any existing whitelist or 
blacklist, only add one if it wasn't there.
2) initBlackListWhiteList() is calling 
`Collections.reverse(blacklistPatterns)` which doesn't make sense when 
configuration is set to a new value.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Lines 176 (patched)


Looks like every time someone calls setConf() a new thread is started - 
isn't it a threda leak?
In general it isn't a good practice to add such side-effects for config 
changes like setConf - it is better to explicitly call a method which will do 
whatever is needed after conf update.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Lines 179 (patched)


Please document this method.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Lines 180 (patched)


This seem to repeat the code from setConf. is there any way to avoid code 
copy?

The only difference seems to be the call to startCacheUpdateService() which 
shows that it isn't a good idea to have it there.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 266 (original), 202 (patched)

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-03-01 Thread Daniel Dai 

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


Ship it!




Ship It!

- Daniel Dai


On March 1, 2018, 11:09 a.m., Vaibhav Gumashta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65634/
> ---
> 
> (Updated March 1, 2018, 11:09 a.m.)
> 
> 
> Review request for hive, Daniel Dai and Thejas Nair.
> 
> 
> Bugs: HIVE-18264
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  a3725c5395 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 86c9c2b33c 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  ac71d0882f 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  7b44df4128 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  f500d63725 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
>  f0f650ddcf 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  0d132f2074 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
>  32ea17495f 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  50f873a013 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  75ea8c4a77 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  207d842f94 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
>  ab6feb6f0b 
>   standalone-metastore/src/test/resources/log4j2.properties 365687e1c9 
> 
> 
> Diff: https://reviews.apache.org/r/65634/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-03-01 Thread Vaibhav Gumashta 

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

(Updated March 1, 2018, 11:09 a.m.)


Review request for hive, Daniel Dai and Thejas Nair.


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


Repository: hive-git


Description
---

https://issues.apache.org/jira/browse/HIVE-18264


Diffs (updated)
-

  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 a3725c5395 
  service/src/java/org/apache/hive/service/server/HiveServer2.java 86c9c2b33c 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 ac71d0882f 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 7b44df4128 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
 f500d63725 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
 f0f650ddcf 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
 0d132f2074 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
 32ea17495f 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
 50f873a013 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 75ea8c4a77 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 207d842f94 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
 ab6feb6f0b 
  standalone-metastore/src/test/resources/log4j2.properties 365687e1c9 


Diff: https://reviews.apache.org/r/65634/diff/4/

Changes: https://reviews.apache.org/r/65634/diff/3-4/


Testing
---


Thanks,

Vaibhav Gumashta

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-02-27 Thread Daniel Dai 

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


Fix it, then Ship it!




Ship It!


standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 583 (original), 568 (patched)


Blindly update cache may overwrite newer content. But will leave this to 
notification based update.


- Daniel Dai


On Feb. 26, 2018, 9:47 p.m., Vaibhav Gumashta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65634/
> ---
> 
> (Updated Feb. 26, 2018, 9:47 p.m.)
> 
> 
> Review request for hive, Daniel Dai and Thejas Nair.
> 
> 
> Bugs: HIVE-18264
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  a3725c5395 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 6c1a0b98cc 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  c6e34a8a22 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  7b44df4128 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  f500d63725 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
>  f0f650ddcf 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  0d132f2074 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
>  32ea17495f 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  50f873a013 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  75ea8c4a77 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  207d842f94 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
>  ab6feb6f0b 
>   standalone-metastore/src/test/resources/log4j2.properties 365687e1c9 
> 
> 
> Diff: https://reviews.apache.org/r/65634/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-02-27 Thread Daniel Dai 

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


Ship it!




Ship It!

- Daniel Dai


On Feb. 26, 2018, 9:47 p.m., Vaibhav Gumashta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65634/
> ---
> 
> (Updated Feb. 26, 2018, 9:47 p.m.)
> 
> 
> Review request for hive, Daniel Dai and Thejas Nair.
> 
> 
> Bugs: HIVE-18264
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  a3725c5395 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 6c1a0b98cc 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
>  c6e34a8a22 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  7b44df4128 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  f500d63725 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
>  f0f650ddcf 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  0d132f2074 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
>  32ea17495f 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  50f873a013 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  75ea8c4a77 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  207d842f94 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
>  ab6feb6f0b 
>   standalone-metastore/src/test/resources/log4j2.properties 365687e1c9 
> 
> 
> Diff: https://reviews.apache.org/r/65634/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-02-26 Thread Vaibhav Gumashta 

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

(Updated Feb. 26, 2018, 9:47 p.m.)


Review request for hive, Daniel Dai and Thejas Nair.


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


Repository: hive-git


Description
---

https://issues.apache.org/jira/browse/HIVE-18264


Diffs (updated)
-

  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 a3725c5395 
  service/src/java/org/apache/hive/service/server/HiveServer2.java 6c1a0b98cc 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
 c6e34a8a22 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 7b44df4128 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
 f500d63725 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
 f0f650ddcf 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
 0d132f2074 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
 32ea17495f 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
 50f873a013 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 75ea8c4a77 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 207d842f94 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
 ab6feb6f0b 
  standalone-metastore/src/test/resources/log4j2.properties 365687e1c9 


Diff: https://reviews.apache.org/r/65634/diff/3/

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


Testing
---


Thanks,

Vaibhav Gumashta

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-02-23 Thread Daniel Dai 

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


Ship it!




Ship It!

- Daniel Dai


On Feb. 23, 2018, 8:14 p.m., Vaibhav Gumashta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65634/
> ---
> 
> (Updated Feb. 23, 2018, 8:14 p.m.)
> 
> 
> Review request for hive, Daniel Dai and Thejas Nair.
> 
> 
> Bugs: HIVE-18264
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  a3725c5395 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  7b44df4128 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  f500d63725 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
>  f0f650ddcf 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  0d132f2074 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
>  32ea17495f 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
>  50f873a013 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  75ea8c4a77 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  207d842f94 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
>  ab6feb6f0b 
> 
> 
> Diff: https://reviews.apache.org/r/65634/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-02-23 Thread Vaibhav Gumashta 

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

(Updated Feb. 23, 2018, 8:14 p.m.)


Review request for hive, Daniel Dai and Thejas Nair.


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


Repository: hive-git


Description
---

https://issues.apache.org/jira/browse/HIVE-18264


Diffs (updated)
-

  
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
 a3725c5395 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 7b44df4128 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
 f500d63725 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
 f0f650ddcf 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
 0d132f2074 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
 32ea17495f 
  
standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java
 50f873a013 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
 75ea8c4a77 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
 207d842f94 
  
standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
 ab6feb6f0b 


Diff: https://reviews.apache.org/r/65634/diff/2/

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


Testing
---


Thanks,

Vaibhav Gumashta

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-02-23 Thread Vaibhav Gumashta 


> On Feb. 21, 2018, 10:07 p.m., Daniel Dai wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
> > Line 311 (original), 215 (patched)
> > 
> >
> > This is not introduced in this patch, but getting columns for table and 
> > apply to partition will not work for schema revolution. We shall get 
> > columns for every individual partition.

I agree, but not sure if current stats works with schema evolution. Let me take 
this up in a follow up jira as this might need a little more thought.


> On Feb. 21, 2018, 10:07 p.m., Daniel Dai wrote:
> > standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
> > Line 800 (original), 632 (patched)
> > 
> >
> > I don't remember but why this is get() not getUnsafe()? It sounds the 
> > same as getAllTables etc. Also apply to getDatabases, alterDatabase, 
> > dropDatabase, getDatabase and createDatabase

We're using get() here so that this call blocks till the database cache is 
populated. We're letting reads go through the cache while the tables are 
getting populated, but not for databases. Let me know if you think otherwise.


- Vaibhav


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


On Feb. 13, 2018, 12:08 p.m., Vaibhav Gumashta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65634/
> ---
> 
> (Updated Feb. 13, 2018, 12:08 p.m.)
> 
> 
> Review request for hive, Daniel Dai and Thejas Nair.
> 
> 
> Bugs: HIVE-18264
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  78b26374f2 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  d58ed677f3 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  e4e7d4239d 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
>  f0f650ddcf 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  80aa3bcdb4 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
>  32ea17495f 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  9100c73beb 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86e72d8d76 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
>  bd61df654a 
> 
> 
> Diff: https://reviews.apache.org/r/65634/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-02-22 Thread Thejas Nair 

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




standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 292 (original), 200 (patched)


It would be more accurate to say "Processed " rather than "Cached " since 
the count includes tables that were skipped.


- Thejas Nair


On Feb. 13, 2018, 12:08 p.m., Vaibhav Gumashta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65634/
> ---
> 
> (Updated Feb. 13, 2018, 12:08 p.m.)
> 
> 
> Review request for hive, Daniel Dai and Thejas Nair.
> 
> 
> Bugs: HIVE-18264
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  78b26374f2 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  d58ed677f3 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  e4e7d4239d 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
>  f0f650ddcf 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  80aa3bcdb4 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
>  32ea17495f 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  9100c73beb 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86e72d8d76 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
>  bd61df654a 
> 
> 
> Diff: https://reviews.apache.org/r/65634/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>

Re: Review Request 65634: HIVE-18264: CachedStore: Store cached partitions/col stats within the table cache

2018-02-21 Thread Daniel Dai 

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




standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
Lines 65 (patched)


key = String.join(delimit, partVals);



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 311 (original), 215 (patched)


This is not introduced in this patch, but getting columns for table and 
apply to partition will not work for schema revolution. We shall get columns 
for every individual partition.



standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
Line 800 (original), 632 (patched)


I don't remember but why this is get() not getUnsafe()? It sounds the same 
as getAllTables etc. Also apply to getDatabases, alterDatabase, dropDatabase, 
getDatabase and createDatabase


- Daniel Dai


On Feb. 13, 2018, 12:08 p.m., Vaibhav Gumashta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65634/
> ---
> 
> (Updated Feb. 13, 2018, 12:08 p.m.)
> 
> 
> Review request for hive, Daniel Dai and Thejas Nair.
> 
> 
> Bugs: HIVE-18264
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> https://issues.apache.org/jira/browse/HIVE-18264
> 
> 
> Diffs
> -
> 
>   
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/DummyRawStoreFailEvent.java
>  78b26374f2 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  d58ed677f3 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
>  e4e7d4239d 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
>  f0f650ddcf 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
>  80aa3bcdb4 
>   
> standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/cache/SharedCache.java
>  32ea17495f 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreControlledCommit.java
>  9100c73beb 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/DummyRawStoreForJdoConnection.java
>  86e72d8d76 
>   
> standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
>  bd61df654a 
> 
> 
> Diff: https://reviews.apache.org/r/65634/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vaibhav Gumashta
> 
>