Martin Mucha has posted comments on this change. Change subject: core: Fire CDI event with all reloaded VdcOptions upon ReloadConfigurationsCommand ......................................................................
Patch Set 6: (2 comments) https://gerrit.ovirt.org/#/c/34657/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ReloadConfigurationsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ReloadConfigurationsCommand.java: Line 27: @Override Line 28: protected void executeCommand() { Line 29: List<VdcOption> updatedConfigValues = DBConfigUtils.refreshReloadableConfigsInVdcOptionCache(); Line 30: if (!updatedConfigValues.isEmpty()) { Line 31: updatedConfigValuesEvent.fire(new UpdatedConfigValues(updatedConfigValues)); > where is the observer ? who should handle this event ? sole observer is abandoned; it was considered "unwanted" after it was entirely implemented. However, this piece should be merged, since having enterprise app which has to restart to reload it's configuration is synonym for failure. So observer should be created for all services reading it's configuration from db. And due to high confusion of whole team (ours, infra, ...) whether this command exist, whether it should exist, and whether it works, proper support for this should be communicated from someone responsible unless we really want "restartful" app design. Line 32: } Line 33: setSucceeded(true); Line 34: } Line 35: https://gerrit.ovirt.org/#/c/34657/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/generic/DBConfigUtils.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/generic/DBConfigUtils.java: Line 281: boolean changed; Line 282: Line 283: if (values == null) { Line 284: values = new HashMap<>(); Line 285: _vdcOptionCache.put(option.getoption_name(), values); > can't lines 285-293 be replaced with a simple if ? ad given code: no, this probably cant be used. We need to differentiate between versions. If value for former version would be same as value for new version, the new version wouldn't be added to the cache. Maybe it would serve our purpose just well(I don't now, I won't check), but cache wouldn't be complete. I tried to use "assign to map value from param always" even if not necessary (which could be observed as premature optimization). Now the code is simpler. I purposedly left there explanatory variables, which can actually be inlined, but I don't care about LOC metric and inlining them would the code less readable for me. Line 286: changed = true; Line 287: } else { Line 288: Object value = values.get(optionVersion); Line 289: boolean newVersion = value == null; -- To view, visit https://gerrit.ovirt.org/34657 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I89b1d94626b4210cd2a7fb04efc4ee9cc490dd6c Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
