Shireesh Anjal has posted comments on this change.
Change subject: gluster: added cluster level check
......................................................................
Patch Set 6: (2 inline comments)
Response to Yair's comments in-line. Regarding Omer's comments,
1. Config sql is not required, as the "deafult" value defined in ConfigValues
takes care of it. We should insert a value in DB only if the value needs to be
different from the default value.
2. Started discussion on engine-devel regarding the way version check is done
in oVirt. I can't understand why we need a config for "every" version when a
feature is introduced in a given version.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java
Line 165: result = validateMetrics();
Line 166: }
Line 167:
Line 168: if (getVdsGroup().supportsGlusterService()
Line 169: &&
!GlusterFeatureSupported.gluster(getVdsGroup().getcompatibility_version())) {
I would call FeatureSupported as an infrastructure class if it provides some
functionality that I can reuse. It doesn't. What it does is provide a
"standard" way of implementing the version check for features - a convention to
be followed.
GlusterFeatureSupported does exactly the same thing, at the same time ensuring
that it is a separate source in a separate package, so that it will be easy to
extract it out into a separate jar/module/project at a later point of time.
When we started out adding gluster functionality in oVirt, the idea was to
reuse the existing infrastructure, but also try to keep gluster code as
separate as possible. This is an interim approach till the time oVirt evolves
into a proper "pluggable" framework, which it currently is not.
So I see absolutely no harm in implementing GlusterFeatureSupported as a
separate class, with a slightly different (and better, IMHO) approach for
checking feature support against compatibility version.
Line 170:
addCanDoActionMessage(VdcBllMessages.GLUSTER_NOT_SUPPORTED);
Line 171:
addCanDoActionMessage(String.format("$compatibilityVersion %1$s",
getVdsGroup().getcompatibility_version().getValue()));
Line 172: result = false;
Line 173: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/gluster/GlusterFeatureSupported.java
Line 7: /**
Line 8: * Convenience class to check if a gluster feature is supported or not
in any given version.<br>
Line 9: * Methods should be named by feature and accept version to check
against.
Line 10: */
Line 11: public class GlusterFeatureSupported {
Please refer to my response to your comment on the source
AddVdsGroupCommand.java
Line 12: /**
Line 13: * Checks if the given version is >= the minimum version
requirement for the feature
Line 14: *
Line 15: * @param featureSupportedFrom
--
To view, visit http://gerrit.ovirt.org/12970
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3944e8b1d8d82eb19643b99f606da2364d6b582
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Sahina Bose <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
