Allon Mureinik has posted comments on this change.
Change subject: core,webadmin: GetConfigurationValuesQuery for UI cache
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetConfigurationValuesQuery.java
Line 13: import
org.ovirt.engine.core.common.queries.GetConfigurationValueParameters;
Line 14: import org.ovirt.engine.core.compat.KeyValuePairCompat;
Line 15: import org.ovirt.engine.core.compat.Version;
Line 16:
Line 17: public class GetConfigurationValuesQuery<P extends
GetConfigurationValueParameters> extends QueriesCommandBase<P> {
This parameter class is a wrapper to the config value you want to get - which
isn't relevant here, 'cause you're getting all of them.
Just use VdcQueryParametersBase
Line 18:
Line 19: private static Set<String> configValuesSet;
Line 20:
Line 21: public GetConfigurationValuesQuery(P parameters) {
Line 15: import org.ovirt.engine.core.compat.Version;
Line 16:
Line 17: public class GetConfigurationValuesQuery<P extends
GetConfigurationValueParameters> extends QueriesCommandBase<P> {
Line 18:
Line 19: private static Set<String> configValuesSet;
I'd set this as final, and create it here, inline.
Line 20:
Line 21: public GetConfigurationValuesQuery(P parameters) {
Line 22: super(parameters);
Line 23:
Line 20:
Line 21: public GetConfigurationValuesQuery(P parameters) {
Line 22: super(parameters);
Line 23:
Line 24: configValuesSet = new HashSet<String>();
in addition to moving it, init it in the right size
-ConfigValues.values().size()
Line 25: }
Line 26:
Line 27: @Override
Line 28: protected void executeQueryCommand() {
Line 29: Map<KeyValuePairCompat<ConfigurationValues, String>, Object>
configValuesMap =
Line 30: new HashMap<KeyValuePairCompat<ConfigurationValues,
String>, Object>();
Line 31:
Line 32: // Cache ConfigValues in String set
Line 33: createConfigValuesSet();
this should be done in a static block - it's a waste to do it over and over
again each time.
Line 34:
Line 35: for (ConfigurationValues configValue :
ConfigurationValues.values()) {
Line 36: // Ignore an admin configuration value on filtered mode
Line 37: // Ignore a configuration value that doesn't exist in
ConfigValues enum
Line 33: createConfigValuesSet();
Line 34:
Line 35: for (ConfigurationValues configValue :
ConfigurationValues.values()) {
Line 36: // Ignore an admin configuration value on filtered mode
Line 37: // Ignore a configuration value that doesn't exist in
ConfigValues enum
This isn't interesting, IMHO.
It's a bug, and should be verified with a test in build time, not in runtime.
Line 38: if (!shouldReturnValue(configValue) ||
!configValuesSet.contains(configValue.toString())) {
Line 39: continue;
Line 40: }
Line 41:
Line 39: continue;
Line 40: }
Line 41:
Line 42: // Creating versions list
Line 43: List<String> versions = new ArrayList<String>();
Here too - init it in the right size - Version.ALL.size().
In addition - this never changes in runtime - you should have this in a static
block.
Line 44: versions.add(Config.DefaultConfigurationVersion);
Line 45: for (Version version : Version.ALL) {
Line 46: versions.add(version.toString());
Line 47: }
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/generic/DBConfigUtils.java
Line 238: // Couldn't find this value at all, adding to cache.
Line 239: Map<String, Object> defaultValues = new
HashMap<String, Object>();
Line 240: defaultValues.put(version, returnValue);
Line 241: _vdcOptionCache.put(option.getoption_name(),
defaultValues);
Line 242: log.debug("Adding new value to configuration cache.");
why is this relevant to this patch?
Line 243: }
Line 244: log.debugFormat("Didn't find the value of {0} in DB for
version {1} - using default: {2}",
Line 245: name, version, returnValue);
Line 246: }
Line 240: defaultValues.put(version, returnValue);
Line 241: _vdcOptionCache.put(option.getoption_name(),
defaultValues);
Line 242: log.debug("Adding new value to configuration cache.");
Line 243: }
Line 244: log.debugFormat("Didn't find the value of {0} in DB for
version {1} - using default: {2}",
same here?
Line 245: name, version, returnValue);
Line 246: }
Line 247:
Line 248: return returnValue;
--
To view, visit http://gerrit.ovirt.org/8563
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id51a9f86bceef07de728335643b16c0554b6876c
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
