OSGi-ified waiting for xml processing #3423
Conversation
I am very interested in the work you have done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume I need multiple runs to go through this PR as it needs some time -- it is not small 😉
Next time we should perhaps split the logical changes into different PRs.
public abstract class AbstractXmlBasedProvider<T_ID, T_OBJECT> { | ||
|
||
private class LocalizedKey { | ||
public T_ID uid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a generic and also an URI could be an ID (refer to your JavaDoc) we should name the variable id
instead of uid
.
|
||
private class LocalizedKey { | ||
public T_ID uid; | ||
public String locale; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A variable that is set only once by the constructor should be marked final (IMHO)
public T_ID uid; | ||
public String locale; | ||
|
||
public LocalizedKey(T_ID uid, String locale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uid
=> id
if (!getOuterType().equals(other.getOuterType())) { | ||
return false; | ||
} | ||
if (locale == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using Objects.equal(local, other.locale)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of https://bugs.eclipse.org/bugs/show_bug.cgi?id=424214 😉
} else if (!locale.equals(other.locale)) { | ||
return false; | ||
} | ||
if (uid == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, Objects.equal
should be simpler instead of all the null checks...
} | ||
@SuppressWarnings("unchecked") | ||
LocalizedKey other = (LocalizedKey) obj; | ||
if (!getOuterType().equals(other.getOuterType())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is private and as fair as I could see an instance is only created internally. Do you really need to check if the key itself is owned by different objects?
*/ | ||
public abstract class AbstractXmlBasedProvider<T_ID, T_OBJECT> { | ||
|
||
private class LocalizedKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WRT to the getOuterType
comment on equals
we could perhaps make this a static class.
return true; | ||
} | ||
|
||
private AbstractXmlBasedProvider<T_ID, T_OBJECT> getOuterType() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WRT to the getOuterType
comment on equals
we could perhaps drop this method (and this will allow us to make the class static).
/** | ||
* Create a translated/localized copy of the given object. | ||
* | ||
* @param bundle the bundle where the config description with its translations originates from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is config description
correct?
* @param object the object to be added | ||
*/ | ||
public final synchronized void add(Bundle bundle, T_OBJECT object) { | ||
if (object == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better (with respect to removal of redundant code) to call addAll(bundle, Collections.singletonList(object))
if (objectList == null) { | ||
return null; | ||
} | ||
for (Entry<Bundle, List<T_OBJECT>> objects : objectList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using ... : bundleObjectsMap.entrySet()) {
?
entrySet shouldn't return null but an empty entry set.
* @return a collection containing all available objects. Never <code>null</code> | ||
*/ | ||
protected final synchronized Collection<T_OBJECT> getAll(Locale locale) { | ||
List<T_OBJECT> ret = new ArrayList<>(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to use an LinkedList
to avoid a lot of reallocations?
if (bundle == null) { | ||
return; | ||
} | ||
List<T_OBJECT> thingTypes = bundleObjectMap.remove(bundle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why thingTypes
? Shouldn't we use objects
?
} | ||
} | ||
|
||
private void removeCachedEntries(T_OBJECT thingType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why thingTypes
? Shouldn't we use objects
?
* | ||
* @param <T> | ||
* the result type of the conversion | ||
*/ | ||
@SuppressWarnings({ "unchecked", "rawtypes" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really suppress all that warnings for the whole class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this changes we don't need to supress warnings (perhaps ?
could be changed to some other type, I just used a q'n'd fix).
diff --git a/bundles/config/org.eclipse.smarthome.config.xml/src/main/java/org/eclipse/smarthome/config/xml/osgi/XmlDocumentBundleTracker.java b/bundles/config/org.eclipse.smarthome.config.xml/src/main/java/org/eclipse/smarthome/config/xml/osgi/XmlDocumentBundleTracker.java
index c9a97abb6..cc04466d3 100644
--- a/bundles/config/org.eclipse.smarthome.config.xml/src/main/java/org/eclipse/smarthome/config/xml/osgi/XmlDocumentBundleTracker.java
+++ b/bundles/config/org.eclipse.smarthome.config.xml/src/main/java/org/eclipse/smarthome/config/xml/osgi/XmlDocumentBundleTracker.java
@@ -53,7 +53,6 @@ import org.slf4j.LoggerFactory;
* @param <T>
* the result type of the conversion
*/
-@SuppressWarnings({ "unchecked", "rawtypes" })
public class XmlDocumentBundleTracker<T> extends BundleTracker<Bundle> {
private final Logger logger = LoggerFactory.getLogger(XmlDocumentBundleTracker.class);
@@ -64,10 +63,10 @@ public class XmlDocumentBundleTracker<T> extends BundleTracker<Bundle> {
private final Map<Bundle, XmlDocumentProvider<T>> bundleDocumentProviderMap = new ConcurrentHashMap<>();
private final Map<Bundle, ScheduledFuture<?>> queue = new ConcurrentHashMap<>();
private final Set<Bundle> finishedBundles = new CopyOnWriteArraySet<>();
- private final Map<String, ServiceRegistration> bundleReadyMarkerRegistrations = new ConcurrentHashMap<>();
+ private final Map<String, ServiceRegistration<?>> bundleReadyMarkerRegistrations = new ConcurrentHashMap<>();
private final String readyMarkerKey;
- private BundleTracker relevantBundlesTracker;
+ private BundleTracker<Object> relevantBundlesTracker;
/**
* Creates a new instance of this class with the specified parameters.
@@ -125,7 +124,7 @@ public class XmlDocumentBundleTracker<T> extends BundleTracker<Bundle> {
@Override
public final synchronized void open() {
- relevantBundlesTracker = new BundleTracker(context,
+ relevantBundlesTracker = new BundleTracker<Object>(context,
Bundle.RESOLVED | Bundle.STARTING | Bundle.STOPPING | Bundle.ACTIVE, null) {
@Override
public Object addingBundle(Bundle bundle, BundleEvent event) {
@@ -342,21 +341,21 @@ public class XmlDocumentBundleTracker<T> extends BundleTracker<Bundle> {
private void registerReadyMarker(Bundle bundle) {
String bsn = bundle.getSymbolicName();
if (!bundleReadyMarkerRegistrations.containsKey(bsn)) {
- ServiceRegistration reg = ReadyUtil.markAsReady(context, readyMarkerKey, bsn);
+ ServiceRegistration<?> reg = ReadyUtil.markAsReady(context, readyMarkerKey, bsn);
bundleReadyMarkerRegistrations.put(bsn, reg);
}
}
private void unregisterReadyMarker(Bundle bundle) {
String bsn = bundle.getSymbolicName();
- ServiceRegistration reg = bundleReadyMarkerRegistrations.remove(bsn);
+ ServiceRegistration<?> reg = bundleReadyMarkerRegistrations.remove(bsn);
if (reg != null) {
reg.unregister();
}
}
private void unregisterReadyMarkers() {
- for (ServiceRegistration reg : bundleReadyMarkerRegistrations.values()) {
+ for (ServiceRegistration<?> reg : bundleReadyMarkerRegistrations.values()) {
reg.unregister();
}
bundleReadyMarkerRegistrations.clear();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this changes we don't need to supress warnings
Right. That was my initial thought too.
And then I realized that it won't run on OSGi 4.2 anymore (which doesn't have generics in its signatures) and I end up having some very-not-so-amused colleagues 😉
But I agree, it sucks!
Do we really suppress all that warnings for the whole class?
You are right, it's safer to do that on variable level. Will change that.
Thanks for the review and valuable feedback - I will fix it of course. |
addressed the comments so far & rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume I need to read the ThingManager
using the Eclipse IDE (so access to the code base) again. But the remaining part (except the test itself) has been reviewed (more or less detailed).
Have you some setups running using that code base?
if (objects == null) { | ||
return; | ||
} | ||
for (T_OBJECT object : objectList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using an CopyOnWriteArrayList, but also for any other collection, I think it would be "better" to use objects.addAll(objectList)
(so the collection implementation can perform an optimized algorithm to add multiple objects) and only removeCachedEntries on the for-each-loop.
} | ||
|
||
} | ||
public class XmlThingTypeProvider extends AbstractXmlBasedProvider<ThingTypeUID, ThingType> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now
- ConfigXmlConfigDescriptionProvider | esh.scope=core.xml.config
- XmlChannelGroupTypeProvider | esh.scope=core.xml.channelGroups
- XmlChannelTypeProvider | esh.scope=core.xml.channels
- XmlThingTypeProvider | esh.scope=core.xml
To be consistent I would expect the esh.scope should be equal to core.xml.things for the XmlThingTypeProvider (to fir to channel type and channel group type provider).
Why do we hide the "type" at all?
} | ||
|
||
@Deactivate | ||
protected void deactivate(ComponentContext context) { | ||
protected void deactivate(ComponentContext componentContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we can remove the unused function argument at all.
|
||
private XmlDocumentBundleTracker<List<?>> thingTypeTracker; | ||
|
||
@Activate | ||
protected void activate(ComponentContext context) { | ||
protected void activate(ComponentContext componentContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using BundleContext
instead of ComponentContext
if we are interested on the bundle context only?
@@ -299,13 +101,41 @@ public void unsetConfigDescriptionProvider(ConfigDescriptionProvider configDescr | |||
this.configDescriptionProvider = null; | |||
} | |||
|
|||
@Reference(target = "(esh.scope=core.xml)") | |||
@Reference(target = "(esh.scope=core.xml.channels)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume annotated fields does not work with the old DS we need to support. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should simply create a "target" property in the generated DS XML and these properties are already supported in DS1.1/OSGi4.2, see specification chapter 112.3.4.
}); | ||
} | ||
|
||
private String getBundleName(ThingHandlerFactory it) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the argument use the name "it"? It doesn't matter, but IMHO uncommon to use "iterator" (??) for an variable name of an argument.
@@ -30,16 +31,15 @@ | |||
* @author Markus Rathgeb - Add locale provider support | |||
* @author Ana Dimova - fragments support | |||
*/ | |||
@SuppressWarnings("deprecation") | |||
public class ResourceBundleTracker extends ResolvedBundleTracker { | |||
@SuppressWarnings({ "deprecation", "rawtypes", "unchecked" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we already discussed the "omit supress warnings for the whole class if possible" already.
Isn't it possible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for "rawtypes", as it complains about
public class ResourceBundleTracker extends BundleTracker
so whatever you like better: living with the warning or with the too generous suppression, I'm fine with both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only "solution" I found has been https://github.com/maggu2810/smarthome/commit/b7785d751d10e9f0fb1444a3ebfa194f206d074b which combines the suppress warnings to the sections that really need it.
But if it is necessary to create separate classes... I assume you will tell me, that it isn't 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it really feels wrong to me to create a separate class just for this specific reason 😄
* | ||
*/ | ||
@SuppressWarnings({ "rawtypes", "unchecked" }) | ||
public class ReadyUtil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC we already discussed the "omit supress warnings for the whole class if possible" already.
Isn't it possible here?
@SJKA You don't wait for me, do you? |
And for me - I planned to review and test today as well. |
What I want to say has been: |
No, I was just waiting for @kaikreuzer's feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work, especially as it removes a lot of code and all still seems to work 😎
I must admit that I didn't review all methods in all detail, but I did functional tests on it without any issues to report.
So once you have addressed @maggu2810's and my comments, this should be good to merge.
* | ||
* @author Simon Kaufmann - initial contribution, factored out of subclasses | ||
* | ||
* @param <T_ID> the key type, e.g. ThingTypeUID, ChannelUID, URI,... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to refactor this to use Identifiable
now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* @return the object if found, <code>null</code> otherwise | ||
*/ | ||
protected final T_OBJECT get(T_ID key, Locale locale) { | ||
for (Entry<Bundle, List<T_OBJECT>> objects : bundleObjectMap.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might actually be far more elegant using lambdas - that might also be a consideration for some of the other methods here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't get it to look more elegant, so I rather left it as it use. At least it is readable.
* @param object the object to return the identifier for | ||
* @return the identifier. Must not be <code>null</code> | ||
*/ | ||
protected abstract T_ID getIndentifier(T_OBJECT object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will probably be replaced by getUID from Identifiable
, but if not, you might want to fix the typo (Indent vs. Ident).
* a fragment. So the fragment bundle resources can override the | ||
* host bundles resources. | ||
* | ||
* @param xmlDocumentPaths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you miss adding some content here?
if (!remainingBundles.isEmpty()) { | ||
logger.trace("Remaining bundles with {}: {}", xmlDirectory, remainingBundles); | ||
} else { | ||
logger.debug("Finished loading bundles with {}", xmlDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to get one of these messages for every bundle in my installation...
Not really sure if we are at all expected to hit this line so often (if so, better reduce to trace) or if there is some issue in the processing and we should not run out of remainingBundles at startup continuously.
@@ -299,13 +101,41 @@ public void unsetConfigDescriptionProvider(ConfigDescriptionProvider configDescr | |||
this.configDescriptionProvider = null; | |||
} | |||
|
|||
@Reference(target = "(esh.scope=core.xml)") | |||
@Reference(target = "(esh.scope=core.xml.channels)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should simply create a "target" property in the generated DS XML and these properties are already supported in DS1.1/OSGi4.2, see specification chapter 112.3.4.
initializeHandler(thing); | ||
} else { | ||
logger.debug( | ||
"Not registering a handler at this point. The thing types of bundle {} are not fully loaded yet.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was that the auto formatter or you yourself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was him (of course)
*/ | ||
public interface ReadyMarker { | ||
|
||
public final static String XML_THING_TYPE = "esh.xmlThingTypes"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not completely happy to define those constants in smarthome.core as this bundle in theory does not know anything about the existence of the xml bundles.
* added a ready marker service interface * registering ready markers for xml thing types * simplified the code by eliminating the AbstractAsyncBundleProcessor * Xml based providers register a ready marker * Dedicated ready markers are registered for each binding's thing types * Using a thread pool for bundle processing instead of spawning an own thread * ThingManager and GenericThingProvider listening to the ready marker registrations. * dumped ResolvedBundleTracker in favor of simply using the plain OSGi BundleTracker * factored out common aspects of all xml based providers into AbstractXmlProvider * Split XmlChannelTypeProvider and XmlChannelGroupTypeProvider Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
namely * BindingInfo * AbstractDescriptionType * ConfigDescription Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Thanks for updates, looks all good to me now! @maggu2810 Feel free to check if your comments have been addressed as expected as well - if so, please merge :-) |
@SJKA : could you explain what issues your PR will solve ? |
No, it doesn't. I is just a major cleanup within the core XML processing. |
@@ -113,7 +115,8 @@ public ConfigDescription(URI uri, List<ConfigDescriptionParameter> parameters, | |||
* | |||
* @return the URI of this description (not null) | |||
*/ | |||
public URI getURI() { | |||
@Override | |||
public URI getUID() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SJKA I just found this one here, which leads to some broken bindings in OH2 repo (see Travis failure here). Seems to be an APIBreaking, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that this change breaks the openHAB build: https://openhab.ci.cloudbees.com/job/openHAB2-Bundles/99/console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should hopefully be fixed with https://github.com/openhab/openhab2-addons/pull/2476.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
By the way, the "root cause" was the introduction of the identifiable interface in #3762.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
this PR is a first step accounting to #1896.
Its main objective is to get rid of the BundleProcessorVetoManager and replace it with ReadyMarker service registrations per bundle. This results in much cleaner and less entangled code (as you can see, this actually is a "negative" contribution).
While implementing this, I had to visit some places which I couldn't touch without cleaning them up properly. So here is a brief summary of what happened:
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com