Re: [Engine-devel] Using config values

----- Original Message ----- > From: "Omer Frenkel" <ofrenkel(a)redhat.com&gt; > To: "Mike Kolesnik" <mkolesni(a)redhat.com&gt; > Cc: "engine-devel" <engine-devel(a)ovirt.org&gt; > Sent: Sunday, December 1, 2013 11:09:32 AM > Subject: Re: [Engine-devel] Using config values > > > > > > > From: "Mike Kolesnik" <mkolesni(a)redhat.com&gt; > To: "Omer Frenkel" <ofrenkel(a)redhat.com&gt; > Cc: "Kanagaraj" <kmayilsa(a)redhat.com&gt;, "engine-devel" > <engine-devel(a)ovirt.org&gt; > Sent: Sunday, December 1, 2013 11:01:50 AM > Subject: Re: [Engine-devel] Using config values > > > > > > > > > > > From: "Mike Kolesnik" <mkolesni(a)redhat.com&gt; > To: "Kanagaraj" <kmayilsa(a)redhat.com&gt; > Cc: "engine-devel" <engine-devel(a)ovirt.org&gt; > Sent: Sunday, December 1, 2013 8:08:42 AM > Subject: Re: [Engine-devel] Using config values > > > > > Hi All, > > Hi Kanagaraj, > > > > > The are some issues arising in configurations whenever we move up on the > versions(3.3 => 3.4), because of the way we store and interpret them. > > Whenever there is a new cluster level, you will need to add a new entry for > all(most) of the configuration. Mostly a copy paste if you see from 3.2 to > 3.3, except some CPU/PM type related configurations. > Better option would be to have the defaul config value in ConfigValues.java > and the overrides will go to config.sql. In this approach you don't need a > new entries to config.sql when there is a new cluster level. > > Lets take an exmaple, "SupportForceCreateVG" - This is supported from 3.1 > onwards, > > If you look at config.sql, you will see following entries > select fn_db_add_config_value('SupportForceCreateVG','false','3.0'); > select fn_db_add_config_value('SupportForceCreateVG','true','3.1'); > select fn_db_add_config_value('SupportForceCreateVG','true','3.2'); > select fn_db_add_config_value('SupportForceCreateVG','true','3.3'); > > And in ConfigValues.java > > @TypeConverterAttribute(Boolean.class) > @DefaultValueAttribute("false") > SupportForceCreateVG, > > Now if there is 3.4 and 3.5, the user needs to add 2 more entries, which i > feel is redundant. > > Instead we can make > > @TypeConverterAttribute(Boolean.class) > @DefaultValueAttribute("true") > SupportForceCreateVG, > > and have only the following in config.sql > select fn_db_add_config_value('SupportForceCreateVG','false','3.0'); > > if a particular value(for a specific cluster level) is not found in > Config.sql, the fallback is to use the value available in ConfigValues.java. > > This has already been implemented, there are many "feature supported" > configurations working like this (for example GlusterSupport). > > I think a more interesting approach would be to move these out of the DB > since these values don't really hav e a reson to be there. > Since the entire thing is abstracted by "Gluster/FeatureSupported" classes > then we can easily change mechanism (of course whatever code is not using it > can be easily converted to use the mechanism) > > For example a simple enum could do the trick: > ------------------------------------- EXAMPLE > ------------------------------------- > /** > * Convenience class to check if a gluster feature is supported or not in any > given version.<br> > * Methods should be named by feature and accept version to check against. > */ > public class GlusterFeatureSupported { > /** > * @param version > * Compatibility version to check for. > * @return <code>true</code> if gluster support is enabled, <code>false</code> > if it's not. > */ > public static boolean gluster(Version version) { > return SupportedFeatures.GLUSTER.isSupportedOn(version); > } > > /** > * @param version > * Compatibility version to check for. > * @return <code>true</code> if gluster heavyweight refresh is enabled, > <code>false</code> if it's not. > */ > public static boolean refreshHeavyWeight(Version version) { > return SupportedFeatures.REFRESH_HEAVYWEIGHT.isSupportedOn(version); > } > > /* More methods... */ > > enum SupportedFeatures { > GLUSTER(Version.v3_0), > REFRESH_HEAVYWEIGHT(Version.v3_0, Version.v3_1), > /* More members */; > > private Set<Version> unsupportedVersions = new HashSet<Version>(); > > private SupportedFeatures(Version... versions) { > unsupportedVersions.addAll(Arrays.asList(versions)); > } > > public boolean isSupportedOn(Version version) { > return !unsupportedVersions.contains(version); > } > } > ------------------------------------- END EXAMPLE > ------------------------------------- > > Thoughts? > > unless i didn't understand something, this is not good, > this should stay configurable by the users, > for example if some user experience some issues with a feature and want to > turn it off/change the values.. > (not all version configuration are boolean, some are different values to > different versions, like cpu-list) > > This is for API level compatibility. > If VDSM doesn't support for example hot plug in 3.1 then the user can't just > decide that it does and change it. > Also, this is not changeable by user since it's not exposed by engine-config > (nor should it be). > some are exposed > > > > This is strictly for the engine-VDSM API compatibility, not for other configs > which are version specific. > right, but still user should be able to turn features off in case of > problems, > or change in some cases (for example it is possible to add support for more > power management devices, i know it was done by users) > no reason to block this > In this case you add additional protection level to block user from enabling features for unsupported cluster levels. However it sounds more reasonable than requiring the user to use a lower cluster level which enforces him to give up on other features.