[jira] [Commented] (SLING-4417) HC Annotation should allow to configure "immediate" SCR property

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15170538#comment-15170538
 ] 

ASF GitHub Bot commented on SLING-4417:
---

GitHub user ghenzler opened a pull request:

https://github.com/apache/sling/pull/127

SLING-4417 now the the default for immediate is false, it needs to be 
explicitly provided here to ensure state is kept



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/ghenzler/sling trunk

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/sling/pull/127.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #127


commit 80ff77358d10583c851861423040b2ce5b31f1a3
Author: georg.henzler 
Date:   2016-02-27T10:30:37Z

SLING-4417 now the the default for immediate is false, it needs to be
explicitly provided here to ensure state is kept




> HC Annotation should allow to configure "immediate" SCR property
> 
>
> Key: SLING-4417
> URL: https://issues.apache.org/jira/browse/SLING-4417
> Project: Sling
>  Issue Type: New Feature
>  Components: Health Check
>Reporter: Georg Henzler
>Assignee: Konrad Windszus
> Fix For: Health Check samples 1.0.8, Health Check Annotations 
> 1.0.4
>
> Attachments: 
> SLING-4417-HC-Annotation-with-immediate-setting-default-false.patch.txt, 
> SLING-4417-HC-Annotation-with-immediate-setting.patch
>
>
> When using @SlingHealthCheck at the moment, the "immediate" property is left 
> to "false" in the SCR descriptor which causes the component object to be 
> created on every call of the health check (making it impossible to keep some 
> state in a private member variable if desired). 
> Let's make the immediate property configurable (the same way it would be 
> provided in the @Component annotation) and make immediate="true" the default 
> (this is a slight change in the behaviour that will not break existing code) 
> for the following reasons:
> - It's more intuitive to think of a HC as singleton (and hence be able to 
> keep some instance variables)
> - It's a tiny little bit better from a performance perspective (the instance 
> does not have to be created on each execution)
> The attached patch includes the (fairly simple) change to 
> annotation(-processor) and the change for two sample components that were 
> using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Carsten Ziegeler (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14533986#comment-14533986
 ] 

Carsten Ziegeler commented on SLING-4417:
-

Right, it doesn't cause problems if immediate is the default - however, there 
is a difference between a lazy and an immediate component, especially when it 
comes to startup. Therefore immediate is actually not the behaviour you want. 
It's not best practice and I simply don't want to make a non best practice the 
default.
Let's simply add the boolean flag and if you need immediate, set it.

 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Georg Henzler (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14533998#comment-14533998
 ] 

Georg Henzler commented on SLING-4417:
--

Ok fair enough - it's good enough for me to be able to set the immediate 
property if needed, let's go ahead with immediate=false as default then. 

 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Georg Henzler (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14530940#comment-14530940
 ] 

Georg Henzler commented on SLING-4417:
--

[~cziegeler] For compatibility reasons I can't really think of a scenario that 
would cause problems... who would have written checks that rely on instance 
variables being reset after each run? This is highly unlikely. Also it is only 
a build-time aspect as long as people don't actively upgrade to the latest 
version of the annotations they will have the same behaviour.

 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Carsten Ziegeler (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14528530#comment-14528530
 ] 

Carsten Ziegeler commented on SLING-4417:
-

Ok, I see your point - for servlets there is a well defined contract for the 
lifecycle of a servlet. The http implementation holds the service in memory 
(and calls init/destroy).
You don't need to make a servlet immediate for this to work - using immediate 
actually means that the servlet is instatiated as soon as possible, even if 
there is no http service.
It's true that ultimately you get the same result and the only way with DS is 
to use immediate (which is really unfortunate).
For compatiblity I would still go with the default being false. We can some 
more javadocs to make this clear.

 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Georg Henzler (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14528496#comment-14528496
 ] 

Georg Henzler commented on SLING-4417:
--

@[~cziegeler] I am aware that state is lost when bundles restart... there is 
quite a few cases in health checks when the recent history can be nice to have 
(and if there is no negative history after a bundle restart, it's fine to be 
green :)).

I personally would not have a concern making the immediate the default and I 
think it will be a lot better understandable for developers writing checks (as 
then the behaviour is in line with servlets, and developers are also aware that 
after a bundle restart servlet state is lost). Also from a backward 
compatibility perspective there should not be a problem (who would have written 
checks that rely on instance variables being reset after each run?).  I agree 
with [~cziegeler] for it's not the task of the HC core to provide a way for 
implementations to keep state.



 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Bertrand Delacretaz (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14528095#comment-14528095
 ] 

Bertrand Delacretaz commented on SLING-4417:


This is useful but I don't think it's a good idea to make immediate the 
default, as the previous @SlingHealthCheck default was (IIUC) immediate=false.

If I'm correct, are you ok with changing the default to immediate=false for 
backwards compatibility?

 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Konrad Windszus (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14528107#comment-14528107
 ] 

Konrad Windszus commented on SLING-4417:


According to the OSGi spec the lifecycle of a delayed component is not that 
much different from an immediate component. The only difference should be the 
point in time it is activated (but not the deactivation of the component). The 
difference is described in OSGi Compendium 4.3 $112.5.4:
{quote}
The activation of a component configuration must be delayed until its service 
is requested. When the service is requested, if the service has the 
servicefactory attribute set to true, SCR must create and activate a unique 
component configuration for each bundle requesting the service. Otherwise, SCR 
must activate a single component configuration which is used by all bundles 
requesting the service. A component instance can determine the bundle it was 
activated for by calling the getUsingBundle() method on the Component Context.
{quote}

{quote}
If the service registered by a component configuration becomes unused because 
there are no more bundles using it, then SCR should deactivate that component 
configuration. This allows SCR implementations to eagerly reclaim activated 
component configurations.
{quote}

The diagrams in that paragraph depict that nicely.

The deferred activation should be no problem but the fact that the references 
to the HealthChecks are no longer used once the healthcheck was executed is a 
problem. This leads to the instance being lost. Why do you not keep those 
service references e.g. in the executor. That would prevent the OSGi container 
from deactivating those components.

 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Carsten Ziegeler (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14528109#comment-14528109
 ] 

Carsten Ziegeler commented on SLING-4417:
-

I think immediate is more an anti pattern, which should be used with care and 
thinking. It prevents lazy instantiation and might waste resources for no 
reason. So making true the default feels wrong and is also backwards compatible 
as Bertrand notes.
A lot of HCs are fine with getting created/removed each time.
Pease note that keeping state within the component works only for as long as 
your bundle is running. If someone restarts your bundle for whatever reason, 
the state is gone. It might be fine for your use case, but still the assumption 
that as long as the server is running you have a single instance of your HC 
running is not right.

 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (SLING-4417) HC Annotation should allow to configure immediate SCR property

[Carsten Ziegeler (JIRA)]

[ 
https://issues.apache.org/jira/browse/SLING-4417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14528114#comment-14528114
 ] 

Carsten Ziegeler commented on SLING-4417:
-

This would be an option - kind of an optimization - I think it's not the task 
of the HC core to provide a way for implementations to keep state.
For example if a HealthCheck is implemented without DS, you usually have a 
singleton registered in the service registry. Therefore it's really an 
implementation concern - if the component needs to keep state between runs it's 
its task to keep the state in one way or the other.

 HC Annotation should allow to configure immediate SCR property
 

 Key: SLING-4417
 URL: https://issues.apache.org/jira/browse/SLING-4417
 Project: Sling
  Issue Type: New Feature
  Components: Health Check
Reporter: Georg Henzler
 Attachments: SLING-4417-HC-Annotation-with-immediate-setting.patch


 When using @SlingHealthCheck at the moment, the immediate property is left 
 to false in the SCR descriptor which causes the component object to be 
 created on every call of the health check (making it impossible to keep some 
 state in a private member variable if desired). 
 Let's make the immediate property configurable (the same way it would be 
 provided in the @Component annotation) and make immediate=true the default 
 (this is a slight change in the behaviour that will not break existing code) 
 for the following reasons:
 - It's more intuitive to think of a HC as singleton (and hence be able to 
 keep some instance variables)
 - It's a tiny little bit better from a performance perspective (the instance 
 does not have to be created on each execution)
 The attached patch includes the (fairly simple) change to 
 annotation(-processor) and the change for two sample components that were 
 using @Component because of this issue.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)