----- Original Message ----- > On 03/20/2013 08:20 PM, Yair Zaslavsky wrote: > > > > ----- Original Message ----- > >> From: "Shireesh Anjal" <san...@redhat.com> > >> To: "Mike Kolesnik" <mkole...@redhat.com> > >> Cc: engine-devel@ovirt.org > >> Sent: Wednesday, March 20, 2013 4:47:08 PM > >> Subject: Re: [Engine-devel] FeatureSupported and compatibility > >> versions > >> > >> On 03/18/2013 01:11 PM, Shireesh Anjal wrote: > >>> On 03/18/2013 12:59 PM, Mike Kolesnik wrote: > >>>> ----- Original Message ----- > >>>>> Hi all, > >>>>> > >>>>> The current mechanism in oVirt to check whether a feature is > >>>>> supported > >>>>> in a particular compatibility version is to use the > >>>>> FeatureSupported > >>>>> class. e.g. > >>>>> > >>>>> FeatureSupported.networkLinking(getVm().getVdsGroupCompatibilityVersion()) > >>>>> > >>>>> > >>>>> Checks whether the "network linking" feature is supported for > >>>>> the > >>>>> the > >>>>> VM's cluster compatibility version. This internally checks > >>>>> whether > >>>>> the > >>>>> value of the corresponding config (NetworkLinkingSupported) for > >>>>> the > >>>>> given compatibility version is true/false. > >>>>> > >>>>> I'm not sure if this is a good idea, since a feature is > >>>>> typically > >>>>> supported "from" a particular version. E.g. Gluster support was > >>>>> introduced in 3.1, and it continues to be available in all > >>>>> subsequent > >>>>> versions. So I see no point in adding configuration for every > >>>>> version > >>>>> indicating whether the feature is supported in that version or > >>>>> not. I > >>>>> suggest to use either of the following options: > >>>> You can "merge" the configs into a single config when older > >>>> versions > >>>> go out of the supported versions for the system. > >>>> > >>>> i.e. in 4.0 you can have upgrade script that merges all > >>>> GlusterFeatureSupported to one entry instead of several. > >> Why are we even storing this information in config? Is this > >> something > >> that can be "configured" at customer site? > > As previously explained (but off list :) ) , Config gives you the > > ability to have a cachable "map" of entry (i.e - "feature name") > > per version and value. > > I guess it was convinient for the developers to use that. > > I also mentioned that customers/oVirt users should config the > > entries of vdc_options using engine-config tool only. > > Not all entries are exposed via engine-config.properties (and no, > > not just "is feature supported" entries are hidden). > > > > > > > > > >>>>> 1) Instead of using a boolean config for each version, use a > >>>>> single > >>>>> string config that indicates the "supported from" version e.g. > >>>>> GlusterSupportedFrom = 3.1. There could be rare cases where a > >>>>> feature, > >>>>> for some reason, is removed in some release. In such cases, we > >>>>> could > >>>>> use > >>>>> one additional config for the "supported to" version. > >>>>> > >>>>> 2) Continue with the boolean approach, but do not have entries > >>>>> for > >>>>> every > >>>>> version; rather make use of the "default value" for majority of > >>>>> cases, > >>>>> and add the explicit version mapping for the minority e.g. > >>>>> GlusterSupported = true by default, and false in case of 3.0 > >>>>> (only > >>>>> one > >>>>> config required for 3.0) > >>>> I'm not sure why we would want to complicate this simple > >>>> mechanism? > >>>> > >>>> Is there much to gain? > >>> I think option 1 suggested above is simpler - to implement as > >>> well > >>> as > >>> to understand. > >>> > >>> Let me give you an example of why I don't like current mechanism. > >>> I > >>> introduce a version check for a feature that was introduced in > >>> 3.1. > >>> I'm being asked now to add three entries in config > >>> > >>> 3.0 - false > >>> 3.1 - true > >>> 3.2 - true > >>> > >>> It will also mean that when 3.3 goes out, someone has to make > >>> sure > >>> that another entry is added for 3.3-true. I think it is not > >>> logical > >>> as > >>> well as scalable if you have more versions. And it sounds far > >>> more > >>> complex (to maintain) than just having > >>> > >>> <Feature>SupportedFrom = 3.1 > >>> > >>> So I would like to know if there are any objections to my > >>> proposal. > >>> I > >>> intend to use this for at least the gluster related features. > > I've sent a patch (http://gerrit.ovirt.org/12970) with following > changes: > > 1) Introduced CompatibilityUtils that provides utility methods for > checking if a given feature is supported in the config. One method to > check based on boolean values (as is being done today for virt > features), and nother to check based on a range (from, to) which I > would > like to use for gluster features. > 2) Renamed FeatureSupported to VirtFeatureSupported, and made it use > the > first utility method from CompatibilityUtils > 3) Introduced GlusterFeatureSupported for gluster features, which > uses > the second utility method from CompatibilityUtils > > Key advantage here is that > - we don't have to touch any virt specifc source for adding > compatibility checks for gluster features > - virt features continue to use the existing boolean config check > > Any comments / suggestions / reviews will be highly appreciated :)
I think splitting to two classes is OK, but the underlying mechanism IMO should be as Omer suggested: Use the default value from the java config file, and if in the DB there is a version specific value then use it for that version only. I don't think "From, To, etc" is a good design, it's not a standard way and is very restrictive. > > >>>>> Thoughts? > >>>>> > >>>>> Regards, > >>>>> Shireesh _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel