You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@syncope.apache.org by GitBox <gi...@apache.org> on 2021/05/20 11:45:32 UTC
[GitHub] [syncope] DmitriyBrashevets opened a new pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
DmitriyBrashevets opened a new pull request #264:
URL: https://github.com/apache/syncope/pull/264
…conf
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] DmitriyBrashevets commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
DmitriyBrashevets commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636180820
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -48,7 +49,7 @@
private static final Map<String, Class<?>> CLASS_CACHE = Collections.synchronizedMap(new HashMap<>());
- public static Optional<Reportlet> buildReportlet(final Implementation impl)
+ public static Optional<Reportlet> buildReportlet(final String domain, final Implementation impl)
Review comment:
I thought that it's becoming more understandable (visible) if public methods have the additional parameter declared explicitly. But can be removed...
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] DmitriyBrashevets commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
DmitriyBrashevets commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636192432
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -48,7 +49,7 @@
private static final Map<String, Class<?>> CLASS_CACHE = Collections.synchronizedMap(new HashMap<>());
- public static Optional<Reportlet> buildReportlet(final Implementation impl)
+ public static Optional<Reportlet> buildReportlet(final String domain, final Implementation impl)
Review comment:
removed addtional param
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -72,7 +73,7 @@
}
}
- public static Optional<AccountRule> buildAccountRule(final Implementation impl)
+ public static Optional<AccountRule> buildAccountRule(final String domain, final Implementation impl)
Review comment:
removed addtional param
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -96,7 +97,7 @@
}
}
- public static Optional<PasswordRule> buildPasswordRule(final Implementation impl)
+ public static Optional<PasswordRule> buildPasswordRule(final String domain, final Implementation impl)
Review comment:
removed addtional param
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -120,7 +121,7 @@
}
}
- public static Optional<PullCorrelationRule> buildPullCorrelationRule(final Implementation impl)
+ public static Optional<PullCorrelationRule> buildPullCorrelationRule(final String domain, final Implementation impl)
Review comment:
removed addtional param
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -145,7 +146,7 @@
}
}
- public static Optional<PushCorrelationRule> buildPushCorrelationRule(final Implementation impl)
+ public static Optional<PushCorrelationRule> buildPushCorrelationRule(final String domain, final Implementation impl)
Review comment:
removed addtional param
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636028258
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -96,7 +97,7 @@
}
}
- public static Optional<PasswordRule> buildPasswordRule(final Implementation impl)
+ public static Optional<PasswordRule> buildPasswordRule(final String domain, final Implementation impl)
Review comment:
same as above
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -48,7 +49,7 @@
private static final Map<String, Class<?>> CLASS_CACHE = Collections.synchronizedMap(new HashMap<>());
- public static Optional<Reportlet> buildReportlet(final Implementation impl)
+ public static Optional<Reportlet> buildReportlet(final String domain, final Implementation impl)
Review comment:
Why adding a `domain` argument and not letting the method body getting it via `AuthContextUtils.getDomain()` ?
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -120,7 +121,7 @@
}
}
- public static Optional<PullCorrelationRule> buildPullCorrelationRule(final Implementation impl)
+ public static Optional<PullCorrelationRule> buildPullCorrelationRule(final String domain, final Implementation impl)
Review comment:
same as above
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
The same exact `if` with statement is also performed in the `synchronized` block: why?
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -72,7 +73,7 @@
}
}
- public static Optional<AccountRule> buildAccountRule(final Implementation impl)
+ public static Optional<AccountRule> buildAccountRule(final String domain, final Implementation impl)
Review comment:
same as above
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -145,7 +146,7 @@
}
}
- public static Optional<PushCorrelationRule> buildPushCorrelationRule(final Implementation impl)
+ public static Optional<PushCorrelationRule> buildPushCorrelationRule(final String domain, final Implementation impl)
Review comment:
same as above
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] DmitriyBrashevets commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
DmitriyBrashevets commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636167234
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
This is a double-check lock pattern. Thread takes monitor and executes the logic regarding initialization. Another thread waits until the the monitor is realeased. As soon as first thread releases monitor -> second thread proceeds its work and reaches the additional `if` clause. As the bean already exists (created by first thread) the second thread is not creating a bean again and not registers it as a singleton one more time.
Do you see any disadvantages it this?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] DmitriyBrashevets commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
DmitriyBrashevets commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636195578
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
Ah... I see... Maybe java optimization will remove this logic from bytecode when the method will become hot.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso merged pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
ilgrosso merged pull request #264:
URL: https://github.com/apache/syncope/pull/264
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636617535
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
I am not completely convinced, but let that be :-)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] DmitriyBrashevets commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
DmitriyBrashevets commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636167234
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
This is a double-check lock pattern. Thread takes monitor and executes the logic regarding initialization. Another thread waits until the the monitor is realeased. As soon as first thread releases monitor -> second threads proceeds its work and reaches the additional `if` clause. As the bean already exists (created by first thread) the second thread is not creating a bean again and not registers it as a singleton.
Do you see any disadvantages it this?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] DmitriyBrashevets commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
DmitriyBrashevets commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636195578
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
Ah... I see... Maybe java optimization will remove this logic from bytecode when the method will become hot.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] DmitriyBrashevets commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
DmitriyBrashevets commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636191747
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
IMHO: Adding of the additional if clause gives a small improvement in performance (because the acquiring of monitor is not requested).
Am I wrong?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a change in pull request #264: [SYNCOPE-1635] Use singleton bean for each domain for each rule with conf
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on a change in pull request #264:
URL: https://github.com/apache/syncope/pull/264#discussion_r636173742
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
I see, there is some literature abut double-check lock pattern, and some is not very pleasant with it, especially for Java implementations.
What would be the disadvantage of simply removing the first `if` and leaving the whole logic in the `synchronized` block?
##########
File path: core/spring/src/main/java/org/apache/syncope/core/spring/ImplementationManager.java
##########
@@ -216,28 +217,27 @@
}
@SuppressWarnings("unchecked")
- private static <T> T buildJavaWithConf(final Class<T> clazz) {
- T bean = null;
-
+ private static <T> T buildJavaWithConf(final String domain, final Class<T> clazz) {
if (clazz != null) {
- if (ApplicationContextProvider.getBeanFactory().containsSingleton(clazz.getName())) {
- bean = (T) ApplicationContextProvider.getBeanFactory().getSingleton(clazz.getName());
- } else {
- try {
- bean = (T) ApplicationContextProvider.getBeanFactory().
- createBean(clazz, AbstractBeanDefinition.AUTOWIRE_BY_TYPE, false);
- ApplicationContextProvider.getBeanFactory().registerSingleton(clazz.getName(), bean);
- } catch (IllegalStateException e) {
- LOG.debug("While attempting to register {}", clazz.getName(), e);
+ String domainableBeanNameWithConf = domain + clazz.getName();
+ DefaultListableBeanFactory beanFactory = ApplicationContextProvider.getBeanFactory();
+
+ if (beanFactory.containsSingleton(domainableBeanNameWithConf)) {
Review comment:
I see, there is wide literature abut double-check lock pattern, and some is not very pleasant with it, especially for Java implementations.
What would be the disadvantage of simply removing the first `if` and leaving the whole logic in the `synchronized` block?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org