Ramesh N has posted comments on this change. Change subject: engine: add feature comptability check for VDS ......................................................................
Patch Set 8: (18 comments) https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetClusterFeaturesByVersionAndAppModeQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetClusterFeaturesByVersionAndAppModeQuery.java: Line 8: GetClusterFeaturesByVersionAndAppModeQuery > GetClusterFeaturesByVersionAndCategoryQuery Done Line 17: getClusterFeaturesForVersionAndAppMode > getClusterFeaturesForVersionAndCategory Done https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HandleVdsVersionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HandleVdsVersionCommand.java: Line 88: > checkClusterAdditionalFeaturesSupported ? Done https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ClusterFeature.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/ClusterFeature.java: Line 10: public > Should this class be split into 2 entities? Split-ed in to two classes. 1. AddtionalClusterFeature which represent the master feature 2. SupportedAddtionalClusterFeature which represents the feature supported by specific cluster. It will have a refrence to the AddtionalClusterFeature. Line 17: > Is this required here? Yes. It make sense in the master list. It helps to identify which feature is relavant for virt and which one is for gluster and which one is applicable for both. https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDS.java: Line 1334: > getAdditionalFeatures? Done https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VDSGroup.java: Line 531: && ObjectUtils.objectsEqual(maintenanceReasonRequired, other.maintenanceReasonRequired) > Duplicate Done https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java: Line 166: private > supportedAdditionalFeatures - these are features in addition to the cluster I can change the comment. Do u want me to change the variable name also to reflect the correct meaning? https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetClusterFeaturesByVersionAndAppModeParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetClusterFeaturesByVersionAndAppModeParameters.java: Line 7: GetClusterFeaturesByVersionAndAppModeParameters > GetClusterFeaturesByVersionAndCategoryParameters Done https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ClusterFeatureDaoImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ClusterFeatureDaoImpl.java: Line 133: getSupportedHostFeaturesByVdsId > This should go to VdsDynamic Done https://gerrit.ovirt.org/#/c/39756/8/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDynamicDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDynamicDAODbFacadeImpl.java: Line 266: .addValue("supported_rng_sources", VmRngDevice.sourcesToCsv(vds.getSupportedRngSources())) Line 267: .addValue("supported_emulated_machines", vds.getSupportedEmulatedMachines()) Line 268: .addValue("is_live_snapshot_supported", vds.getLiveSnapshotSupport()) Line 269: .addValue("is_live_merge_supported", vds.getLiveMergeSupport()) Line 270: .addValue("maintenance_reason", vds.getMaintenanceReason()); > Exclude this file Done Line 271: return parameterSource; Line 272: } Line 273: Line 274: @Override https://gerrit.ovirt.org/#/c/39756/8/packaging/dbscripts/cluster_features_sp.sql File packaging/dbscripts/cluster_features_sp.sql: Line 1: /* ---------------------------------------------------------------- Line 2: Stored procedures for database operations on Cluster Features Line 3: related tables: cluster_features, supported_cluster_features, supported_host_features Line 4: ----------------------------------------------------------------*/ Line 5: Create or replace function InsertClusterFeature(v_feature_id UUID, > General : Done Line 6: v_feature VARCHAR(256), Line 7: v_version VARCHAR(40), Line 8: v_app_mode INTEGER, Line 9: v_description TEXT) Line 18: Create > Are these 2 procs used from engine - Insert/UpdateClusterFeature ? No.. Insert is used in the sql script file. Update is not used yet. May be needed in future. Line 50: -- than ZERO (0) and applicable set of roles are identified. Line 51: -- Line 52: -- 1 & 2 (0000 0001 & 0000 0010) = 0000 0000 = 0 Roles with this app_mode would NOT be listed Line 53: -- 2 & 2 (0000 0010 & 0000 0010) = 0000 0010 = 2 > 0 Roles with this app_mode would be listed Line 54: -- 255 & 2 (1111 1111 & 0000 0010) = 0000 0010 = 2 > 0 Roles with this app_mode would be listed > please add a newline here Done Line 55: Create or replace FUNCTION GetClusterFeaturesByVersionAndAppMode(v_version VARCHAR(256), v_app_mode INTEGER) Line 56: RETURNS SETOF supported_host_features STABLE Line 57: AS $procedure$ Line 58: BEGIN https://gerrit.ovirt.org/#/c/39756/8/packaging/dbscripts/upgrade/03_06_1290_add_cluster_features_table.sql File packaging/dbscripts/upgrade/03_06_1290_add_cluster_features_table.sql: Line 1: -- ---------------------------------------------------------------------- > General : TABs => spaces Sorry for the trouble. Some how this tabs escaped my eyes as I can't see them in git diff. Line 2: -- tables cluster_features, supported_cluster_features and supported_host_features Line 3: -- Maintains the list of features which are supported by Engine. This is on top of the Line 4: -- standard cluster compatibility version check. Line 5: -- ---------------------------------------------------------------------- Line 10: -- 1. VirtOnly 0000 0001 : -- 2. GlusterOnly 0000 0010 : -- 3. AllModes 1111 1111 > -- 1. Virt 0000 0001 Done Line 18: app_mode > you could call this as 'category' Done Line 35: CREATE UNIQUE INDEX IDX_supported_cluster_features ON supported_cluster_features(cluster_id, feature_id); Line 36: Line 37: CREATE TABLE supported_host_features Line 38: ( Line 39: vds_id UUID NOT NULL, > isn't it more right to use the term host_id instead of the old vds ??? (the I will change this to host_id Line 40: feature VARCHAR(256) NOT NULL, Line 41: CONSTRAINT PK_supported_host_features PRIMARY KEY (vds_id, feature), Line 42: FOREIGN KEY (vds_id) REFERENCES vds_static(vds_id) ON DELETE CASCADE Line 43: ) ; -- To view, visit https://gerrit.ovirt.org/39756 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icba02b189a169bc676e0c5f47f7aaf394f0b49a6 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
