Merlijn, Apologies for the delayed reply. I realized that I had typed this up but forgotten to actually send it.
You're right that there are still cases where the hook-persistent nature of the config.changed states continue to cause problems. However, after some discussion with Ben, I actually think that *any* sort of automatic state removal is the wrong approach, whether it happens at the end of a hook or at the end of an dispatch loop (essentially what you're proposing with events). Instead, Ben convinced me that the right thing to do is to always have states be explicitly acknowledged and removed by the handlers. This doesn't work as expected currently because of an implementation detail of how the handler queue is managed on state removals, but I think it's more appropriate to fix that rather than add a new type of state. In that approach, the config.changed state would be set when the change is detected, all applicable handlers that are watching for it would execute, each one explicitly acknowledging that it's been handled by removing it, and then, after all handlers are done, the removals would be applied. Note that the initial handler (e.g., install or start_service) would also need to clear the changed state if present to prevent the secondary handler (reinstall or restart_service) from acting on it. Alternatively, the approach I took for the ibm-base layer was to remove the gating state and separate reinstall handlers entirely, and always just drive off the config.changed states: https://code.launchpad.net/~johnsca/layer-ibm-base/fix-multi-call/+merge/292845 Note that there is still some potential semantic value to having "new" and "changed" be distinguishable, but perhaps it's not as valuable enough to worry about. On Fri, Apr 22, 2016 at 6:57 PM, Merlijn Sebrechts < merlijn.sebrec...@gmail.com> wrote: > Hi Cory > > > > I think this is another symptom of the deeper issue that the reactive > framework treats events like states. 'config.changed' is an event. The > following code is something that intuitively seems correct. We want to > reinstall when the config has changed while the service is installed. > However, it will still have the unwanted side effect you stated earlier. > > @when('installed', 'config.changed.install_source')def reinstall(): > install() > > > Please note that *your fix will only work when the service is installed > during the first config-changed hook*. If a service is installed during a > subsequent config-changed hook, you will again have the same issue. This > can happen when you have config options such as "(bool)install_plugin-x" > and "(string)plugin-x-source". > > Anticipating these kind of conflicts requires a thorough understanding of > both the reactive framework and hooks. You are correct in thinking that > these conflicts should not happen. If we require every Charmer to have full > understanding of these things, we might miss out on valuable contributions. > > > I would urge you to seriously consider making the differentiation between > events and states. For people who have used hooks it might seem logical > that config.changed is active during an entire hook. Newcomers might have > more difficulty understanding this. > > So my suggestion is: > > - An event is only active right after the event happens. > - A handler can only be added to the queue when his events + his states > are active > - A handler will be removed from the queue only when one of his states > becomes inactive. Events of handlers that are in the queue are not > 'rechecked'. > > > Another use-case for this: > > @when('service.running', 'configfile.changed') > def restart_service() > > 1. When the config file changes, and the service is running, restart the > service. > 2. When the config file changes and the service is not running, don't > restart the service. > 3. When the config file changed before the service was running, and now > we start the service, don't restart the service. > 4. When the config file changes, the service restarts, and the config > file changes again, we want to restart the service again. > > 1 and 2 are currently possible. 3 and 4 would be if 'file.changed' would > be an event. > > > > > > Kind regards > Merlijn Sebrechts > > 2016-04-22 23:02 GMT+02:00 Cory Johns <cory.jo...@canonical.com>: > >> I have proposed https://github.com/juju-solutions/layer-basic/pull/61 as >> a slight change to how the config.changed states from the basic layer >> work. Currently, the changed states are set during the first hook >> invocation, under the assumption that the values were "changed" from >> "nothing" (not being set at all). However, this is slightly problematic in >> a case like the following, where we expect install() to only be called >> once, unless the value has changed after the fact: >> >> @when_not('installed')def install(): >> # do install >> set_state('installed') >> @when('config.changed.install_source')def reinstall(): >> install() >> >> >> The proposal adds new states, config.new, and changes config.changed to >> not be set the first time. You could get the old behavior by saying >> @when_any('config.new.foo', 'config.changed.foo'). >> >> Is anyone depending on the current behavior? Are there any objections to >> this change? >> >> -- >> Juju mailing list >> Juju@lists.ubuntu.com >> Modify settings or unsubscribe at: >> https://lists.ubuntu.com/mailman/listinfo/juju >> >> >
-- Juju mailing list Juju@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju
