glimmerveen commented on code in PR #486:
URL: https://github.com/apache/felix-dev/pull/486#discussion_r2995980785
##########
scr/src/main/java/org/apache/felix/scr/impl/ComponentRegistry.java:
##########
@@ -132,9 +133,12 @@ public class ComponentRegistry
private final ScheduledExecutorService m_componentActor;
+ private final UpdateChangeCountProperty m_updateChangeCountPropertyTask;
+
public ComponentRegistry(final ScrConfiguration scrConfiguration, final
ScrLogger logger, final ScheduledExecutorService componentActor )
{
m_configuration = scrConfiguration;
+ m_updateChangeCountPropertyTask = new
UpdateChangeCountProperty(m_configuration.serviceChangecountTimeout());
Review Comment:
I tried to convey it in earlier comments, but I will try to elaborate on it
here. I have some monitoring logic that uses the ServiceComponentRuntime
information to assess the state of component configurations, and I rely on the
changecount update to know that there has been an update and that I need to
re-query the ServiceComponentRuntime service to know what the current state is.
In the _as is_ situation (<= 2.2.14), the timeout not only delayed the
changecount update, but in case of consecutive updates could effectively delay
the changecount update way beyond the configured timeout (as a delayed update
task would discard its changecount update if it saw an other update was already
scheduled). Eventually you'll reach the point where the task is actually
executed and the changecount is update, and observing code could query the
ServiceComponentRuntime, but in some cases it meant that the state I was
looking for was already passed. The _mitigation_ up to now was configuring a
very low timeout, as it would effectively ensure that in a series of updates an
early there would always be a brief period of no updates and thus the timeout
clearing and allowed to update the changecount.
With the changes proposed in this PR the behaviour of the update to the
changecount changes, as the delay due to its periodic scheduled nature combined
with the removal of "skipping an update logic" now is guaranteed to happen at
the timeout duration at the latest. The fact that the update happens at the
duration of the timeout at the latest mitigates the "continuous postponing"
that happened before. My concern however is if the proposed "maximum minimum"
window of 100ms is sufficient for my use case.
In the _as is_ situation you could even go as far as configuring a timeout
of 0. You can of course debate whether using that as timeout is sensible value
to use, but you could, and it would behave (close to) what you would expect:
the changecount update happening immediately (and most changecount update would
be observable, but not all as the "skipping update logic" could still cancel an
update).
I think the proposed approach is an improvement as it is more predictable. I
would prefer though that the minimum value enforced by SCR itself to be as low
as the logic allows (1ms in this case). Not to promote the use of such a low
value, but to impose the least amount of restrictions, which leaves more
choice/control for the user/integrator.
An alternative could be to make the timeout optional, and allow the
user/integrator to opt-in for all changecount updates. If that is the preferred
route, I am more than willing to open PR to add 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
