11:09 a.m.
------=_Part_21094117_1742490232.1385888972528
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
----- Original Message -----
From: "Mike Kolesnik" <mkolesni(a)redhat.com>
To: "Omer Frenkel" <ofrenkel(a)redhat.com>
Cc: "Kanagaraj" <kmayilsa(a)redhat.com>, "engine-devel"
<engine-devel(a)ovirt.org>
Sent: Sunday, December 1, 2013 11:01:50 AM
Subject: Re: [Engine-devel] Using config values
----- Original Message -----
> ----- Original Message -----
>
> > From: "Mike Kolesnik"
<mkolesni(a)redhat.com>
>
> > To: "Kanagaraj" <kmayilsa(a)redhat.com>
>
> > Cc: "engine-devel" <engine-devel(a)ovirt.org>
>
> > Sent: Sunday, December 1, 2013 8:08:42 AM
>
> > Subject: Re: [Engine-devel] Using config values
>
> > ----- Original Message -----
>
>
>
> > > 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
> > Regards,
>
> > Mike
>
> > > Please share your thoughts on this.
> >
>
> > > Thanks,
> >
>
> > > Kanagaraj
> >
>
> > _______________________________________________
>
> > Engine-devel mailing list
>
> > Engine-devel(a)ovirt.org
>
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
>
------=_Part_21094117_1742490232.1385888972528
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable
<html><body><div style=3D"font-family: times new roman, new york,
times, se=
rif; font-size: 12pt; color:
#000000"><div><br></div><div><br></div><hr
id=
=3D"zwchr"><blockquote style=3D"border-left:2px solid
#1010FF;margin-left:5=
px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-de=
coration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><b>Fr=
om: </b>"Mike Kolesnik"
&lt;mkolesni(a)redhat.com&gt;<br><b>To: </b>"Omer Fre=
nkel" &lt;ofrenkel(a)redhat.com&gt;<br><b>Cc:
</b>"Kanagaraj" <kmayilsa@re=
dhat.com>, "engine-devel"
&lt;engine-devel(a)ovirt.org&gt;<br><b>Sent: </b=
Sunday, December 1, 2013 11:01:50 AM<br><b>Subject:
</b>Re: [Engine-devel]=
Using config
values<br><div><br></div><div style=3D"font-family: times
new=
roman, new york, times, serif; font-size: 12pt; color: #000000"><hr
id=3D"=
zwchr"><blockquote style=3D"border-left:2px solid
#1010FF;margin-left:5px;p=
adding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decora=
tion:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div styl=
e=3D"font-family: times new roman, new york, times, serif; font-size: 12pt;=
color:
#000000"><div><br></div><div><br></div><hr
id=3D"zwchr"><blockquote=
style=3D"border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;co=
lor:#000;font-weight:normal;font-style:normal;text-decoration:none;font-fam=
ily:Helvetica,Arial,sans-serif;font-size:12pt;"><b>From:
</b>"Mike Kolesnik=
" &lt;mkolesni(a)redhat.com&gt;<br><b>To:
</b>"Kanagaraj" <kmayilsa@redhat=
.com><br><b>Cc: </b>"engine-devel"
&lt;engine-devel(a)ovirt.org&gt;<br><b>=
Sent: </b>Sunday, December 1, 2013 8:08:42 AM<br><b>Subject:
</b>Re: [Engin=
e-devel] Using config values<br><div><br></div><div
style=3D"font-family: t=
imes new roman, new york, times, serif; font-size: 12pt; color: #000000"><h=
r id=3D"zwchr"><blockquote style=3D"border-left:2px solid
#1010FF;margin-le=
ft:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;tex=
t-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;">H=
i All,</blockquote><div><br></div><div>Hi
Kanagaraj,<br></div><div><br></di=
v><blockquote style=3D"border-left:2px solid #1010FF;margin-left:5px;paddin=
g-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:=
none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><br> The are
s=
ome issues arising in configurations whenever we move up on the versions(3.=
3 =3D> 3.4), because of the way we store and interpret
them.<br><div><br=
</div>Whenever there is a new cluster level, you will need to
add a new en=
try 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.<br> Better opt=
ion would be to have the defaul config value in ConfigValues.java and the o=
verrides will go to config.sql. In this approach you don't need a new entri=
es to config.sql when there is a new cluster
level.<br><div><br></div>Lets =
take an exmaple, "SupportForceCreateVG" - This is supported from 3.1 onward=
s,<br><div><br></div>If you look at config.sql, you will see
following entr=
ies <br> select
fn_db_add_config_value('SupportForceCreateVG','false','3.0'=
); <br> select
fn_db_add_config_value('SupportForceCreateVG','true','3.1');=
<br> select
fn_db_add_config_value('SupportForceCreateVG','true','3.2');
<=
br> select
fn_db_add_config_value('SupportForceCreateVG','true','3.3');<br>=
<div><br></div>And in
ConfigValues.java<br><div><br></div>  =
; @TypeConverterAttribute(Boolean.class)<br>
@DefaultVal=
ueAttribute("false")<br>
SupportForceCreateVG,<br><div><=
br></div>Now if there is 3.4 and 3.5, the user needs to add 2 more entries,=
which i feel is redundant.<br><div><br></div>Instead we can make
<br><div>=
<br></div>
@TypeConverterAttribute(Boolean.class)<br> &nb=
sp; @DefaultValueAttribute("true")<br>
Suppo=
rtForceCreateVG,<br><div><br></div>and have only the following in
config.sq=
l<br> select
fn_db_add_config_value('SupportForceCreateVG','false','3.0');<=
br><div><br></div>if a particular value(for a specific cluster level)
is no=
t found in Config.sql, the fallback is to use the value available in Config=
Values.java.</blockquote><div><br></div><div>This has
already been implemen=
ted, there are many "feature supported" configurations working like this (f=
or example
GlusterSupport).<br></div><div><br></div><div>I think
a more int=
eresting approach would be to move these out of the DB since these values d=
on't really hav e a reson to be there.<br></div><div>Since the
entire thing=
is abstracted by "Gluster/FeatureSupported" classes then we can easily cha=
nge mechanism (of course whatever code is not using it can be easily conver=
ted to use the
mechanism)<br></div><div><br></div><div>For example a
simple=
enum could do the
trick:<br></div><div>-----------------------------------=
-- EXAMPLE
-------------------------------------<br></div><div>/**<br> =
;* Convenience class to check if a gluster feature is supported or not in a=
ny given version.<br><br> * Methods should be named by
feature a=
nd accept version to check against.<br> */<br>public class
GlusterFeat=
ureSupported {</div><div>
/**<br> =
* @param version<br>
* &nbs=
p; Compatibility version to
check for.<=
br> * @return
<code>true</code> if glus=
ter support is enabled, <code>false</code> if it's
not.<br>&nbs=
p; */<br> public static
boolean gluster=
(Version version)
{<br> return
Su=
pportedFeatures.GLUSTER.isSupportedOn(version);<br>
}<br>=
<div><br></div>
/**<br> * @param =
version<br>
* &n=
bsp; Compatibility version to check
for.<br> &=
nbsp; * @return <code>true</code> if
gluster heavyw=
eight refresh is enabled, <code>false</code> if it's
not.<br>&n=
bsp; */<br> public
static boolean refre=
shHeavyWeight(Version version)
{<br> &nb=
sp; return SupportedFeatures.REFRESH_HEAVYWEIGHT.isSupportedOn(version);<br=
}<br></div><div><br></div><div>
/* Mo=
re methods...
*/</div><div><br> enum SupportedFeatures
{<=
br>
GLUSTER(Version.v3_0),<br>&nb=
sp;
REFRESH_HEAVYWEIGHT(Version.v3_0, V=
ersion.v3_1),</div><div>
/* More =
members
*/;<br><div><br></div>
pr=
ivate Set<Version> unsupportedVersions =3D new
HashSet<Version>=
();<br><div><br></div>
private Su=
pportedFeatures(Version... versions)
{<br> &nb=
sp;
unsupportedVersions.addAll(Arrays.asList(=
versions));<br>
}<br><div><br></d=
iv> public
boolean isSupportedOn(=
Version version)
{<br>  =
; return
!unsupportedVersions.contains(version);<br>  =
;
}<br> }</div><div>-------=
------------------------------ END EXAMPLE --------------------------------=
-----</div><div><br></div><div>Thoughts?</div></div></blockquote><div><br>u=
nless i didn't understand something, this is not
good,<br></div><div>this s=
hould stay configurable by the users,<br></div><div>for example if some
use=
r experience some issues with a feature and want to turn it off/change the =
values..</div><div>(not all version configuration are boolean, some are dif=
ferent values to different versions, like
cpu-list)</div></div></blockquote=
<div><br></div><div>This is for API level
compatibility.<br></div><div>If =
VDSM doesn't support for
example hot plug in 3.1 then the user can't just d=
ecide that it does and change it.<br></div><div>Also, this is not
changeabl=
e by user since it's not exposed by engine-config (nor should it be).<br></=
div></div></blockquote><div>some are
exposed<br></div><blockquote style=3D"=
border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;f=
ont-weight:normal;font-style:normal;text-decoration:none;font-family:Helvet=
ica,Arial,sans-serif;font-size:12pt;"><div style=3D"font-family: times new
=
roman, new york, times, serif; font-size: 12pt; color: #000000"><div>This
i=
s strictly for the engine-VDSM API compatibility, not for other configs whi=
ch are version
specific.<br></div></div></blockquote><div>right, but still
=
user should be able to turn features off in case of
problems,<br></div><div=
or change in some cases (for example it is possible to add support for
mor=
e power management devices, i know it was done by
users)<br></div><div>no r=
eason to block this<br></div><blockquote style=3D"border-left:2px
solid #10=
10FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-st=
yle:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font=
-size:12pt;"><div style=3D"font-family: times new roman, new york, times,
s=
erif; font-size: 12pt; color: #000000"><blockquote
style=3D"border-left:2px=
solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:norm=
al;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-=
serif;font-size:12pt;"><div style=3D"font-family: times new roman, new
york=
, times, serif; font-size: 12pt; color:
#000000"><div><br></div><blockquote=
style=3D"border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;co=
lor:#000;font-weight:normal;font-style:normal;text-decoration:none;font-fam=
ily:Helvetica,Arial,sans-serif;font-size:12pt;"><div style=3D"font-family:
=
times new roman, new york, times, serif; font-size: 12pt; color: #000000"><=
div><br></div><div><br></div><div>Regards,<br></div><div>Mike<br></div><blo=
ckquote style=3D"border-left:2px solid #1010FF;margin-left:5px;padding-left=
:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;f=
ont-family:Helvetica,Arial,sans-serif;font-size:12pt;"><br> Please share
yo=
ur thoughts on this.<br><div><br></div>Thanks,<br>
Kanagaraj<br><div><br></=
div></blockquote></div><br>_______________________________________________<=
br>Engine-devel mailing
list<br>Engine-devel@ovirt.org<br>http://lists.ovir=
t.org/mailman/listinfo/engine-devel<br></blockquote><div><br></div></div></=
blockquote><div><br></div></div></blockquote><div><br></div></div></body></=
html>
------=_Part_21094117_1742490232.1385888972528--