killbill-aplcache

tenant: optimize tenant key/value caching Put a placeholder

3/18/2015 3:05:26 PM

Details

diff --git a/profiles/killbill/src/main/java/org/killbill/billing/server/notifications/PushNotificationListener.java b/profiles/killbill/src/main/java/org/killbill/billing/server/notifications/PushNotificationListener.java
index 16dc8db..f0dfcdd 100644
--- a/profiles/killbill/src/main/java/org/killbill/billing/server/notifications/PushNotificationListener.java
+++ b/profiles/killbill/src/main/java/org/killbill/billing/server/notifications/PushNotificationListener.java
@@ -75,6 +75,10 @@ public class PushNotificationListener {
         final TenantContext context = contextFactory.createTenantContext(event.getTenantId());
         try {
             final List<String> callbacks = getCallbacksForTenant(context);
+            if (callbacks.isEmpty()) {
+                // Optimization - see https://github.com/killbill/killbill/issues/297
+                return;
+            }
             dispatchCallback(event.getTenantId(), event, callbacks);
         } catch (final TenantApiException e) {
             log.warn("Failed to retrieve push notification callback for tenant {}", event.getTenantId());
diff --git a/tenant/src/main/java/org/killbill/billing/tenant/api/user/DefaultTenantUserApi.java b/tenant/src/main/java/org/killbill/billing/tenant/api/user/DefaultTenantUserApi.java
index 9ff60e6..f1fe45c 100644
--- a/tenant/src/main/java/org/killbill/billing/tenant/api/user/DefaultTenantUserApi.java
+++ b/tenant/src/main/java/org/killbill/billing/tenant/api/user/DefaultTenantUserApi.java
@@ -110,11 +110,11 @@ public class DefaultTenantUserApi implements TenantUserApi {
     @Override
     public List<String> getTenantValuesForKey(final String key, final TenantContext context) throws TenantApiException {
         final InternalTenantContext internalContext = internalCallContextFactory.createInternalTenantContext(context);
-        final String value = getCachedTenantValueForKey(key, internalContext);
-        if (value != null) {
-            return ImmutableList.<String>of(value);
+        if (!isCachedInTenantKVCache(key)) {
+            return tenantDao.getTenantValueForKey(key, internalContext);
+        } else {
+            return getCachedTenantValuesForKey(key, internalContext);
         }
-        return tenantDao.getTenantValueForKey(key, internalContext);
     }
 
     @Override
@@ -135,12 +135,15 @@ public class DefaultTenantUserApi implements TenantUserApi {
         tenantKVCache.remove(tenantKey);
     }
 
-    private String getCachedTenantValueForKey(final String key, final InternalTenantContext internalContext) {
-        if (!isCachedInTenantKVCache(key)) {
-            return null;
-        }
+    private List<String> getCachedTenantValuesForKey(final String key, final InternalTenantContext internalContext) {
         final String tenantKey = getCacheKeyName(key, internalContext);
-        return (String) tenantKVCache.get(tenantKey, new CacheLoaderArgument(ObjectType.TENANT_KVS));
+        final Object cachedTenantValues = tenantKVCache.get(tenantKey, new CacheLoaderArgument(ObjectType.TENANT_KVS));
+        if (cachedTenantValues == null) {
+            return ImmutableList.<String>of();
+        } else {
+            // Current, we only cache single-value keys
+            return ImmutableList.<String>of((String) cachedTenantValues);
+        }
     }
 
     private String getCacheKeyName(final String key, final InternalTenantContext internalContext) {
diff --git a/tenant/src/test/java/org/killbill/billing/tenant/api/user/TestDefaultTenantUserApi.java b/tenant/src/test/java/org/killbill/billing/tenant/api/user/TestDefaultTenantUserApi.java
index b39f4b4..9d70464 100644
--- a/tenant/src/test/java/org/killbill/billing/tenant/api/user/TestDefaultTenantUserApi.java
+++ b/tenant/src/test/java/org/killbill/billing/tenant/api/user/TestDefaultTenantUserApi.java
@@ -46,6 +46,30 @@ public class TestDefaultTenantUserApi extends TenantTestSuiteWithEmbeddedDb {
         Assert.assertEquals(value.size(), 0);
     }
 
+    @Test(groups = "slow", description = "https://github.com/killbill/killbill/issues/297")
+    public void testVerifyCacheOnAbsentValues() throws Exception {
+        final String tenantKey = TenantKey.PLUGIN_CONFIG_.toString() + "MyPluginName";
+
+        // Warm the cache with the empty value
+        List<String> value = tenantUserApi.getTenantValuesForKey(tenantKey, callContext);
+        Assert.assertEquals(value.size(), 0);
+
+        // Update the DAO directly (caching is done at the API layer)
+        tenantDao.addTenantKeyValue(tenantKey, "TheValue-hidden!", true, internalCallContext);
+
+        // Verify we still hit the cache
+        value = tenantUserApi.getTenantValuesForKey(tenantKey, callContext);
+        Assert.assertEquals(value.size(), 0);
+
+        // Update the cache
+        tenantUserApi.addTenantKeyValue(tenantKey, "TheValue", callContext);
+
+        // Verify the cache now has the right value
+        value = tenantUserApi.getTenantValuesForKey(tenantKey, callContext);
+        Assert.assertEquals(value.size(), 1);
+        Assert.assertEquals(value.get(0), "TheValue");
+    }
+
     @Test(groups = "slow")
     public void testSystemKeySingleValue() throws Exception {
         final String tenantKey = TenantKey.PLUGIN_CONFIG_.toString() + "MyPluginName";
diff --git a/util/src/main/java/org/killbill/billing/util/cache/BaseCacheLoader.java b/util/src/main/java/org/killbill/billing/util/cache/BaseCacheLoader.java
index 7aa7472..c0044c6 100644
--- a/util/src/main/java/org/killbill/billing/util/cache/BaseCacheLoader.java
+++ b/util/src/main/java/org/killbill/billing/util/cache/BaseCacheLoader.java
@@ -1,7 +1,9 @@
 /*
  * Copyright 2010-2013 Ning, Inc.
+ * Copyright 2014-2015 Groupon, Inc
+ * Copyright 2014-2015 The Billing Project, LLC
  *
- * Ning licenses this file to you under the Apache License, version 2.0
+ * The Billing Project licenses this file to you under the Apache License, version 2.0
  * (the "License"); you may not use this file except in compliance with the
  * License.  You may obtain a copy of the License at:
  *
@@ -30,6 +32,8 @@ import net.sf.ehcache.loader.CacheLoader;
 
 public abstract class BaseCacheLoader implements CacheLoader {
 
+    static final String EMPTY_VALUE_PLACEHOLDER = "__#VALEUR!__";
+
     private Status cacheLoaderStatus;
 
     @Inject
diff --git a/util/src/main/java/org/killbill/billing/util/cache/EhCacheBasedCacheController.java b/util/src/main/java/org/killbill/billing/util/cache/EhCacheBasedCacheController.java
index ce26eb3..1f7998f 100644
--- a/util/src/main/java/org/killbill/billing/util/cache/EhCacheBasedCacheController.java
+++ b/util/src/main/java/org/killbill/billing/util/cache/EhCacheBasedCacheController.java
@@ -41,7 +41,7 @@ public class EhCacheBasedCacheController<K, V> implements CacheController<K, V> 
     @Override
     public V get(final K key, final CacheLoaderArgument cacheLoaderArgument) {
         final Element element = cache.getWithLoader(key, null, cacheLoaderArgument);
-        if (element == null) {
+        if (element == null || element.getObjectValue() == null || element.getObjectValue().equals(BaseCacheLoader.EMPTY_VALUE_PLACEHOLDER)) {
             return null;
         }
         return (V) element.getObjectValue();
diff --git a/util/src/main/java/org/killbill/billing/util/cache/TenantKVCacheLoader.java b/util/src/main/java/org/killbill/billing/util/cache/TenantKVCacheLoader.java
index 02b709f..a949445 100644
--- a/util/src/main/java/org/killbill/billing/util/cache/TenantKVCacheLoader.java
+++ b/util/src/main/java/org/killbill/billing/util/cache/TenantKVCacheLoader.java
@@ -59,7 +59,7 @@ public class TenantKVCacheLoader extends BaseCacheLoader {
         final InternalTenantContext internalTenantContext = new InternalTenantContext(Long.valueOf(tenantRecordId));
         final List<String> valuesForKey = tenantApi.getTenantValuesForKey(rawKey, internalTenantContext);
         if (valuesForKey == null || valuesForKey.isEmpty()) {
-            return null;
+            return EMPTY_VALUE_PLACEHOLDER;
         }
         if (valuesForKey.size() > 1) {
             throw new IllegalStateException("TenantKVCacheLoader expecting no more than one value for key " + key);