You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@griffin.apache.org by gu...@apache.org on 2018/09/22 13:46:41 UTC
incubator-griffin git commit: [GRIFFIN-198] Fix caching of incomplete
results in HiveMetaStoreService
Repository: incubator-griffin
Updated Branches:
refs/heads/master 8154346f6 -> 5c88d9ca4
[GRIFFIN-198] Fix caching of incomplete results in HiveMetaStoreService
Avoiding polluting cache with empty or null values.
Test caching behavior of HiveMetaStoreServiceImpl.
Verified that cache population in evictHiveCache() is not happening, added unittest.
This is only partial fix, getAllTable() calls still can return and cache incomplete results.
Author: Nikolay Sokolov <ch...@gmail.com>
Closes #419 from chemikadze/hive-caching.
Project: http://git-wip-us.apache.org/repos/asf/incubator-griffin/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-griffin/commit/5c88d9ca
Tree: http://git-wip-us.apache.org/repos/asf/incubator-griffin/tree/5c88d9ca
Diff: http://git-wip-us.apache.org/repos/asf/incubator-griffin/diff/5c88d9ca
Branch: refs/heads/master
Commit: 5c88d9ca43fa96afdfbed37a54b5c7bbe7d541d8
Parents: 8154346
Author: Nikolay Sokolov <ch...@gmail.com>
Authored: Sat Sep 22 21:46:32 2018 +0800
Committer: William Guo <gu...@apache.org>
Committed: Sat Sep 22 21:46:32 2018 +0800
----------------------------------------------------------------------
.travis.yml | 2 +-
.../metastore/hive/HiveMetaStoreService.java | 2 +
.../hive/HiveMetaStoreServiceImpl.java | 20 +++---
.../hive/HiveMetaStoreServiceImplTest.java | 73 ++++++++++++++++++--
4 files changed, 83 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-griffin/blob/5c88d9ca/.travis.yml
----------------------------------------------------------------------
diff --git a/.travis.yml b/.travis.yml
index 143b665..713b27e 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -24,4 +24,4 @@ node_js:
install:
- npm install -g bower
-script: mvn clean verify -q
+script: travis_wait mvn clean verify -q
http://git-wip-us.apache.org/repos/asf/incubator-griffin/blob/5c88d9ca/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreService.java
----------------------------------------------------------------------
diff --git a/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreService.java b/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreService.java
index 1dcea1b..4ef7690 100644
--- a/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreService.java
+++ b/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreService.java
@@ -35,4 +35,6 @@ public interface HiveMetaStoreService {
Map<String, List<Table>> getAllTable();
Table getTable(String dbName, String tableName);
+
+ void evictHiveCache();
}
http://git-wip-us.apache.org/repos/asf/incubator-griffin/blob/5c88d9ca/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImpl.java
----------------------------------------------------------------------
diff --git a/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImpl.java b/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImpl.java
index 2ec62c8..f5014e7 100644
--- a/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImpl.java
+++ b/service/src/main/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImpl.java
@@ -56,7 +56,7 @@ public class HiveMetaStoreServiceImpl implements HiveMetaStoreService {
}
@Override
- @Cacheable
+ @Cacheable(unless = "#result==null")
public Iterable<String> getAllDatabases() {
Iterable<String> results = null;
try {
@@ -75,7 +75,7 @@ public class HiveMetaStoreServiceImpl implements HiveMetaStoreService {
@Override
- @Cacheable
+ @Cacheable(unless = "#result==null")
public Iterable<String> getAllTableNames(String dbName) {
Iterable<String> results = null;
try {
@@ -88,20 +88,21 @@ public class HiveMetaStoreServiceImpl implements HiveMetaStoreService {
} catch (Exception e) {
reconnect();
LOGGER.error("Exception fetching tables info: {}", e);
+ return null;
}
return results;
}
@Override
- @Cacheable
+ @Cacheable(unless = "#result==null || #result.isEmpty()")
public List<Table> getAllTable(String db) {
return getTables(db);
}
@Override
- @Cacheable
+ @Cacheable(unless = "#result==null")
public Map<String, List<Table>> getAllTable() {
Map<String, List<Table>> results = new HashMap<>();
Iterable<String> dbs;
@@ -116,6 +117,8 @@ public class HiveMetaStoreServiceImpl implements HiveMetaStoreService {
return results;
}
for (String db : dbs) {
+ // TODO: getAllTable() is not reusing caches of getAllTable(db) and vise versa
+ // TODO: getTables() can return empty values on metastore exception
results.put(db, getTables(db));
}
return results;
@@ -123,7 +126,7 @@ public class HiveMetaStoreServiceImpl implements HiveMetaStoreService {
@Override
- @Cacheable
+ @Cacheable(unless="#result==null")
public Table getTable(String dbName, String tableName) {
Table result = null;
try {
@@ -149,9 +152,10 @@ public class HiveMetaStoreServiceImpl implements HiveMetaStoreService {
beforeInvocation = true)
public void evictHiveCache() {
LOGGER.info("Evict hive cache");
- getAllTable();
- LOGGER.info("After evict hive cache, " +
- "automatically refresh hive tables cache.");
+ // TODO: calls within same bean are not cached -- this call is not populating anything
+// getAllTable();
+// LOGGER.info("After evict hive cache, " +
+// "automatically refresh hive tables cache.");
}
http://git-wip-us.apache.org/repos/asf/incubator-griffin/blob/5c88d9ca/service/src/test/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImplTest.java
----------------------------------------------------------------------
diff --git a/service/src/test/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImplTest.java b/service/src/test/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImplTest.java
index 358abea..a24f9b3 100644
--- a/service/src/test/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImplTest.java
+++ b/service/src/test/java/org/apache/griffin/core/metastore/hive/HiveMetaStoreServiceImplTest.java
@@ -22,21 +22,26 @@ package org.apache.griffin.core.metastore.hive;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.mockito.BDDMockito.given;
-import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.*;
import java.util.Arrays;
import java.util.List;
+import org.apache.griffin.core.config.CacheConfig;
import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
import org.apache.hadoop.hive.metastore.api.MetaException;
import org.apache.hadoop.hive.metastore.api.Table;
import org.apache.thrift.TException;
import org.junit.Before;
+import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.TestConfiguration;
import org.springframework.boot.test.mock.mockito.MockBean;
+import org.springframework.cache.CacheManager;
+import org.springframework.cache.annotation.EnableCaching;
+import org.springframework.cache.concurrent.ConcurrentMapCacheManager;
import org.springframework.context.annotation.Bean;
import org.springframework.test.context.junit4.SpringRunner;
@@ -45,11 +50,17 @@ import org.springframework.test.context.junit4.SpringRunner;
public class HiveMetaStoreServiceImplTest {
@TestConfiguration
- public static class HiveMetaStoreServiceConfiguration {
+ @EnableCaching
+ public static class HiveMetaStoreServiceConfiguration extends CacheConfig {
@Bean("hiveMetaStoreServiceImpl")
public HiveMetaStoreService service() {
return new HiveMetaStoreServiceImpl();
}
+
+ @Bean
+ CacheManager cacheManager() {
+ return new ConcurrentMapCacheManager("hive");
+ }
}
@MockBean
@@ -58,9 +69,12 @@ public class HiveMetaStoreServiceImplTest {
@Autowired
private HiveMetaStoreService service;
+ @Autowired
+ private CacheManager cacheManager;
+
@Before
public void setup() {
-
+ cacheManager.getCache("hive").clear();
}
@Test
@@ -73,8 +87,13 @@ public class HiveMetaStoreServiceImplTest {
public void testGetAllDatabasesForMetaException() throws MetaException {
given(client.getAllDatabases()).willThrow(MetaException.class);
doNothing().when(client).reconnect();
- service.getAllDatabases();
assertTrue(service.getAllDatabases() == null);
+ verify(client).getAllDatabases();
+ verify(client).reconnect();
+ // check it's not cached
+ service.getAllDatabases();
+ verify(client, times(2)).reconnect();
+ verify(client, times(2)).getAllDatabases();
}
@@ -92,6 +111,12 @@ public class HiveMetaStoreServiceImplTest {
given(client.getAllTables(dbName)).willThrow(MetaException.class);
doNothing().when(client).reconnect();
assertTrue(service.getAllTableNames(dbName) == null);
+ verify(client).reconnect();
+ verify(client).getAllTables(dbName);
+ // check it's not cached
+ service.getAllTableNames(dbName);
+ verify(client, times(2)).reconnect();
+ verify(client, times(2)).getAllTables(dbName);
}
@@ -110,7 +135,13 @@ public class HiveMetaStoreServiceImplTest {
String useDbName = "default";
given(client.getAllTables(useDbName)).willThrow(MetaException.class);
doNothing().when(client).reconnect();
- assertEquals(service.getAllTable(useDbName).size(), 0);
+ assertEquals(0, service.getAllTable(useDbName).size());
+ verify(client).reconnect();
+ verify(client).getAllTables(useDbName);
+ // check it's not cached
+ service.getAllTable(useDbName);
+ verify(client, times(2)).reconnect();
+ verify(client, times(2)).getAllTables(useDbName);
}
@Test
@@ -157,5 +188,37 @@ public class HiveMetaStoreServiceImplTest {
given(client.getTable(dbName, tableName)).willThrow(Exception.class);
doNothing().when(client).reconnect();
assertTrue(service.getTable(dbName, tableName) == null);
+ verify(client).reconnect();
+ verify(client).getTable(dbName, tableName);
+ // check it's not cached
+ service.getTable(dbName, tableName);
+ verify(client, times(2)).reconnect();
+ verify(client, times(2)).getTable(dbName, tableName);
+ }
+
+ @Test
+ public void testEvictHiveCache() throws Exception {
+ String useDbName = "default";
+ String tableName = "tableName";
+ List<String> databases = Arrays.asList(useDbName);
+ given(client.getAllDatabases()).willReturn(databases);
+ given(client.getAllTables(databases.get(0))).willReturn(Arrays
+ .asList(tableName));
+ given(client.getTable(useDbName, tableName)).willReturn(new Table());
+ // populate cache
+ assertEquals(service.getAllTable().size(), 1);
+ verify(client).getAllDatabases();
+ verify(client).getAllTables(useDbName);
+ verify(client).getTable(useDbName, tableName);
+ // verify cached
+ service.getAllTable();
+ verifyNoMoreInteractions(client);
+ // reset the cache, verify values are cached again
+ service.evictHiveCache();
+ service.getAllTable().size();
+ service.getAllTable().size();
+ verify(client, times(2)).getAllDatabases();
+ verify(client, times(2)).getAllTables(useDbName);
+ verify(client, times(2)).getTable(useDbName, tableName);
}
}