Maintaining ReadyMarkers in a dedicated service #4203

sjsf · 2017-09-06T13:09:58Z

...instead of using the SCR for it.

The reason behind this is that on at least one commercial OSGi server when running with activated security manager, registering services takes quite long (up to several hundreds of milliseconds per service). When running on limited hardware resources, this significantly impacts the startup times. Therefore it does not look like such a nice idea to register hundreds of services just as "Ready Markers".

As an improvement to this, I suggest to have a single service which holds these ready markers and other components can register as listeners.

This does not mean we shouldn't use services for the "startup level"solution (see #1896). However, it simply turned out that it is not necessarily a good idea to flood the SCR with "marker" services.

Signed-off-by: Simon Kaufmann [email protected]

triller-telekom · 2017-09-06T13:50:00Z

...pse.smarthome.core/OSGI-INF/org.eclipse.smarthome.core.internal.service.ReadyServiceImpl.xml

@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="UTF-8"?>


This file was committed by accident, right? :)

oh yes, thanks!

maggu2810 · 2017-09-06T18:23:27Z

What is the reason to open the trackers async and create multiple schedulers that only use case is to schedule one Job on activation? Does the open of a tracker block the activation such long?

maggu2810 · 2017-09-06T18:27:35Z

...lipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/service/ReadyMarkerFilter.java

+    private final String type;
+
+    public ReadyMarkerFilter() {
+        this.type = null;


this(null, null);

maggu2810 · 2017-09-06T18:29:12Z

...org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/service/ReadyMarker.java

+        this.identifier = identifier;
+    }
+
+    public String getType() {


You could mark that the return type is non null.

maggu2810 · 2017-09-06T18:29:16Z

...org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/service/ReadyMarker.java

+        return type;
+    }
+
+    public String getIdentifier() {


You could mark that the return type is non null.

maggu2810 · 2017-09-06T18:30:05Z

...arthome.core/src/main/java/org/eclipse/smarthome/core/internal/service/ReadyServiceImpl.java

+@Component
+public class ReadyServiceImpl implements ReadyService {
+
+    private static final Logger logger = LoggerFactory.getLogger(ReadyServiceImpl.class);


I didn't see the need to use a logger in a static context, so could we keep the logger instance non static?

maggu2810 · 2017-09-06T18:32:31Z

...arthome.core/src/main/java/org/eclipse/smarthome/core/internal/service/ReadyServiceImpl.java

+public class ReadyServiceImpl implements ReadyService {
+
+    private static final Logger logger = LoggerFactory.getLogger(ReadyServiceImpl.class);
+    private static final ReadyMarkerFilter ANY = new ReadyMarkerFilter();


Perhaps we could make the ANY constructor (that uses null twice) private to and use a static function that returns an instance (no need for a singleton). Perhaps the meaning of ReadyMarkerFilter.any() is more intuitive then new ReadyMarkerFilter().

If i make that private, there is no public constructor left. My intention was to keep the one with arguments private in order to prevent the constructor-mess we had at other places and rather have the builder-like pattern using with...()-Methods.

So do I understand you right that using ReadyMarkerFilter.any().withType("..."); is the usage you are suggesting?

No 😉

If I understand the current situation correctly you can create three different type of ready marker filters.

filter type, created by ReadyMarkerFilter.withType(String type)

filter identifier, created by ReadyMarkerFilter.withIdentifier(String type)

filter nothing (the any one), created by new ReadyMarkerFilter()

Correct me if I am wrong.

So, I just think about making all constructors private and use for the last one a factory method, too. All three functions should return immutable objects.

filter type, created by ReadyMarkerFilter.withType(String type)

filter identifier, created by ReadyMarkerFilter.withIdentifier(String type)

filter nothing (the any one), created by ReadyMarkerFilter.any()

Ah, that's the misunderstanding... the with...(...)-Methods are not static. Therefore you can create:

filter nothing, created by new ReadyMarkerFilter()

filter type, created by new ReadyMarkerFilter().withType(String type)

filter identifier, created by new ReadyMarkerFilter().withIdentifier(String identifier)

filter for both, created by new ReadyMarkerFilter().withType(String type).withIdentifier(String identifier)

The reasoning was to have a design is that it can be evolved/extended with further parameters without creating a mess with the string arguments in constructors. I'm really not sure it these two parameters will be the only ones needed in future.

the with...(...)-Methods are not static

It seems I have forgotten how to read...

sjsf · 2017-09-07T10:20:13Z

Does the open of a tracker block the activation such long?

Yes, during tracker-opening, all matching bundles will be "added" to that tracker synchronously by the framework.
I couldn't believe it myself, but running it on this specific runtime with enabled security manager on a rather-not-so-powerful hardware took several seconds to scan all bundles.

kaikreuzer · 2017-09-11T09:55:34Z

...g.xml/src/main/java/org/eclipse/smarthome/config/xml/ConfigXmlConfigDescriptionProvider.java

@@ -41,18 +46,27 @@
    private XmlDocumentBundleTracker<List<ConfigDescription>> configDescriptionTracker;

    private ConfigI18nLocalizationService configI18nLocalizerService;
+    private ReadyService readyService;
+
+    private ScheduledExecutorService scheduler = ThreadPoolManager.getScheduledPool("xml-processing");


Could you define "xml-processing" somewhere as a constant and reuse it? Would make it easier to keep track of the created thread pools.

kaikreuzer · 2017-09-11T09:57:25Z

...config.xml/src/main/java/org/eclipse/smarthome/config/xml/osgi/XmlDocumentBundleTracker.java

-        }
-        ScheduledFuture<?> future = scheduler.schedule(() -> {
-            processBundle(bundle);
+        ScheduledFuture<?> future = scheduler.schedule(new Runnable() {


Could you use "submit"?

kaikreuzer · 2017-09-11T10:02:32Z

...ml/src/main/java/org/eclipse/smarthome/core/binding/xml/internal/XmlBindingInfoProvider.java

-                bindingInfoReader, this, READY_MARKER);
-        bindingInfoTracker.open();
+                bindingInfoReader, this, READY_MARKER, readyService);
+        trackerJob = scheduler.schedule(() -> {


kaikreuzer · 2017-09-11T10:03:06Z

...ng.xml/src/main/java/org/eclipse/smarthome/core/thing/xml/internal/XmlThingTypeProvider.java

-        thingTypeTracker.open();
+                READY_MARKER, readyService);
+
+        trackerJob = scheduler.schedule(() -> {


kaikreuzer · 2017-09-11T10:06:14Z

...rg.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/service/ReadyService.java

+ * @author Simon Kaufmann - initial contribution and API.
+ *
+ */
+@NonNullByDefault


Did we already agree on using that annotation?

No idea, I lost the overview...

Check this to get an overview again :-)

amitjoy · 2017-09-11T10:08:04Z

...arthome.core/src/main/java/org/eclipse/smarthome/core/internal/service/ReadyServiceImpl.java

+    }
+
+    private void notifyTrackers(ReadyMarker readyMarker, Consumer<ReadyTracker> action) {
+        trackers.entrySet().stream().filter(entry -> {


Not a change request but a suggestion:

trackers.entrySet().stream().filter(entry -> entry.getValue().apply(readyMarker)).map(entry -> entry.getKey()) .forEach(action);

or

trackers.entrySet().stream().filter(entry -> entry.getValue().apply(readyMarker)).map(Entry::getKey) .forEach(action);

I've been using the block-notation on purpose, to force the auto-formatter to insert line-breaks in places where the human brain can follow. I couldn't figure out how to make JDT do do something similar automatically. Do you by any chance have an idea?

Ah now I understand. What I usually do to achieve this is that I manually make changes to the stream notations and turn off autoformatter for certain section.

// @formatter:off trackers.entrySet().stream() .filter(entry -> entry.getValue().apply(readyMarker)) .map(Entry::getKey) .forEach(action); // @formatter:on

...instead of using the SCR for it. Signed-off-by: Simon Kaufmann <[email protected]>

triller-telekom reviewed Sep 6, 2017

View reviewed changes

sjsf force-pushed the readyService branch from 28dc5cf to c492ba7 Compare September 6, 2017 16:58

maggu2810 reviewed Sep 6, 2017

View reviewed changes

triller-telekom mentioned this pull request Sep 7, 2017

ChannelItemProvider depends on bundle start order #1550

Closed

sjsf force-pushed the readyService branch 2 times, most recently from 8ea28ba to e6233e7 Compare September 8, 2017 08:42

kaikreuzer reviewed Sep 11, 2017

View reviewed changes

amitjoy reviewed Sep 11, 2017

View reviewed changes

Maintaining ReadyMarkers in a dedicated service

2245c1d

...instead of using the SCR for it. Signed-off-by: Simon Kaufmann <[email protected]>

sjsf force-pushed the readyService branch from e6233e7 to 2245c1d Compare September 11, 2017 11:30

kaikreuzer approved these changes Sep 14, 2017

View reviewed changes

kaikreuzer merged commit 72523bc into eclipse-archived:master Sep 14, 2017

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintaining ReadyMarkers in a dedicated service #4203

Maintaining ReadyMarkers in a dedicated service #4203

sjsf commented Sep 6, 2017

triller-telekom Sep 6, 2017

sjsf Sep 6, 2017

maggu2810 commented Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

sjsf Sep 7, 2017

maggu2810 Sep 7, 2017 •

edited

Loading

sjsf Sep 7, 2017

maggu2810 Sep 7, 2017

sjsf commented Sep 7, 2017

kaikreuzer Sep 11, 2017

kaikreuzer Sep 11, 2017

kaikreuzer Sep 11, 2017

kaikreuzer Sep 11, 2017

kaikreuzer Sep 11, 2017

sjsf Sep 11, 2017

kaikreuzer Sep 11, 2017

amitjoy Sep 11, 2017

amitjoy Sep 11, 2017

sjsf Sep 11, 2017 •

edited

Loading

amitjoy Sep 11, 2017 •

edited

Loading

Maintaining ReadyMarkers in a dedicated service #4203

Maintaining ReadyMarkers in a dedicated service #4203

Conversation

sjsf commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggu2810 commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggu2810 Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjsf commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjsf Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

amitjoy Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

maggu2810 Sep 7, 2017 •

edited

Loading

sjsf Sep 11, 2017 •

edited

Loading

amitjoy Sep 11, 2017 •

edited

Loading