You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kylin.apache.org by xx...@apache.org on 2022/12/05 10:20:54 UTC
[kylin] 02/22: KYLIN-5311 Improve performance of getSubstitutor
This is an automated email from the ASF dual-hosted git repository.
xxyu pushed a commit to branch kylin5
in repository https://gitbox.apache.org/repos/asf/kylin.git
commit c583f433242f482168db596abd1183bac23f4d72
Author: Junqing Cai <ca...@163.com>
AuthorDate: Thu Oct 13 15:08:03 2022 +0800
KYLIN-5311 Improve performance of getSubstitutor
---
.../kylin/common/ICachedExternalConfigLoader.java | 2 +-
.../org/apache/kylin/common/KylinConfigBase.java | 9 +-
.../kylin/common/KylinExternalConfigLoader.java | 9 +-
.../apache/kylin/common/PropertiesDelegate.java | 34 ++---
.../apache/kylin/common/KylinConfigBaseTest.java | 8 +
.../kylin/common/PropertiesDelegateTest.java | 116 ++++++++++++++
.../kylin/common/util/CompositeMapViewTest.java | 167 +++++++++++++++++++++
7 files changed, 316 insertions(+), 29 deletions(-)
diff --git a/src/core-common/src/main/java/org/apache/kylin/common/ICachedExternalConfigLoader.java b/src/core-common/src/main/java/org/apache/kylin/common/ICachedExternalConfigLoader.java
index 9af569f8d1..6d805eb85c 100644
--- a/src/core-common/src/main/java/org/apache/kylin/common/ICachedExternalConfigLoader.java
+++ b/src/core-common/src/main/java/org/apache/kylin/common/ICachedExternalConfigLoader.java
@@ -23,5 +23,5 @@ import com.google.common.collect.ImmutableMap;
import io.kyligence.config.core.loader.IExternalConfigLoader;
public interface ICachedExternalConfigLoader extends IExternalConfigLoader {
- ImmutableMap getPropertyEntries();
+ ImmutableMap<Object, Object> getPropertyEntries();
}
diff --git a/src/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java b/src/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java
index 8498eba0e6..d0b4fda104 100644
--- a/src/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java
+++ b/src/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java
@@ -179,8 +179,8 @@ public abstract class KylinConfigBase implements Serializable {
/**
* only reload properties
*/
- volatile PropertiesDelegate properties;
- volatile StrSubstitutor substitutor;
+ final PropertiesDelegate properties;
+ final transient StrSubstitutor substitutor;
protected KylinConfigBase(IExternalConfigLoader configLoader) {
this(new Properties(), configLoader);
@@ -190,6 +190,7 @@ public abstract class KylinConfigBase implements Serializable {
this(props, false, configLoader);
}
+ @SuppressWarnings("rawtypes")
protected KylinConfigBase(Properties props, boolean force, IExternalConfigLoader configLoader) {
if (props instanceof PropertiesDelegate) {
this.properties = (PropertiesDelegate) props;
@@ -220,12 +221,12 @@ public abstract class KylinConfigBase implements Serializable {
* @return
*/
protected Properties getProperties(Collection<String> propertyKeys) {
- final StrSubstitutor substitutor = getSubstitutor();
+ val subStitutorTmp = getSubstitutor();
Properties result = new Properties();
for (Entry<Object, Object> entry : this.properties.entrySet()) {
if (propertyKeys == null || propertyKeys.contains(entry.getKey())) {
- result.put(entry.getKey(), substitutor.replace((String) entry.getValue()));
+ result.put(entry.getKey(), subStitutorTmp.replace((String) entry.getValue()));
}
}
diff --git a/src/core-common/src/main/java/org/apache/kylin/common/KylinExternalConfigLoader.java b/src/core-common/src/main/java/org/apache/kylin/common/KylinExternalConfigLoader.java
index d4fe301060..206539dd7a 100644
--- a/src/core-common/src/main/java/org/apache/kylin/common/KylinExternalConfigLoader.java
+++ b/src/core-common/src/main/java/org/apache/kylin/common/KylinExternalConfigLoader.java
@@ -33,11 +33,11 @@ import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.Map;
+import java.util.Objects;
import java.util.Properties;
import javax.annotation.Nonnull;
-import com.google.common.collect.Maps;
import org.apache.kylin.common.exception.KylinException;
import org.apache.kylin.common.util.OrderedProperties;
import org.slf4j.Logger;
@@ -45,6 +45,7 @@ import org.slf4j.LoggerFactory;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
public class KylinExternalConfigLoader implements ICachedExternalConfigLoader {
@@ -153,7 +154,7 @@ public class KylinExternalConfigLoader implements ICachedExternalConfigLoader {
@Override
public String getConfig() {
StringWriter writer = new StringWriter();
- for (Map.Entry<String, String> entry: properties.entrySet()) {
+ for (Map.Entry<String, String> entry : properties.entrySet()) {
writer.append(entry.getKey() + "=" + entry.getValue()).append("\n");
}
return writer.toString();
@@ -161,7 +162,7 @@ public class KylinExternalConfigLoader implements ICachedExternalConfigLoader {
@Override
public String getProperty(String key) {
- return properties.get(key);
+ return Objects.toString(properties.get(key), null);
}
/**
@@ -176,7 +177,7 @@ public class KylinExternalConfigLoader implements ICachedExternalConfigLoader {
}
@Override
- public ImmutableMap getPropertyEntries() {
+ public ImmutableMap<Object, Object> getPropertyEntries() {
return propertyEntries;
}
}
diff --git a/src/core-common/src/main/java/org/apache/kylin/common/PropertiesDelegate.java b/src/core-common/src/main/java/org/apache/kylin/common/PropertiesDelegate.java
index 24f28a79cc..54dbf6d096 100644
--- a/src/core-common/src/main/java/org/apache/kylin/common/PropertiesDelegate.java
+++ b/src/core-common/src/main/java/org/apache/kylin/common/PropertiesDelegate.java
@@ -29,12 +29,13 @@ import java.util.function.BiConsumer;
import java.util.function.BiFunction;
import java.util.function.Function;
-import io.kyligence.config.core.loader.IExternalConfigLoader;
+import javax.validation.constraints.NotNull;
+
import org.apache.kylin.common.util.CompositeMapView;
import com.google.common.collect.Maps;
-import io.kyligence.config.external.loader.NacosExternalConfigLoader;
+import io.kyligence.config.core.loader.IExternalConfigLoader;
import lombok.EqualsAndHashCode;
/**
@@ -42,28 +43,28 @@ import lombok.EqualsAndHashCode;
* A few functions of hashtable are disabled.
* In the future, we should replace the java.util.Properties
*/
-@EqualsAndHashCode
+@EqualsAndHashCode(callSuper = false)
+@SuppressWarnings("sync-override")
public class PropertiesDelegate extends Properties {
@EqualsAndHashCode.Include
- private final ConcurrentMap<Object, Object> properties = Maps.newConcurrentMap();
+ private final transient ConcurrentMap<Object, Object> properties = Maps.newConcurrentMap();
@EqualsAndHashCode.Include
private final transient IExternalConfigLoader configLoader;
- private final Map delegation;
+ private final transient Map<Object, Object> delegation;
public PropertiesDelegate(Properties properties, IExternalConfigLoader configLoader) {
this.properties.putAll(properties);
this.configLoader = configLoader;
if (configLoader == null) {
this.delegation = this.properties;
- } else if (configLoader instanceof KylinExternalConfigLoader) {
- this.delegation = new CompositeMapView(((ICachedExternalConfigLoader)this.configLoader).getPropertyEntries(), this.properties);
- } else if (configLoader instanceof NacosExternalConfigLoader) {
- this.delegation = new CompositeMapView((this.configLoader).getProperties(), this.properties);
+ } else if (configLoader instanceof ICachedExternalConfigLoader) {
+ this.delegation = new CompositeMapView<>(
+ ((ICachedExternalConfigLoader) this.configLoader).getPropertyEntries(), this.properties);
} else {
- this.delegation = new CompositeMapView((this.configLoader).getProperties(), this.properties);
+ this.delegation = new CompositeMapView<>((this.configLoader).getProperties(), this.properties);
}
}
@@ -110,7 +111,6 @@ public class PropertiesDelegate extends Properties {
return delegation.size();
}
-
@Override
public boolean isEmpty() {
return delegation.isEmpty();
@@ -142,7 +142,7 @@ public class PropertiesDelegate extends Properties {
}
@Override
- public void putAll(Map<?, ?> t) {
+ public void putAll(@NotNull Map<?, ?> t) {
properties.putAll(t);
}
@@ -151,11 +151,6 @@ public class PropertiesDelegate extends Properties {
properties.clear();
}
- @Override
- public Object clone() {
- throw new UnsupportedOperationException();
- }
-
@Override
public String toString() {
throw new UnsupportedOperationException();
@@ -213,13 +208,12 @@ public class PropertiesDelegate extends Properties {
}
@Override
- public synchronized Object compute(Object key,
- BiFunction<? super Object, ? super Object, ? extends Object> remappingFunction) {
+ public Object compute(Object key, BiFunction<? super Object, ? super Object, ? extends Object> remappingFunction) {
throw new UnsupportedOperationException();
}
@Override
- public synchronized Object merge(Object key, Object value,
+ public Object merge(Object key, Object value,
BiFunction<? super Object, ? super Object, ? extends Object> remappingFunction) {
throw new UnsupportedOperationException();
}
diff --git a/src/core-common/src/test/java/org/apache/kylin/common/KylinConfigBaseTest.java b/src/core-common/src/test/java/org/apache/kylin/common/KylinConfigBaseTest.java
index 903fb8f6a0..3630c7bbd1 100644
--- a/src/core-common/src/test/java/org/apache/kylin/common/KylinConfigBaseTest.java
+++ b/src/core-common/src/test/java/org/apache/kylin/common/KylinConfigBaseTest.java
@@ -1371,6 +1371,14 @@ class KylinConfigBaseTest {
config.setProperty("kylin.server.leader-race.heart-beat-timeout-rate", "1");
Assertions.assertEquals(1.0, config.getEpochRenewTimeoutRate());
}
+
+ @Test
+ void testGetSubstitutor() {
+ KylinConfig config = KylinConfig.getInstanceFromEnv();
+ val sub1 = config.getSubstitutor();
+ val sub2 = config.getSubstitutor();
+ Assertions.assertSame(sub1, sub2);
+ }
}
class EnvironmentUpdateUtils {
diff --git a/src/core-common/src/test/java/org/apache/kylin/common/PropertiesDelegateTest.java b/src/core-common/src/test/java/org/apache/kylin/common/PropertiesDelegateTest.java
index bea2ab9321..f515659fd2 100644
--- a/src/core-common/src/test/java/org/apache/kylin/common/PropertiesDelegateTest.java
+++ b/src/core-common/src/test/java/org/apache/kylin/common/PropertiesDelegateTest.java
@@ -22,6 +22,7 @@ import java.util.ArrayList;
import java.util.Enumeration;
import java.util.HashSet;
import java.util.List;
+import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.stream.Collectors;
@@ -30,6 +31,11 @@ import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
+
+import lombok.val;
+
class PropertiesDelegateTest {
private PropertiesDelegate delegate;
@@ -83,6 +89,19 @@ class PropertiesDelegateTest {
Assertions.assertEquals("update_v2", delegate.getProperty("key_in_external"));
}
+ @Test
+ void testPutAll() {
+ val originSize = delegate.size();
+ val map1 = Maps.<String, String> newHashMap();
+ map1.put("k1", "v1");
+ map1.put("k2", "v2");
+ map1.put("k3", "v3");
+ delegate.putAll(map1);
+ Assertions.assertEquals(originSize + map1.size(), delegate.size());
+
+ Assertions.assertEquals("v1", delegate.getProperty("k1"));
+ }
+
@Test
void testSetProperty() {
delegate.setProperty("key_in_prop", "update_v0");
@@ -95,6 +114,17 @@ class PropertiesDelegateTest {
Assertions.assertEquals("update_v2", delegate.getProperty("key_in_external"));
}
+ @Test
+ void testSize() {
+ Assertions.assertEquals(3, delegate.size());
+ }
+
+ @Test
+ void testEntrySet() {
+ Set<Map.Entry<Object, Object>> entries = delegate.entrySet();
+ Assertions.assertEquals(3, entries.size());
+ }
+
@Test
void testKeys() {
List<String> keys = new ArrayList<>();
@@ -135,4 +165,90 @@ class PropertiesDelegateTest {
sets.add(properties);
Assertions.assertEquals(4, sets.size());
}
+
+ @Test
+ void testContainsKey() {
+ Assertions.assertTrue(delegate.containsKey("key_in_prop"));
+ Assertions.assertTrue(delegate.containsKey("key_override_external"));
+ Assertions.assertTrue(delegate.containsKey("key_in_external"));
+
+ Assertions.assertFalse(delegate.containsKey("not_key"));
+ }
+
+ @Test
+ void testContainsValue() {
+ Assertions.assertTrue(delegate.containsValue("v0"));
+ Assertions.assertTrue(delegate.containsValue("v11"));
+ Assertions.assertTrue(delegate.containsValue("v2"));
+
+ Assertions.assertFalse(delegate.containsValue("not_value"));
+ }
+
+ @Test
+ void testIsEmpty() {
+ Assertions.assertFalse(delegate.isEmpty());
+
+ val emptyDelegate = new PropertiesDelegate(new Properties(), null);
+ Assertions.assertTrue(emptyDelegate.isEmpty());
+ }
+
+ @Test
+ void testClear() {
+ Assertions.assertEquals(3, delegate.size());
+ delegate.clear();
+ Assertions.assertEquals(2, delegate.size());
+ }
+
+ @Test
+ void testConstruct() {
+ Properties properties = new Properties();
+ properties.put("key_in_prop", "v0");
+ {
+ val p = new PropertiesDelegate(properties, null);
+ Assertions.assertEquals(1, p.size());
+ }
+
+ {
+ TestExternalConfigLoader testExternalConfigLoader = new TestExternalConfigLoader(properties);
+ val p = new PropertiesDelegate(new Properties(), testExternalConfigLoader);
+ Assertions.assertEquals(1, p.size());
+ }
+
+ {
+ ICachedExternalConfigLoader iCachedExternalConfigLoader = new ICachedExternalConfigLoader() {
+ @Override
+ public ImmutableMap<Object, Object> getPropertyEntries() {
+ return ImmutableMap.of("key_in_prop", "v0");
+ }
+
+ @Override
+ public String getConfig() {
+ return null;
+ }
+
+ @Override
+ public String getProperty(String s) {
+ return null;
+ }
+ };
+ val p = new PropertiesDelegate(new Properties(), iCachedExternalConfigLoader);
+ Assertions.assertEquals(1, p.size());
+ }
+
+ }
+
+ @Test
+ void testNotSupport() {
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> delegate.remove("a", "b"));
+ Assertions.assertThrows(UnsupportedOperationException.class, delegate::toString);
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> delegate.forEach((k, v) -> {
+ }));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> delegate.replace("a", "b"));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> delegate.replace("a", "b", "b2"));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> delegate.computeIfAbsent("a", (k) -> ""));
+ Assertions.assertThrows(UnsupportedOperationException.class,
+ () -> delegate.computeIfPresent("a", (k, v) -> ""));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> delegate.compute("a", (k, v) -> ""));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> delegate.merge("a", "b", (k, v) -> ""));
+ }
}
diff --git a/src/core-common/src/test/java/org/apache/kylin/common/util/CompositeMapViewTest.java b/src/core-common/src/test/java/org/apache/kylin/common/util/CompositeMapViewTest.java
new file mode 100644
index 0000000000..f3623fe996
--- /dev/null
+++ b/src/core-common/src/test/java/org/apache/kylin/common/util/CompositeMapViewTest.java
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kylin.common.util;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
+
+import lombok.val;
+
+class CompositeMapViewTest {
+
+ private CompositeMapView<String, String> compositeMapView;
+
+ @BeforeEach
+ void setup() {
+ compositeMapView = prepareData();
+ }
+
+ private CompositeMapView<String, String> prepareData() {
+ val map1 = Maps.<String, String> newHashMap();
+ map1.put("k1", "v1");
+ map1.put("k2", "v2");
+ map1.put("k3", "v3");
+
+ val map2 = Maps.<String, String> newHashMap();
+ map2.put("k1", "vv1");
+ map2.put("kk2", "vv2");
+ map2.put("kk3", "v3");
+ return new CompositeMapView<>(map1, map2);
+ }
+
+ @Test
+ void testSize() {
+ Assertions.assertEquals(5, compositeMapView.size());
+ }
+
+ @Test
+ void testIsEmpty() {
+ {
+ Assertions.assertFalse(compositeMapView.isEmpty());
+ }
+
+ {
+ CompositeMapView<String, String> compositeMapView = new CompositeMapView<>(Maps.newHashMap(),
+ Maps.newHashMap());
+ Assertions.assertTrue(compositeMapView.isEmpty());
+ }
+ }
+
+ @Test
+ void testIsEmpty_Null() {
+ val emptyMap = Collections.emptyMap();
+ Assertions.assertThrows(NullPointerException.class, () -> new CompositeMapView<>(null, null));
+
+ Assertions.assertThrows(NullPointerException.class, () -> new CompositeMapView<>(emptyMap, null));
+
+ Assertions.assertThrows(NullPointerException.class, () -> new CompositeMapView<>(null, emptyMap));
+ }
+
+ @Test
+ void testContainsKey() {
+
+ Assertions.assertTrue(compositeMapView.containsKey("k1"));
+ Assertions.assertTrue(compositeMapView.containsKey("kk3"));
+ Assertions.assertTrue(compositeMapView.containsKey("k2"));
+
+ Assertions.assertFalse(compositeMapView.containsKey("noKey"));
+ }
+
+ @Test
+ void testContainsValue() {
+
+ Assertions.assertTrue(compositeMapView.containsValue("v1"));
+ Assertions.assertTrue(compositeMapView.containsValue("vv2"));
+ Assertions.assertTrue(compositeMapView.containsValue("v2"));
+
+ Assertions.assertFalse(compositeMapView.containsValue("noValue"));
+ }
+
+ @Test
+ void testGetKey() {
+
+ //both
+ Assertions.assertEquals("vv1", compositeMapView.get("k1"));
+ //left only
+ Assertions.assertEquals("v2", compositeMapView.get("k2"));
+ //right only
+ Assertions.assertEquals("v3", compositeMapView.get("kk3"));
+
+ Assertions.assertNull(compositeMapView.get("notKey"));
+ }
+
+ @Test
+ void testKeySet() {
+
+ val expectedKeySet = Sets.newHashSet(Arrays.asList("k1", "k2", "k3", "kk2", "kk3"));
+ Assertions.assertEquals(expectedKeySet, compositeMapView.keySet());
+ }
+
+ @Test
+ void testValues() {
+
+ val expectedValueSet = Sets.newHashSet(Arrays.asList("v1", "v2", "v3", "vv1", "vv2", "v3"));
+ Assertions.assertTrue(expectedValueSet.containsAll(compositeMapView.values()));
+ Assertions.assertEquals(expectedValueSet.size(), compositeMapView.values().size());
+ }
+
+ @Test
+ void testEntrySet() {
+
+ val entryList = compositeMapView.entrySet().stream().sorted(Map.Entry.comparingByKey(String::compareTo))
+ .map(e -> e.getKey() + "," + e.getValue()).collect(Collectors.toList());
+ val expectedEntryList = Arrays.asList("k1,vv1", "k2,v2", "k3,v3", "kk2,vv2", "kk3,v3");
+
+ Assertions.assertEquals(expectedEntryList, entryList);
+ }
+
+ @Test
+ void testNotSupport() {
+
+ val emptyMap = Collections.emptyMap();
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.put("a", "b"));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.putIfAbsent("a", "b"));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.remove("a"));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.remove("a", "b"));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.putAll(emptyMap));
+ Assertions.assertThrows(UnsupportedOperationException.class, compositeMapView::clear);
+ Assertions.assertThrows(UnsupportedOperationException.class, compositeMapView::toString);
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.forEach((k, v) -> {
+ }));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.replaceAll((k, v) -> ""));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.replace("a", "b"));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.replace("a", "b", "b2"));
+ Assertions.assertThrows(UnsupportedOperationException.class,
+ () -> compositeMapView.computeIfAbsent("a", (k) -> ""));
+ Assertions.assertThrows(UnsupportedOperationException.class,
+ () -> compositeMapView.computeIfPresent("a", (k, v) -> ""));
+ Assertions.assertThrows(UnsupportedOperationException.class, () -> compositeMapView.compute("a", (k, v) -> ""));
+ Assertions.assertThrows(UnsupportedOperationException.class,
+ () -> compositeMapView.merge("a", "b", (k, v) -> ""));
+
+ }
+}