ConfigDispatcher: Support for multiple configurations per service #4685

triller-telekom · 2017-12-06T13:37:55Z

Signed-off-by: Stefan Triller stefan.triller@telekom.de

triller-telekom · 2017-12-06T14:10:51Z

I am creating factory configurations based on the "exclusive pid" in the first line of a configuration file, i.e. pid:service.pid#configuration1 will create one service with pid "service.pid" and pid:service.pid#configuration2 will create another service with pid "service.pid" but with a different configuration.

I am "tagging" these configurations in ConfigAdmin via a special property named service.context so I can map the files to a Configuration object in the ConfigAdmin.

htreu

Please see inline comments: The "exclusive PIDs can not be overridden" feature is not existent for exclusive PIDs with context.

htreu · 2017-12-06T14:40:35Z

....dispatch/src/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcher.java

            lines = lines.subList(1, lines.size());
-            exclusivePIDMap.setProcessedPID(pid, configFile.getAbsolutePath());
+            exclusivePIDMap.setProcessedPID(exclusivePID, configFile.getAbsolutePath());


Its the right thing to add the full PID including the context here. Although, in line 421 (not part of this PR) the exclusivePID map is checked, whether the given PID is existent there: This should prevent the ConfigDispatcher from overwriting already parsed exclusive configs!
Currently this is possible (there is also a test for this).
See line 409 - 411 for further comments.

Are you sure that the current implementation skips the content of the second file?

In https://github.com/eclipse/smarthome/blob/master/bundles/config/org.eclipse.smarthome.config.dispatch/src/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcher.java#L351

It creates an empty Properties dictionary and fills it with the content of the second file and then updates the configuration with it.

That's why in this PR I let the duplicate file overwrite the configuration...

Yes, I´m sorry! See here: https://github.com/eclipse/smarthome/pull/4685/files#diff-f38a1a84005ec66f7434ebe672d47578R372 for the duplicate check. But it only gives a warning to the log. So overwriting is okay.

htreu · 2017-12-06T14:42:55Z

....dispatch/src/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcher.java

+                configuration = configAdmin.createFactoryConfiguration(pid, null);
+                Dictionary p = new Properties();
+                p.put(SERVICE_CONTEXT, context);
+                configuration.update(p);


This only works because the config is not overridden by the check in line 422: When you correct the check there to also take the context into account the SERVICE_CONTEXT property set here will be gone. May be the SERVICE_CONTEXT should be set around the check in line 422 for exclusive PIDs with context, right after creating a blank dictionary for this configuration.

Since we dont talk about overwriting an existing exclusive config any more I like to stress this issue again: This works only because the check in line 422 checks for the pid which is stripped from the context in this case.
I would like it better to have the new Properties() and p.put(SERVICE_CONTEXT, context) somewhere around the check if we are dealing with an exclusive PID.

I have put the creating of the new properties dictionary near the place where the empty properties are created for the non-context cases.

htreu

Minor comments and a refactoring proposal.

htreu · 2017-12-07T15:56:49Z

....test/src/test/java/org/eclipse/smarthome/config/dispatch/test/ConfigDispatcherOSGiTest.java

+        verifyValueOfConfigurationPropertyWithContext("service.pid#ctx1", "property1", "valueDup");
+        // ctx2 is parsed as is
+        verifyValueOfConfigurationPropertyWithContext("service.pid#ctx2", "property1", "value2");
+


htreu · 2017-12-07T17:34:47Z

....dispatch/src/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcher.java

@@ -260,8 +213,8 @@ private void readDefaultConfig() {
        }
    }

-    private void readConfigFiles() {
-        File dir = getSourcePath().toFile();
+    private void readConfigFiles(Path pathToWatch) {


In the ConfigDispatcher the path should be named like basePath since the CD does not watch paths any more.

On a second thought the initial processing of the files in the base config dir might also be a good thing the new ConfigDispatcherFileWatcher could do after activation. It would simply call configDispatcher.processFile in the for loop. Or to make the separation even more fine granular: Let the CDFileWatcher list the files in the base config dir and pass it to a new public void processFiles(List<File> files) on the ConfigDispatcher which does the sorting and processing.
wdyt?

Since the ConfigDispatcher has a strict ordering in its constructor the calls to processOrphanExclusivePIDs() and storeCurrentExclusivePIDList() should be done afterwards in the new processFiles(List<File> files) method.
This will also slim down the c'tor of the ConfigDispatcher :-)

htreu · 2017-12-07T18:00:40Z

....dispatch/src/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcher.java

            lines = lines.subList(1, lines.size());
-            exclusivePIDMap.setProcessedPID(pid, configFile.getAbsolutePath());
+            exclusivePIDMap.setProcessedPID(exclusivePID, configFile.getAbsolutePath());
+


htreu · 2017-12-08T10:11:52Z

....dispatch/src/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcher.java

-                } catch (IOException e) {
-                    logger.warn("Could not process config file '{}': {}", file.getName(), e.getMessage());
-                }
+                processConfigFile(file);


This will now recursively process all subdirectories from services. Is this intended? You may just call internalProcessConfigFile here to restore the original behaviour.

Good catch! I had it like that before and thought that this can be done "easier", but I was wrong. Thanks for pointing me to that mistake!

htreu

Looks good.

htreu · 2017-12-12T13:20:32Z

Please merge here, a follow up PR is waiting for this functionality :-)

kaikreuzer · 2017-12-12T13:23:15Z

You can easily base your local branch on top of this branch - nothing should block you from working on other PRs. Simply rebase your stuff once this PR is merged.

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

…Watcher Separate the processing of file-system events from our ConfigDispatcher tasks, to simplify tests. Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

sjsf

see inline.

One general question: Is there any particular reason what this mechanism works only for exclusive PIDs?

sjsf · 2017-12-19T10:03:48Z

....test/src/test/java/org/eclipse/smarthome/config/dispatch/test/ConfigDispatcherOSGiTest.java

+        ConfigDispatcher cd = new ConfigDispatcher(bundleContext, configAdmin);
+        cd.processConfigFile(new File(absoluteConfigDirectory));
+
+        // Modify this file, so that we are sure it is the last modified file


As you added this comment here... is touching the file really still necessary, now that the ConfigDispatcher is separated from the file system watcher and you call processConfigFile(...) on it directly?

sjsf · 2017-12-19T10:05:00Z

....test/src/test/java/org/eclipse/smarthome/config/dispatch/test/ConfigDispatcherOSGiTest.java

-        initialize(configDirectory, servicesDirectory, defaultConfigFileName);
+        initialize(defaultConfigFileName);
+
+        String absoluteConfigDirectory = configDirectory + SEP + servicesDirectory;


Wouldn't it have been worth extracting this block to a method instead of repeating it all over the place?

sjsf · 2017-12-19T10:08:33Z

....test/src/test/java/org/eclipse/smarthome/config/dispatch/test/ConfigDispatcherOSGiTest.java

+            pid = pidWithContext.split(ConfigDispatcher.SERVICE_CONTEXT_MARKER)[0];
+            configContext = pidWithContext.split(ConfigDispatcher.SERVICE_CONTEXT_MARKER)[1];
+        } else {
+            fail("PID does not have a context");


java throw new IAE(...)

It's nothing you assert about the subject under test but rather a bug in the test code itself.

sjsf · 2017-12-19T10:09:17Z

....test/src/test/java/org/eclipse/smarthome/config/dispatch/test/ConfigDispatcherOSGiTest.java

+            configs = configAdmin.listConfigurations("(&(service.factoryPid=" + pid + ")("
+                    + ConfigDispatcher.SERVICE_CONTEXT + "=" + configContext + "))");
+        } catch (IOException e) {
+            fail("IOException occured while retrieving configuration for pid " + pidWithContext);


same here - these should stay exceptions

sjsf · 2017-12-19T10:12:45Z

....test/src/test/java/org/eclipse/smarthome/config/dispatch/test/ConfigDispatcherOSGiTest.java

+        if (configs == null) {
+            return null;
+        }
+        // otherwise we should have exactly one configuration


superfluous comment - it states the obvious and has the risk of getting out-of-sync with the code

sjsf · 2017-12-19T10:21:06Z

...rc/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcherFileWatcher.java

+ *
+ */
+@Component(immediate = true)
+public class ConfigDispatcherFileWatcher extends AbstractWatchService {


It's good that you factored it out of the config dispatcher and also that its tests do not rely on real file changes anymore. However, you essentially fixed the test which failed on windows by simply dropping them, hence leaving this classes logic completely untested now. Sound like a regression to me. Or did I miss anything?

sjsf · 2017-12-19T10:22:06Z

...rc/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcherFileWatcher.java

+/**
+ * Watches file-system events and passes them to our {@code ConfigDispatcher}
+ *
+ * @author Stefan Triller - initial contribution


no, you factored it out of another class.

sjsf · 2017-12-19T10:23:00Z

...rc/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcherFileWatcher.java

+        }
+    }
+
+    @Reference(cardinality = ReferenceCardinality.MANDATORY, policy = ReferencePolicy.STATIC)


STATIC/MANDATORY is the default, hence just @Reference is enough

From my observations at least the IDE does produce different XML in both cases: @Reference only does not put 1..1& static into the XML.

sjsf · 2017-12-19T10:36:43Z

....test/src/test/java/org/eclipse/smarthome/config/dispatch/test/ConfigDispatcherOSGiTest.java

+        cd.fileRemoved(serviceConfigFile.getAbsolutePath());
+
+        Configuration c1 = getConfigurationWithContext("service.pid#ctx1");
+        // test if configuration with context ctx1 was deleted


you added a commend and a failure message. Both are best case superfluous by stating the obvious or even worse are harmful to readability by stating the exact opposite of what actually should be asserted here. Please drop them both. Everywhere. (Won't repeat that at every occurrence)

sjsf · 2017-12-19T10:52:00Z

....dispatch/src/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcher.java

+        } else if (context != null && exclusivePIDMap.contains(exclusivePID)) {
+            Dictionary p = new Properties();
+            p.put(SERVICE_CONTEXT, context);
+            configuration.update(p);


what is this line here intended to do in any of these cases?

new service configuration as created in line 324

an existing service configuration as grabbed in line 316

The Configuration.update() method is the information for the config admin that the config has changed and it will instantiate the new service instance with these (here empty) properties. The same applies to existing ones: they will get modified with empty properties, just to be updated later again with the real values again.

I understand that you just copied the same pattern from below - which doesn't make it any better and should be fixed there too. This explains why all of our services always get instantiated with "null" configurations at first and then get an updated afterwards immediately with the real values... 😄 This created unneeded restarts and re-initializations of all kinds of crap transitively.

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Notify configadmin only if there is a new configuration Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

triller-telekom · 2017-12-19T14:36:34Z

One general question: Is there any particular reason what this mechanism works only for exclusive PIDs?

Since the purpose of different contexts is that they hold configurations for different use cases I thought it makes sense to also separate the use cases into different files, each holding one configuration for exactly one use case.

I am not exactly sure on how to properly test the new ConfigDispatcherFileWatcher. Its solely purpose is to react on file-system changes and then delegate the work to the ConfigDispatcher. So all I could check is if the void processWatchEvent(WatchEvent<?> event, Kind<?> kind, Path path) method is called with the right arguments if I put a file on the file-system. But isn't this basic java stuff and we would have other problems if this wouldn't work? Also this is a OGSi service and creating an OSGi test for such simply java build-in functionality is a bit overkill, isn't it? Please correct me if my assumptions are wrong and point me to a solution.

// Modify this file, so that we are sure it is the last modified file

As you added this comment here... is touching the file really still necessary, now that the ConfigDispatcher is separated from the file system watcher and you call processConfigFile(...) on it directly?

Yes, this is necessary because the tests itself look into the file-system to find the last modified file and then compare it against the value in the configuration, see https://github.com/eclipse/smarthome/blob/master/bundles/config/org.eclipse.smarthome.config.dispatch.test/src/test/java/org/eclipse/smarthome/config/dispatch/test/ConfigDispatcherOSGiTest.java#L798-L838

htreu · 2017-12-19T14:48:05Z

I'd like to add to the ConfigDispatcherFileWatcher test discussion: We could easily make the ConfigDispatcher an OSGi service and attach it to the CDFW using a setter (mandatory, static). Then set up a plain unit test for CDFW and providing a mock ConfigDispatcher. Then simply call the processWatchEvent method while verifying the correct call on the ConfigDispatcher mock. wdyt?

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

kaikreuzer · 2018-01-03T08:39:33Z

....dispatch/src/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcher.java

 */
-@Component(immediate = true)


Is it intended to remove the immediate=true?

kaikreuzer · 2018-01-03T08:40:08Z

...rc/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcherFileWatcher.java

+    @Activate
+    public void activate(BundleContext bundleContext) {
+        super.activate();
+        // configDispatcher = new ConfigDispatcher(bundleContext);


Can this be removed?

kaikreuzer · 2018-01-03T08:40:26Z

...rc/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcherFileWatcher.java

+    @Override
+    public void deactivate() {
+        super.deactivate();
+


remove empty line

kaikreuzer · 2018-01-03T08:41:29Z

...rc/main/java/org/eclipse/smarthome/config/dispatch/internal/ConfigDispatcherFileWatcher.java

+    }
+
+    @Activate
+    public void activate(BundleContext bundleContext) {


Better override the activate() method without a parameter.

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

…lipse-archived#4685) * ConfigDispatcher: Support for multiple configurations per service * Split ConfigDispatcher into ConfigDispatcher and ConfigDispatcherFileWatcher * Move initial config processing out of ConfigDispatcher constructor * Do not accidentally parse subdirectories * Remove unnecessary update on configuration Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

triller-telekom mentioned this pull request Dec 6, 2017

Support hierarchical configuration & exclusive cfg files #4178

Closed

7 tasks

htreu suggested changes Dec 6, 2017

View reviewed changes

htreu reviewed Dec 7, 2017

View reviewed changes

htreu mentioned this pull request Dec 8, 2017

Running the build "mvn clean install" fails on window 10 #4370

Closed

htreu reviewed Dec 8, 2017

View reviewed changes

htreu approved these changes Dec 8, 2017

View reviewed changes

triller-telekom added 11 commits December 12, 2017 14:27

ConfigDispatcher: Support for multiple configurations per service

1bb1890

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Copy duplicate file into services folder after initialization

f5d2013

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Ensure file has been copied in test case

3ddb092

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Create properties of a new configuration in one place

808d58d

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Wrap checks in waitForAssert

1a28dd8

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Split ConfigDispatcher into ConfigDispatcher and ConfigDispatcherFile…

da7b284

…Watcher Separate the processing of file-system events from our ConfigDispatcher tasks, to simplify tests. Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Simplify tests by removing file watching logic

1fe07a3

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Touch last modified test files

a6ac873

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Rename service context to avoid future name clashes with OSGI

acec8e4

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Move initial config processing out of ConfigDispatcher constructor

64f87dc

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Do not accidentally parse subdirectories

408caea

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

This was referenced Dec 14, 2017

Eclipse SmartHome Configuration Dispatcher Tests failing #4734

Closed

dispatch.test FAIL #4758

Closed

sjsf suggested changes Dec 19, 2017

View reviewed changes

triller-telekom added 7 commits December 19, 2017 13:39

Remove reasons from assertThat statements for more readability

15a172a

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Replace fail() with IllegalArgumentException

e24cf04

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

remove comments which might get out of sync

254a86f

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

removed duplicate checks

4219d9d

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Extract config path creation into private method

f733f6b

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Remove unnecessary update on configuration

10013a2

Notify configadmin only if there is a new configuration Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

lines breaks due to code formatter on test file

afbcfc9

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Corrected authorship

47b7a52

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

triller-telekom force-pushed the serviceFactory branch from cccc1ac to 47b7a52 Compare December 19, 2017 14:25

triller-telekom added 3 commits December 20, 2017 10:53

ConfigDispatcher is now a OSGi service again

2d678c4

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

Tests for ConfigDispatcherFileWatcher

c964f15

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

fix wrong method name

5985e6a

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

htreu approved these changes Dec 21, 2017

View reviewed changes

kaikreuzer suggested changes Jan 3, 2018

View reviewed changes

triller-telekom added 2 commits January 3, 2018 09:49

start dispatcher immediately + activate method without params

6817f81

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

ConfigDispatcher has to provide itself as a service

5408a9d

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

kaikreuzer approved these changes Jan 3, 2018

View reviewed changes

sjsf approved these changes Jan 3, 2018

View reviewed changes

sjsf merged commit c73ec3b into eclipse-archived:master Jan 3, 2018

triller-telekom deleted the serviceFactory branch January 3, 2018 14:30

kaikreuzer mentioned this pull request Jan 3, 2018

smarthome.config.xml.test failure: NoClassDefFound #4837

Closed

This was referenced Jan 10, 2018

SSL/DTLS service #4105

Closed

Core/config: Add NestedConfigConverter #4234

Closed

sjsf mentioned this pull request Feb 2, 2018

Adapt MQTT service to the multi-config concept #5015

Closed

htreu added this to the 0.10.0 milestone Oct 30, 2018

		*/
		@Component(immediate = true)

ConfigDispatcher: Support for multiple configurations per service #4685

ConfigDispatcher: Support for multiple configurations per service #4685

Conversation

triller-telekom commented Dec 6, 2017 • edited Loading

triller-telekom commented Dec 6, 2017

htreu left a 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

htreu left a 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

htreu left a comment

Choose a reason for hiding this comment

htreu commented Dec 12, 2017

kaikreuzer commented Dec 12, 2017

sjsf left a 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

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

triller-telekom commented Dec 19, 2017

htreu commented Dec 19, 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

triller-telekom commented Dec 6, 2017 •

edited

Loading