[jira] [Commented] (KYLIN-5263) replace log4j with logback

2022-09-15 Thread zhaoliu (Jira) 


[ 
https://issues.apache.org/jira/browse/KYLIN-5263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17605603#comment-17605603
 ] 

zhaoliu commented on KYLIN-5263:


等改完后测下

> replace log4j with logback
> --
>
> Key: KYLIN-5263
> URL: https://issues.apache.org/jira/browse/KYLIN-5263
> Project: Kylin
>  Issue Type: Improvement
>  Components: Query Engine
>Affects Versions: v4.0.1
>Reporter: zhaoliu
>Priority: Minor
> Attachments: screenshot-1.png
>
>
> 在查询并发高的时候，发现线程会阻塞在日志打印处，可以使用更高效的logback替换当前的log4j.
>  !screenshot-1.png! 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[jira] [Commented] (KYLIN-5263) replace log4j with logback

2022-09-15 Thread liyang (Jira) 


[ 
https://issues.apache.org/jira/browse/KYLIN-5263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17605544#comment-17605544
 ] 

liyang commented on KYLIN-5263:
---

有没有办法评估一下换成 logback 能提升多少？

any estimate of the improvement after change to logback?

> replace log4j with logback
> --
>
> Key: KYLIN-5263
> URL: https://issues.apache.org/jira/browse/KYLIN-5263
> Project: Kylin
>  Issue Type: Improvement
>  Components: Query Engine
>Affects Versions: v4.0.1
>Reporter: zhaoliu
>Priority: Minor
> Attachments: screenshot-1.png
>
>
> 在查询并发高的时候，发现线程会阻塞在日志打印处，可以使用更高效的logback替换当前的log4j.
>  !screenshot-1.png! 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

[GitHub] [kylin] zznlime commented on a diff in pull request #1969: Kylin 5256 Add a cache for the system property get by the optional config in KylinConfigBase

2022-09-15 Thread GitBox 


zznlime commented on code in PR #1969:
URL: https://github.com/apache/kylin/pull/1969#discussion_r971936827


##
src/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java:
##
@@ -111,6 +110,33 @@ public abstract class KylinConfigBase implements 
Serializable {
 
 protected static final Map STATIC_SYSTEM_ENV = new 
ConcurrentHashMap<>(System.getenv());
 
+// It's a workaround to avoid lock in bottom hash table
+// It can be removed after updating JDK to 11
+protected static final ConcurrentHashMap 
STATIC_SYSTEM_PROPERTY = new ConcurrentHashMap<>(
+System.getProperties());
+
+protected static String getSystemProperty(String key) {
+Object oval = STATIC_SYSTEM_PROPERTY.get(key);
+return (oval instanceof String) ? (String) oval : null;
+}
+
+protected static String getSystemProperty(String key, String defaultValue) 
{
+String val = getSystemProperty(key);
+return (val == null) ? defaultValue : val;
+}
+
+// Mainly invoked in tests
+public static String setSystemProperty(String key, String value) {
+System.setProperty(key, value);
+return (String) STATIC_SYSTEM_PROPERTY.put(key, value);

Review Comment:
   this commit has been revert



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@kylin.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kylin] zznlime commented on a diff in pull request #1969: Kylin 5256 Add a cache for the system property get by the optional config in KylinConfigBase

2022-09-15 Thread GitBox 


zznlime commented on code in PR #1969:
URL: https://github.com/apache/kylin/pull/1969#discussion_r971936057


##
src/core-common/src/main/java/org/apache/kylin/common/PropertiesDelegate.java:
##
@@ -18,36 +18,38 @@
 
 package org.apache.kylin.common;
 
-import com.google.common.collect.Maps;
-import io.kyligence.config.core.loader.IExternalConfigLoader;
-import io.kyligence.config.external.loader.NacosExternalConfigLoader;
-import lombok.EqualsAndHashCode;
-
 import java.util.Collections;
 import java.util.Enumeration;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
 import java.util.concurrent.ConcurrentMap;
 
+import com.google.common.collect.Maps;
+
+import io.kyligence.config.external.loader.NacosExternalConfigLoader;
+import lombok.EqualsAndHashCode;
+
 @EqualsAndHashCode
 public class PropertiesDelegate extends Properties {
 
 @EqualsAndHashCode.Include
 private final ConcurrentMap properties = 
Maps.newConcurrentMap();
 
 @EqualsAndHashCode.Include
-private final transient IExternalConfigLoader configLoader;
+private final transient ICachedExternalConfigLoader configLoader;
 
-public PropertiesDelegate(Properties properties, IExternalConfigLoader 
configLoader) {
-if(configLoader != null) 
this.properties.putAll(configLoader.getProperties());
+public PropertiesDelegate(Properties properties, 
ICachedExternalConfigLoader configLoader) {
+if (configLoader != null)
+this.properties.putAll(configLoader.getPropertyEntries());
 this.properties.putAll(properties);
 this.configLoader = configLoader;
 }
 
 public void reloadProperties(Properties properties) {
 this.properties.clear();
-if(configLoader != null) 
this.properties.putAll(configLoader.getProperties());
+if (configLoader != null)

Review Comment:
   This commit has been revert. In the future, it's better to change 
NacosExternalConfigLoader.dataIdContentMap  into public, which can help get 
config from this map directly instead of building a new Properties for every 
call of IExternalConfigLoader.getProperties().



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@kylin.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kylin] zznlime commented on a diff in pull request #1969: Kylin 5256 Add a cache for the system property get by the optional config in KylinConfigBase

2022-09-15 Thread GitBox 


zznlime commented on code in PR #1969:
URL: https://github.com/apache/kylin/pull/1969#discussion_r971926576


##
src/core-common/src/main/java/org/apache/kylin/common/util/Unsafe.java:
##
@@ -29,6 +29,7 @@
 import org.apache.commons.lang.StringUtils;
 
 import lombok.extern.slf4j.Slf4j;
+import org.apache.kylin.common.KylinConfigBase;

Review Comment:
   Have removed KylinConfigBase import from Unsafe.java



##
src/core-common/src/main/java/org/apache/kylin/common/util/Unsafe.java:
##
@@ -113,7 +114,7 @@ public static void restoreAllSystemProp(Map 
systemProp) {
 
 /** Set system property */
 public static String setProperty(String property, String value) {
-return System.setProperty(property, value);
+return KylinConfigBase.setSystemProperty(property, value);

Review Comment:
   Fixed



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@kylin.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kylin] zznlime commented on a diff in pull request #1969: Kylin 5256 Add a cache for the system property get by the optional config in KylinConfigBase

2022-09-15 Thread GitBox 


zznlime commented on code in PR #1969:
URL: https://github.com/apache/kylin/pull/1969#discussion_r971925963


##
src/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java:
##
@@ -138,15 +164,15 @@ public static String getKylinHome() {
 public static String getKylinHomeWithoutWarn() {
 String kylinHome = System.getenv("KYLIN_HOME");
 if (StringUtils.isEmpty(kylinHome)) {
-kylinHome = System.getProperty("KYLIN_HOME");
+kylinHome = getSystemProperty("KYLIN_HOME");
 }
 return kylinHome;
 }
 
 public static String getKylinConfHome() {
 String confHome = System.getenv("KYLIN_CONF");
 if (StringUtils.isEmpty(confHome)) {
-confHome = System.getProperty("KYLIN_CONF");
+confHome = getSystemProperty("KYLIN_HOME");

Review Comment:
   Fixed. Thanks for checking.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@kylin.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [kylin] zznlime commented on a diff in pull request #1969: Kylin 5256 Add a cache for the system property get by the optional config in KylinConfigBase

2022-09-15 Thread GitBox 


zznlime commented on code in PR #1969:
URL: https://github.com/apache/kylin/pull/1969#discussion_r971924698


##
src/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java:
##
@@ -111,6 +110,33 @@ public abstract class KylinConfigBase implements 
Serializable {
 
 protected static final Map STATIC_SYSTEM_ENV = new 
ConcurrentHashMap<>(System.getenv());
 
+// It's a workaround to avoid lock in bottom hash table
+// It can be removed after updating JDK to 11
+protected static final ConcurrentHashMap 
STATIC_SYSTEM_PROPERTY = new ConcurrentHashMap<>(
+System.getProperties());
+
+protected static String getSystemProperty(String key) {
+Object oval = STATIC_SYSTEM_PROPERTY.get(key);

Review Comment:
   Have added checkKey() for getSystemProperty.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@kylin.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org