<html><body><div style="font-family: times new roman, new york, times, serif; font-size: 12pt; color: #000000"><div><br></div><div><br></div><hr id="zwchr"><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><b>From: </b>"Mike Kolesnik" &lt;mkolesni@redhat.com&gt;<br><b>To: </b>"Kanagaraj" &lt;kmayilsa@redhat.com&gt;<br><b>Cc: </b>"engine-devel" &lt;engine-devel@ovirt.org&gt;<br><b>Sent: </b>Sunday, December 1, 2013 8:08:42 AM<br><b>Subject: </b>Re: [Engine-devel] Using config values<br><div><br></div><div style="font-family: times new roman, new york, times, serif; font-size: 12pt; color: #000000"><hr id="zwchr"><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;">
  

    
  
  
    Hi All,</blockquote><div><br></div><div>Hi Kanagaraj,<br></div><div><br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-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 some issues arising in configurations whenever we move up on
    the versions(3.3 =&gt; 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
    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.<br>
    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.<br><div><br></div>
    Lets take an exmaple, "SupportForceCreateVG" - This is supported
    from 3.1 onwards,<br><div><br></div>
    If you look at config.sql, you will see following entries <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>
    &nbsp;&nbsp;&nbsp; @TypeConverterAttribute(Boolean.class)<br>
    &nbsp;&nbsp;&nbsp; @DefaultValueAttribute("false")<br>
    &nbsp;&nbsp;&nbsp; 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>
    &nbsp;&nbsp;&nbsp; @TypeConverterAttribute(Boolean.class)<br>
    &nbsp;&nbsp;&nbsp; @DefaultValueAttribute("true")<br>
    &nbsp;&nbsp;&nbsp; SupportForceCreateVG,<br><div><br></div>
    and have only the following in config.sql<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 not found in
    Config.sql, the fallback is to use the value available in
    ConfigValues.java.</blockquote><div><br></div><div>This has already been implemented, there are many "feature supported" configurations working like this (for example GlusterSupport).<br></div><div><br></div><div>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.<br></div><div>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)<br></div><div><br></div><div>For example a simple enum could do the trick:<br></div><div>------------------------------------- EXAMPLE -------------------------------------<br></div><div>/**<br>&nbsp;* Convenience class to check if a gluster feature is supported or not in any given version.&lt;br&gt;<br>&nbsp;* Methods should be named by feature and accept version to check against.<br>&nbsp;*/<br>public class GlusterFeatureSupported {</div><div>&nbsp;&nbsp;&nbsp; /**<br>&nbsp;&nbsp;&nbsp;&nbsp; * @param version<br>&nbsp;&nbsp;&nbsp;&nbsp; *&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; Compatibility version to check for.<br>&nbsp;&nbsp;&nbsp;&nbsp; * @return &lt;code&gt;true&lt;/code&gt; if gluster support is enabled, &lt;code&gt;false&lt;/code&gt; if it's not.<br>&nbsp;&nbsp;&nbsp;&nbsp; */<br>&nbsp;&nbsp;&nbsp; public static boolean gluster(Version version) {<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; return SupportedFeatures.GLUSTER.isSupportedOn(version);<br>&nbsp;&nbsp;&nbsp; }<br><div><br></div>&nbsp;&nbsp;&nbsp; /**<br>&nbsp;&nbsp;&nbsp;&nbsp; * @param version<br>&nbsp;&nbsp;&nbsp;&nbsp; *&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; Compatibility version to check for.<br>&nbsp;&nbsp;&nbsp;&nbsp; * @return &lt;code&gt;true&lt;/code&gt; if gluster heavyweight refresh is enabled, &lt;code&gt;false&lt;/code&gt; if it's not.<br>&nbsp;&nbsp;&nbsp;&nbsp; */<br>&nbsp;&nbsp;&nbsp; public static boolean refreshHeavyWeight(Version version) {<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; return SupportedFeatures.REFRESH_HEAVYWEIGHT.isSupportedOn(version);<br>&nbsp;&nbsp;&nbsp; }<br></div><div><br></div><div>&nbsp;&nbsp;&nbsp; /* More methods... */</div><div><br>&nbsp;&nbsp;&nbsp; enum SupportedFeatures {<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; GLUSTER(Version.v3_0),<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; REFRESH_HEAVYWEIGHT(Version.v3_0, Version.v3_1),</div><div>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; /* More members */;<br><div><br></div>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; private Set&lt;Version&gt; unsupportedVersions = new HashSet&lt;Version&gt;();<br><div><br></div>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; private SupportedFeatures(Version... versions) {<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; unsupportedVersions.addAll(Arrays.asList(versions));<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; }<br><div><br></div>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; public boolean isSupportedOn(Version version) {<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; return !unsupportedVersions.contains(version);<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; }<br>&nbsp;&nbsp;&nbsp; }</div><div>------------------------------------- END EXAMPLE -------------------------------------</div><div><br></div><div>Thoughts?</div></div></blockquote><div><br>unless i didn't understand something, this is not good,<br></div><div>this should stay configurable by the users,<br></div><div>for example if some user 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 different values to different versions, like cpu-list)<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div style="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><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><br>
    Please share your 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.ovirt.org/mailman/listinfo/engine-devel<br></blockquote><div><br></div></div></body></html>