Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Copy Nearcacheconfig when it needs to be changed #18348

Merged
merged 2 commits into from
Mar 4, 2021
Merged

Copy Nearcacheconfig when it needs to be changed #18348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a5f452
Revert "Make NearcacheConfig equal before and after it is used"
sancar Mar 3, 2021
e5051d2
Copy Nearcacheconfig when it needs to be changed
sancar Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions hazelcast/src/main/java/com/hazelcast/client/impl/clientside/DefaultClientExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
import static com.hazelcast.config.InstanceTrackingConfig.InstanceTrackingProperties.PRODUCT;
import static com.hazelcast.config.InstanceTrackingConfig.InstanceTrackingProperties.START_TIMESTAMP;
import static com.hazelcast.config.InstanceTrackingConfig.InstanceTrackingProperties.VERSION;
import static com.hazelcast.config.NearCacheConfigAccessor.initDefaultMaxSizeForOnHeapMaps;
import static com.hazelcast.internal.config.ConfigValidator.checkNearCacheConfig;
import static com.hazelcast.internal.util.ExceptionUtil.rethrow;
import static com.hazelcast.internal.util.InstanceTrackingUtil.writeInstanceTrackingFile;
Expand Down Expand Up @@ -208,7 +207,6 @@ private ClientProxyFactory createClientMapProxyFactory() {
NearCacheConfig nearCacheConfig = clientConfig.getNearCacheConfig(id);
if (nearCacheConfig != null) {
checkNearCacheConfig(id, nearCacheConfig, clientConfig.getNativeMemoryConfig(), true);
initDefaultMaxSizeForOnHeapMaps(nearCacheConfig);
return new NearCachedClientMapProxy(MapService.SERVICE_NAME, id, context);
} else {
return new ClientMapProxy(MapService.SERVICE_NAME, id, context);
Expand Down
2 changes: 0 additions & 2 deletions hazelcast/src/main/java/com/hazelcast/config/Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;

import static com.hazelcast.config.NearCacheConfigAccessor.initDefaultMaxSizeForOnHeapMaps;
import static com.hazelcast.internal.config.ConfigUtils.lookupByPattern;
import static com.hazelcast.internal.config.DeclarativeConfigUtil.SYSPROP_MEMBER_CONFIG;
import static com.hazelcast.internal.config.DeclarativeConfigUtil.validateSuffixInSystemProperty;
Expand Down Expand Up @@ -461,7 +460,6 @@ public MapConfig findMapConfig(String name) {
name = getBaseName(name);
MapConfig config = lookupByPattern(configPatternMatcher, mapConfigs, name);
if (config != null) {
initDefaultMaxSizeForOnHeapMaps(config.getNearCacheConfig());
return new MapConfigReadOnly(config);
}
return new MapConfigReadOnly(getMapConfig("default"));
Expand Down
27 changes: 15 additions & 12 deletions hazelcast/src/main/java/com/hazelcast/config/EvictionConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,24 @@ public class EvictionConfig implements EvictionConfiguration, IdentifiedDataSeri
*/
public static final EvictionPolicy DEFAULT_EVICTION_POLICY = EvictionPolicy.LRU;

private static final int UNSET = -1;
private MaxSizePolicy maxSizePolicy = DEFAULT_MAX_SIZE_POLICY;
private EvictionPolicy evictionPolicy = DEFAULT_EVICTION_POLICY;
private String comparatorClassName;
private EvictionPolicyComparator comparator;
private int size = UNSET;
int defaultSize = DEFAULT_MAX_ENTRY_COUNT;
protected int size = DEFAULT_MAX_ENTRY_COUNT;
protected MaxSizePolicy maxSizePolicy = DEFAULT_MAX_SIZE_POLICY;
protected EvictionPolicy evictionPolicy = DEFAULT_EVICTION_POLICY;

protected String comparatorClassName;
protected EvictionPolicyComparator comparator;

/**
* Used by the {@link NearCacheConfigAccessor} to
* initialize the proper default value for on-heap maps.
*/
boolean sizeConfigured;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if sizeConfigured was set to true, after deserialization what keeps it still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reverted the EvictionConfig behaviour. So it seems that setting sizeConfigured on deserialization was not necessary. For the sake of keeping it backward compatible, I will not touch this. Lets consider if that is necessary as a separate fix if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, i see this is an existing issue, not introduced in this PR. Created an issue for it: #18354


public EvictionConfig() {
}

public EvictionConfig(EvictionConfig config) {
this.defaultSize = config.defaultSize;
this.sizeConfigured = true;
this.size = config.size;
this.maxSizePolicy = config.maxSizePolicy;
this.evictionPolicy = config.evictionPolicy;
Expand All @@ -95,9 +100,6 @@ public EvictionConfig(EvictionConfig config) {
* @return the size which is used by the {@link MaxSizePolicy}
*/
public int getSize() {
if (size == UNSET) {
return defaultSize;
}
return size;
}

Expand All @@ -114,6 +116,7 @@ public int getSize() {
* @return this EvictionConfig instance
*/
public EvictionConfig setSize(int size) {
this.sizeConfigured = true;
this.size = checkNotNegative(size,
"size cannot be a negative number!");
return this;
Expand Down Expand Up @@ -284,7 +287,7 @@ public final boolean equals(Object o) {

EvictionConfig that = (EvictionConfig) o;

return ((size == UNSET && that.size == UNSET) || getSize() == that.getSize())
return size == that.size
&& maxSizePolicy == that.maxSizePolicy
&& evictionPolicy == that.evictionPolicy
&& Objects.equals(comparatorClassName, that.comparatorClassName)
Expand Down
17 changes: 13 additions & 4 deletions hazelcast/src/main/java/com/hazelcast/config/NearCacheConfigAccessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,23 @@ public final class NearCacheConfigAccessor {
private NearCacheConfigAccessor() {
}

public static void initDefaultMaxSizeForOnHeapMaps(NearCacheConfig nearCacheConfig) {
public static NearCacheConfig copyWithInitializedDefaultMaxSizeForOnHeapMaps(NearCacheConfig nearCacheConfig) {
if (nearCacheConfig == null) {
return;
return null;
}

EvictionConfig evictionConfig = nearCacheConfig.getEvictionConfig();
if (nearCacheConfig.getInMemoryFormat() != InMemoryFormat.NATIVE) {
evictionConfig.defaultSize = MapConfig.DEFAULT_MAX_SIZE;
if (nearCacheConfig.getInMemoryFormat() == InMemoryFormat.NATIVE
|| evictionConfig.sizeConfigured) {
return nearCacheConfig;
}

// create copy of eviction config
EvictionConfig copyEvictionConfig = new EvictionConfig(evictionConfig)
.setSize(MapConfig.DEFAULT_MAX_SIZE);

// create copy of nearCache config and set eviction config
return new NearCacheConfig(nearCacheConfig)
.setEvictionConfig(copyEvictionConfig);
}
}
6 changes: 0 additions & 6 deletions ...lcast/src/main/java/com/hazelcast/internal/dynamicconfig/search/DynamicFirstSearcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@

import com.hazelcast.config.Config;
import com.hazelcast.config.ConfigPatternMatcher;
import com.hazelcast.config.MapConfig;
import com.hazelcast.internal.dynamicconfig.ConfigurationService;
import com.hazelcast.nio.serialization.IdentifiedDataSerializable;

import javax.annotation.Nonnull;
import java.util.Map;

import static com.hazelcast.config.NearCacheConfigAccessor.initDefaultMaxSizeForOnHeapMaps;
import static com.hazelcast.internal.config.ConfigUtils.lookupByPattern;
import static com.hazelcast.partition.strategy.StringPartitioningStrategy.getBaseName;

Expand All @@ -48,10 +46,6 @@ public T getConfig(@Nonnull String name, String fallbackName, @Nonnull ConfigSup
T config = configSupplier.getDynamicConfig(configurationService, baseName);
if (config == null) {
config = lookupByPattern(configPatternMatcher, staticCacheConfigs, baseName);
if (config != null && MapConfig.class.isAssignableFrom(config.getClass())) {
// this is required only for map config
initDefaultMaxSizeForOnHeapMaps(((MapConfig) config).getNearCacheConfig());
}
}
if (config == null) {
config = configSupplier.getStaticConfig(staticConfig, fallbackName);
Expand Down
5 changes: 0 additions & 5 deletions hazelcast/src/main/java/com/hazelcast/internal/dynamicconfig/search/StaticFirstSearcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@

import com.hazelcast.config.Config;
import com.hazelcast.config.ConfigPatternMatcher;
import com.hazelcast.config.MapConfig;
import com.hazelcast.internal.dynamicconfig.ConfigurationService;
import com.hazelcast.nio.serialization.IdentifiedDataSerializable;

import javax.annotation.Nonnull;
import java.util.Map;

import static com.hazelcast.config.NearCacheConfigAccessor.initDefaultMaxSizeForOnHeapMaps;
import static com.hazelcast.internal.config.ConfigUtils.lookupByPattern;
import static com.hazelcast.partition.strategy.StringPartitioningStrategy.getBaseName;

Expand All @@ -48,9 +46,6 @@ public T getConfig(@Nonnull String name, String fallbackName, @Nonnull ConfigSup
T config = lookupByPattern(configPatternMatcher, staticCacheConfigs, baseName);
if (config == null) {
config = configSupplier.getDynamicConfig(configurationService, baseName);
} else if (MapConfig.class.isAssignableFrom(config.getClass())) {
// this is required only for map config
initDefaultMaxSizeForOnHeapMaps(((MapConfig) config).getNearCacheConfig());
}
if (config == null && fallbackName != null) {
config = configSupplier.getStaticConfig(staticConfig, fallbackName);
Expand Down
4 changes: 3 additions & 1 deletion hazelcast/src/main/java/com/hazelcast/internal/nearcache/impl/DefaultNearCacheManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.hazelcast.internal.nearcache.impl;

import com.hazelcast.config.NearCacheConfig;
import com.hazelcast.config.NearCacheConfigAccessor;
import com.hazelcast.config.NearCachePreloaderConfig;
import com.hazelcast.internal.adapter.DataStructureAdapter;
import com.hazelcast.internal.nearcache.NearCache;
Expand Down Expand Up @@ -89,7 +90,8 @@ public <K, V> NearCache<K, V> getOrCreateNearCache(String name,
}

protected <K, V> NearCache<K, V> createNearCache(String name, NearCacheConfig nearCacheConfig) {
return new DefaultNearCache<>(name, nearCacheConfig, serializationService,
NearCacheConfig copy = NearCacheConfigAccessor.copyWithInitializedDefaultMaxSizeForOnHeapMaps(nearCacheConfig);
return new DefaultNearCache<>(name, copy, serializationService,
scheduler, classLoader, properties);
}

Expand Down
2 changes: 0 additions & 2 deletions hazelcast/src/main/java/com/hazelcast/map/impl/MapRemoteService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@

import java.util.UUID;

import static com.hazelcast.config.NearCacheConfigAccessor.initDefaultMaxSizeForOnHeapMaps;
import static com.hazelcast.internal.config.ConfigValidator.checkMapConfig;
import static com.hazelcast.internal.config.ConfigValidator.checkNearCacheConfig;

Expand Down Expand Up @@ -56,7 +55,6 @@ public DistributedObject createDistributedObject(String name, UUID source, boole
mapServiceContext.getNodeEngine().getProperties(), nodeEngine.getLogger(MapConfig.class));

if (mapConfig.isNearCacheEnabled()) {
initDefaultMaxSizeForOnHeapMaps(mapConfig.getNearCacheConfig());
checkNearCacheConfig(name, mapConfig.getNearCacheConfig(), config.getNativeMemoryConfig(), false);
return new NearCachedMapProxyImpl(name, mapServiceContext.getService(), nodeEngine, mapConfig);
} else {
Expand Down
2 changes: 1 addition & 1 deletion hazelcast/src/test/java/com/hazelcast/config/EvictionConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public class EvictionConfigTest {
public void testEqualsAndHashCode() {
assumeDifferentHashCodes();
EqualsVerifier.forClass(EvictionConfig.class)
.allFieldsShouldBeUsedExcept("defaultSize")
.allFieldsShouldBeUsedExcept("sizeConfigured")
.suppress(Warning.NONFINAL_FIELDS)
.withPrefabValues(EvictionConfig.class,
new EvictionConfig().setSize(1000).setMaxSizePolicy(ENTRY_COUNT).setEvictionPolicy(LFU),
Expand Down
27 changes: 13 additions & 14 deletions hazelcast/src/test/java/com/hazelcast/config/NearCacheConfigAccessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.junit.runner.RunWith;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

@RunWith(HazelcastParallelClassRunner.class)
@Category({QuickTest.class, ParallelJVMTest.class})
Expand All @@ -36,27 +37,25 @@ public void testConstructor() {
}

@Test
public void testInitDefaultMaxSizeForOnHeapMaps_whenNull_thenDoNothing() {
NearCacheConfigAccessor.initDefaultMaxSizeForOnHeapMaps(null);
public void testCopyInitDefaultMaxSizeForOnHeapMaps_whenNull_thenDoNothing() {
mdumandag marked this conversation as resolved.
Show resolved Hide resolved
NearCacheConfigAccessor.copyWithInitializedDefaultMaxSizeForOnHeapMaps(null);
}

@Test
public void testNearCacheConfigEqual_BeforeAndAfterDefaultSizeIsInitialized() {
NearCacheConfig config1 = new NearCacheConfig();
NearCacheConfig config2 = new NearCacheConfig();
NearCacheConfigAccessor.initDefaultMaxSizeForOnHeapMaps(config1);
public void testCopyInitDefaultMaxSizeForOnHeapMaps_doesNotChangeOriginal_createsChangedCopy() {
NearCacheConfig nearCacheConfig = new NearCacheConfig();
NearCacheConfig copy = NearCacheConfigAccessor.copyWithInitializedDefaultMaxSizeForOnHeapMaps(nearCacheConfig);

assertEquals(config1, config2);
assertEquals(copy.getEvictionConfig().getSize(), MapConfig.DEFAULT_MAX_SIZE);
assertEquals(nearCacheConfig.getEvictionConfig().getSize(), EvictionConfig.DEFAULT_MAX_ENTRY_COUNT);
}

@Test
public void testEvictionConfigCopy_whenDefaultSizeIsInitialized() {
NearCacheConfig config1 = new NearCacheConfig();
NearCacheConfigAccessor.initDefaultMaxSizeForOnHeapMaps(config1);
public void testCopyInitDefaultMaxSizeForOnHeapMaps_doesNotCopy_whenSizeIsConfigured() {
NearCacheConfig nearCacheConfig = new NearCacheConfig();
nearCacheConfig.setEvictionConfig(new EvictionConfig().setSize(10));
NearCacheConfig copy = NearCacheConfigAccessor.copyWithInitializedDefaultMaxSizeForOnHeapMaps(nearCacheConfig);

EvictionConfig evictionConfig = config1.getEvictionConfig();
EvictionConfig evictionConfig2 = new EvictionConfig(evictionConfig);

assertEquals(evictionConfig.getSize(), evictionConfig2.getSize());
assertTrue(nearCacheConfig == copy);
}
}