This is an automated email from the ASF dual-hosted git repository. jxue pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/helix.git
commit 8f75ae83c04218544a8637315ee4138edce6fd95 Author: ywang4 <[email protected]> AuthorDate: Mon Feb 11 14:59:59 2019 -0800 Add unit test for PropertyCache class --- .../main/java/org/apache/helix/PropertyKey.java | 10 +- .../apache/helix/common/caches/PropertyCache.java | 56 +++++---- .../helix/common/caches/TestPropertyCache.java | 132 +++++++++++++++++++++ 3 files changed, 165 insertions(+), 33 deletions(-) diff --git a/helix-core/src/main/java/org/apache/helix/PropertyKey.java b/helix-core/src/main/java/org/apache/helix/PropertyKey.java index e16198d..2ddc8bb 100644 --- a/helix-core/src/main/java/org/apache/helix/PropertyKey.java +++ b/helix-core/src/main/java/org/apache/helix/PropertyKey.java @@ -19,10 +19,14 @@ package org.apache.helix; * under the License. */ +import static org.apache.helix.PropertyType.*; + import java.util.Arrays; +import java.util.Objects; import org.apache.helix.model.ClusterConfig; import org.apache.helix.model.ClusterConstraints; +import org.apache.helix.model.ControllerHistory; import org.apache.helix.model.CurrentState; import org.apache.helix.model.Error; import org.apache.helix.model.ExternalView; @@ -30,7 +34,6 @@ import org.apache.helix.model.HealthStat; import org.apache.helix.model.HelixConfigScope.ConfigScopeProperty; import org.apache.helix.model.IdealState; import org.apache.helix.model.InstanceConfig; -import org.apache.helix.model.ControllerHistory; import org.apache.helix.model.LiveInstance; import org.apache.helix.model.MaintenanceSignal; import org.apache.helix.model.Message; @@ -46,8 +49,6 @@ import org.apache.helix.task.WorkflowContext; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.apache.helix.PropertyType.*; - /** * Key allowing for type-safe lookups of and conversions to {@link HelixProperty} objects. */ @@ -123,7 +124,8 @@ public class PropertyKey { if (!Arrays.equals(_params, key._params)) { return false; } - if (!_typeClazz.equals(key._typeClazz)) { + // Avoid NPE when one typeClazz is null + if (!Objects.equals(_typeClazz, key._typeClazz)) { return false; } return _configScope == key._configScope; diff --git a/helix-core/src/main/java/org/apache/helix/common/caches/PropertyCache.java b/helix-core/src/main/java/org/apache/helix/common/caches/PropertyCache.java index 3820bff..6010c1d 100644 --- a/helix-core/src/main/java/org/apache/helix/common/caches/PropertyCache.java +++ b/helix-core/src/main/java/org/apache/helix/common/caches/PropertyCache.java @@ -22,10 +22,10 @@ package org.apache.helix.common.caches; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; + import org.apache.helix.HelixDataAccessor; import org.apache.helix.HelixProperty; import org.apache.helix.PropertyKey; @@ -34,6 +34,9 @@ import org.apache.helix.controller.LogUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.Sets; + /** * A general cache for HelixProperty that supports LIST, GET, SET, DELETE methods of Helix property. * All operation is in memory and is not persisted into the data store, but it provides a method to @@ -87,8 +90,8 @@ public class PropertyCache<T extends HelixProperty> extends AbstractDataCache<T> private final boolean _useSelectiveUpdate; private final PropertyCacheKeyFuncs<T> _keyFuncs; - public PropertyCache(ControlContextProvider contextProvider, String propertyDescription, PropertyCacheKeyFuncs<T> keyFuncs, - boolean useSelectiveUpdate) { + public PropertyCache(ControlContextProvider contextProvider, String propertyDescription, + PropertyCacheKeyFuncs<T> keyFuncs, boolean useSelectiveUpdate) { super(contextProvider); _propertyDescription = propertyDescription; _keyFuncs = keyFuncs; @@ -97,13 +100,13 @@ public class PropertyCache<T extends HelixProperty> extends AbstractDataCache<T> _useSelectiveUpdate = useSelectiveUpdate; } - protected class SelectivePropertyRefreshInputs { + static class SelectivePropertyRefreshInputs<K extends HelixProperty> { private final List<PropertyKey> reloadKeys; private final List<PropertyKey> cachedKeys; - private final Map<PropertyKey, T> cachedPropertyMap; + private final Map<PropertyKey, K> cachedPropertyMap; SelectivePropertyRefreshInputs(List<PropertyKey> keysToReload, - List<PropertyKey> currentlyCachedKeys, Map<PropertyKey, T> currentCache) { + List<PropertyKey> currentlyCachedKeys, Map<PropertyKey, K> currentCache) { reloadKeys = keysToReload; cachedKeys = currentlyCachedKeys; cachedPropertyMap = currentCache; @@ -117,35 +120,31 @@ public class PropertyCache<T extends HelixProperty> extends AbstractDataCache<T> return reloadKeys; } - Map<PropertyKey, T> getCachedPropertyMap() { + Map<PropertyKey, K> getCachedPropertyMap() { return cachedPropertyMap; } } - @SuppressWarnings("unchecked") - private SelectivePropertyRefreshInputs genSelectiveUpdateInput( - HelixDataAccessor accessor, Map<String, T> currentCache, PropertyCache.PropertyCacheKeyFuncs<T> propertyKeyFuncs) { + @VisibleForTesting + SelectivePropertyRefreshInputs<T> genSelectiveUpdateInput(HelixDataAccessor accessor, + Map<String, T> oldCache, PropertyCache.PropertyCacheKeyFuncs<T> propertyKeyFuncs) { // Generate keys for all current live instances - Set<PropertyKey> curObjKeys = new HashSet<>(); + Set<PropertyKey> latestKeys = Sets.newHashSet(); for (String liveInstanceName : accessor.getChildNames(propertyKeyFuncs.getRootKey(accessor))) { - curObjKeys.add(propertyKeyFuncs.getObjPropertyKey(accessor, liveInstanceName)); + latestKeys.add(propertyKeyFuncs.getObjPropertyKey(accessor, liveInstanceName)); } - // Generate cached objects - Set<PropertyKey> cachedKeys = new HashSet<>(); + Set<PropertyKey> oldCachedKeys = Sets.newHashSet(); Map<PropertyKey, T> cachedObjs = new HashMap<>(); - for (String objName : currentCache.keySet()) { + for (String objName : oldCache.keySet()) { PropertyKey objKey = propertyKeyFuncs.getObjPropertyKey(accessor, objName); - cachedKeys.add(objKey); - cachedObjs.put(objKey, currentCache.get(objName)); + oldCachedKeys.add(objKey); + cachedObjs.put(objKey, oldCache.get(objName)); } - cachedKeys.retainAll(curObjKeys); - - // Generate keys to reload - Set<PropertyKey> reloadKeys = new HashSet<>(curObjKeys); - reloadKeys.removeAll(cachedKeys); + Set<PropertyKey> cachedKeys = Sets.intersection(oldCachedKeys, latestKeys); + Set<PropertyKey> reloadKeys = Sets.difference(latestKeys, cachedKeys); - return new SelectivePropertyRefreshInputs(new ArrayList<>(reloadKeys), + return new SelectivePropertyRefreshInputs<>(new ArrayList<>(reloadKeys), new ArrayList<>(cachedKeys), cachedObjs); } @@ -160,8 +159,8 @@ public class PropertyCache<T extends HelixProperty> extends AbstractDataCache<T> } else { doSimpleCacheRefresh(accessor); } - LogUtil.logInfo(LOG, genEventInfo(), String - .format("Refreshed %s property %s took %s ms. Selective: %s", _objMap.size(), + LogUtil.logInfo(LOG, genEventInfo(), + String.format("Refreshed %s property %s took %s ms. Selective: %s", _objMap.size(), _propertyDescription, System.currentTimeMillis() - start, _useSelectiveUpdate)); } @@ -171,11 +170,10 @@ public class PropertyCache<T extends HelixProperty> extends AbstractDataCache<T> } private void doRefreshWithSelectiveUpdate(final HelixDataAccessor accessor) { - SelectivePropertyRefreshInputs input = + SelectivePropertyRefreshInputs<T> input = genSelectiveUpdateInput(accessor, _objCache, _keyFuncs); - Map<PropertyKey, T> updatedData = - refreshProperties(accessor, input.getReloadKeys(), input.getCachedKeys(), - input.getCachedPropertyMap()); + Map<PropertyKey, T> updatedData = refreshProperties(accessor, input.getReloadKeys(), + input.getCachedKeys(), input.getCachedPropertyMap()); _objCache = propertyKeyMapToStringMap(updatedData, _keyFuncs); // need to separate keys so we can potentially update cache map asynchronously while diff --git a/helix-core/src/test/java/org/apache/helix/common/caches/TestPropertyCache.java b/helix-core/src/test/java/org/apache/helix/common/caches/TestPropertyCache.java new file mode 100644 index 0000000..7b0382e --- /dev/null +++ b/helix-core/src/test/java/org/apache/helix/common/caches/TestPropertyCache.java @@ -0,0 +1,132 @@ +package org.apache.helix.common.caches; + +import static org.mockito.Mockito.*; + +import java.util.HashMap; +import java.util.Map; + +import org.apache.helix.HelixDataAccessor; +import org.apache.helix.HelixProperty; +import org.apache.helix.PropertyKey; +import org.apache.helix.common.controllers.ControlContextProvider; +import org.apache.helix.model.InstanceConfig; +import org.testng.Assert; +import org.testng.annotations.Test; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; + +/** + * Unit test for {@link PropertyCache} + */ +public class TestPropertyCache { + private static final ControlContextProvider MOCK_CONTROL_CONTEXT_PROVIDER = + new ControlContextProvider() { + @Override + public String getClusterName() { + return "mockCluster"; + } + + @Override + public String getClusterEventId() { + return "id"; + } + + @Override + public void setClusterEventId(String eventId) { + + } + + @Override + public String getPipelineName() { + return "pipeline"; + } + }; + + @Test(description = "Unit test for simple cache refresh") + public void testSimpleCacheRefresh() { + @SuppressWarnings("unchecked") + PropertyCache<HelixProperty> propertyCache = new PropertyCache<>(MOCK_CONTROL_CONTEXT_PROVIDER, + "mock property cache", mock(PropertyCache.PropertyCacheKeyFuncs.class), false); + HelixDataAccessor accessor = mock(HelixDataAccessor.class); + Map<String, HelixProperty> propertyConfigMap = ImmutableMap.of("id", new HelixProperty("test")); + when(accessor.getChildValuesMap(any(PropertyKey.class), any(Boolean.class))) + .thenReturn(propertyConfigMap); + + propertyCache.refresh(accessor); + + Assert.assertEquals(propertyCache.getPropertyMap(), propertyConfigMap); + Assert.assertEquals(propertyCache.getPropertyByName("id"), new HelixProperty("test")); + } + + @Test(description = "Unit test for selective cache refresh") + public void testSelectivePropertyRefreshInputs() { + HelixDataAccessor accessor = mock(HelixDataAccessor.class); + Map<String, HelixProperty> currentCache = ImmutableMap.of("instance0", + new HelixProperty("key0"), "instance1", new HelixProperty("key1")); + + PropertyCache.PropertyCacheKeyFuncs<HelixProperty> mockCacheKeyFuncs = + new PropertyCache.PropertyCacheKeyFuncs<HelixProperty>() { + @Override + public PropertyKey getRootKey(HelixDataAccessor accessor) { + return mock(PropertyKey.class); + } + + @Override + public PropertyKey getObjPropertyKey(HelixDataAccessor accessor, String objName) { + return new PropertyKey.Builder("fake").instance(objName); + } + + @Override + public String getObjName(HelixProperty obj) { + return obj.getRecord().getId(); + } + }; + when(accessor.getChildNames(any(PropertyKey.class))) + .thenReturn(ImmutableList.of("instance1", "instance2")); + @SuppressWarnings("unchecked") + PropertyCache<HelixProperty> propertyCache = new PropertyCache<>(MOCK_CONTROL_CONTEXT_PROVIDER, + "mock property cache", mock(PropertyCache.PropertyCacheKeyFuncs.class), false); + + PropertyCache.SelectivePropertyRefreshInputs<HelixProperty> selectivePropertyRefreshInputs = + propertyCache.genSelectiveUpdateInput(accessor, currentCache, mockCacheKeyFuncs); + + Assert.assertEquals(selectivePropertyRefreshInputs.getReloadKeys().size(), 1); + Assert.assertEquals(selectivePropertyRefreshInputs.getReloadKeys().get(0), + new PropertyKey.Builder("fake").instance("instance2")); + } + + @Test(description = "First set the property cache and update the object from caller") + public void testDefensiveCopyOnDataUpdate() { + @SuppressWarnings("unchecked") + PropertyCache<HelixProperty> propertyCache = new PropertyCache<>(MOCK_CONTROL_CONTEXT_PROVIDER, + "mock property cache", mock(PropertyCache.PropertyCacheKeyFuncs.class), false); + HelixProperty helixProperty = new HelixProperty("id"); + Map<String, HelixProperty> propertyConfigMap = new HashMap<>(); + + propertyCache.setPropertyMap(propertyConfigMap); + // increment the property map from outside + propertyConfigMap.put("id", helixProperty); + + Assert.assertTrue(propertyCache.getPropertyMap().isEmpty()); + } + + //TODO investigate if deep copy is needed for PropertyCache + @Test(enabled = false, description = "First set the property cache and mutate the object from caller") + public void testDefensiveCopyOnDataMutate() { + // init + @SuppressWarnings("unchecked") + PropertyCache<InstanceConfig> propertyCache = new PropertyCache<>(MOCK_CONTROL_CONTEXT_PROVIDER, + "mock property cache", mock(PropertyCache.PropertyCacheKeyFuncs.class), false); + InstanceConfig instanceConfig = new InstanceConfig("id"); + Map<String, InstanceConfig> propertyConfigMap = ImmutableMap.of("id", instanceConfig); + + propertyCache.setPropertyMap(propertyConfigMap); + // mutate the property from outside + instanceConfig.setHostName("fakeHost"); + String hostName = propertyCache.getPropertyByName("id").getHostName(); + Assert.assertTrue(hostName.isEmpty()); + } +}
