Ramesh N has posted comments on this change. Change subject: <WIP>engine: add feature comptability check for VDS ......................................................................
Patch Set 6: (16 comments) Patch set to follow. https://gerrit.ovirt.org/#/c/39756/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsGroupCommand.java: Line 53: } Line 54: Line 55: @Override Line 56: protected void executeCommand() { Line 57: VDSGroup vdsGroup = getVdsGroup(); > s/vdsGroup/cluster Done Line 58: vdsGroup.setArchitecture(getArchitecture()); Line 59: Line 60: checkMaxMemoryOverCommitValue(); Line 61: vdsGroup.setDetectEmulatedMachine(true); Line 73: getCpuProfileDao().save(CpuProfileHelper.createCpuProfile(getParameters().getVdsGroup().getId(), Line 74: getParameters().getVdsGroup().getName())); Line 75: } Line 76: Line 77: if (vdsGroup.getFeaturesSupported() != null && !vdsGroup.getFeaturesSupported().isEmpty()) { > is those 2 are repeated called together, please add another method to VdsGr Replaced this with CollectionUtils.isNotEmpty(cluster.getFeaturesSupported() Line 78: for(ClusterFeature feature:vdsGroup.getFeaturesSupported()){ Line 79: feature.setVdsGroupId(vdsGroup.getId()); Line 80: } Line 81: getClusterFeatureDao().saveAll(vdsGroup.getFeaturesSupported()); Line 74: getParameters().getVdsGroup().getName())); Line 75: } Line 76: Line 77: if (vdsGroup.getFeaturesSupported() != null && !vdsGroup.getFeaturesSupported().isEmpty()) { Line 78: for(ClusterFeature feature:vdsGroup.getFeaturesSupported()){ > please format the code. the eclipse formatter suggests more spaces in that Done Line 79: feature.setVdsGroupId(vdsGroup.getId()); Line 80: } Line 81: getClusterFeatureDao().saveAll(vdsGroup.getFeaturesSupported()); Line 82: } Line 77: if (vdsGroup.getFeaturesSupported() != null && !vdsGroup.getFeaturesSupported().isEmpty()) { Line 78: for(ClusterFeature feature:vdsGroup.getFeaturesSupported()){ Line 79: feature.setVdsGroupId(vdsGroup.getId()); Line 80: } Line 81: getClusterFeatureDao().saveAll(vdsGroup.getFeaturesSupported()); > please use injection instead. Done Line 82: } Line 83: Line 84: setActionReturnValue(vdsGroup.getId()); Line 85: setSucceeded(true); Line 78: for(ClusterFeature feature:vdsGroup.getFeaturesSupported()){ Line 79: feature.setVdsGroupId(vdsGroup.getId()); Line 80: } Line 81: getClusterFeatureDao().saveAll(vdsGroup.getFeaturesSupported()); Line 82: } > i wasn't aware of a requirement to allow the user to specify the desired su I changed the logic. Now feature list will be retrieved from DB using GetClusterFeaturesByVersionAndAppModeQuery and will shown to the user. User can select the ones he wants. Selected one will be added to supported_cluster_features table. Line 83: Line 84: setActionReturnValue(vdsGroup.getId()); Line 85: setSucceeded(true); Line 86: } https://gerrit.ovirt.org/#/c/39756/6/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 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import java.io.Serializable; Line 4: Line 5: import org.ovirt.engine.core.common.utils.ObjectUtils; > ObjectUtils.objectsEqual is redundant. you should use java.util.Objects.equ Done Line 6: import org.ovirt.engine.core.compat.Guid; Line 7: Line 8: public class ClusterFeature implements Serializable { Line 9: Line 9: Line 10: private static final long serialVersionUID = -8387930346405670858L; Line 11: private String feature; Line 12: private boolean enabled;; Line 13: private Guid vdsGroupId; > s/vdsGroupId/clusterId and same for the getter/setter. Done Line 14: Line 15: public ClusterFeature() { Line 16: } Line 17: Line 15: public ClusterFeature() { Line 16: } Line 17: Line 18: public ClusterFeature(String feature, boolean enabled, Guid vdsGroupId) { Line 19: super(); > the call to super is redundant. Done Line 20: this.feature = feature; Line 21: this.enabled = enabled; Line 22: this.vdsGroupId = vdsGroupId; Line 23: } Line 65: return true; Line 66: } Line 67: } Line 68: return false; Line 69: } > please implement toString using ToStringBuilder which simplifies debug. Done https://gerrit.ovirt.org/#/c/39756/6/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 86: private Guid clusterPolicyId; Line 87: Line 88: private String clusterPolicyName; Line 89: Line 90: private List<ClusterFeature> featuresSupported; > Set would be more appropriate Done Line 91: Line 92: @ValidUri(message = "VALIDATION.VDS_GROUP.SPICE_PROXY.HOSTNAME_OR_IP", Line 93: groups = { CreateEntity.class, UpdateEntity.class }) Line 94: @Size(max = BusinessEntitiesDefinitions.SPICE_PROXY_ADDR_SIZE) https://gerrit.ovirt.org/#/c/39756/6/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 162: Line 163: private String maintenanceReason; Line 164: Line 165: // Comma separated values of all the features supported Line 166: private String supportedFeatures; > why not Set as well ? Done Line 167: Line 168: public VdsDynamic() { Line 169: rpmVersion = new RpmVersion(); Line 170: libvirtVersion = new RpmVersion(); Line 162: Line 163: private String maintenanceReason; Line 164: Line 165: // Comma separated values of all the features supported Line 166: private String supportedFeatures; > why not Set as well ? Done Line 167: Line 168: public VdsDynamic() { Line 169: rpmVersion = new RpmVersion(); Line 170: libvirtVersion = new RpmVersion(); https://gerrit.ovirt.org/#/c/39756/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ClusterFeatureDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ClusterFeatureDao.java: Line 20: * Line 21: * @param vdsGroupId Line 22: * @return Line 23: */ Line 24: public List<ClusterFeature> getByVdsGroupId(Guid vdsGroupId); > s/getByVdsGroupId/getByClusterId Done Line 25: Line 26: /** Line 27: * Update the ClusterFeature. Line 28: * https://gerrit.ovirt.org/#/c/39756/6/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 20: private static final RowMapper<ClusterFeature> clusterFeatureRowMapper = new ClusterFeatureRowMapper(); Line 21: Line 22: private static final class ClusterFeatureRowMapper implements RowMapper<ClusterFeature> { Line 23: @Override Line 24: public ClusterFeature mapRow(ResultSet rs, int rowNum) > please merge lines Done Line 25: throws SQLException { Line 26: ClusterFeature feature = new ClusterFeature(); Line 27: feature.setVdsGroupId(getGuidDefaultEmpty(rs, "vds_group_id")); Line 28: feature.setEnabled(rs.getBoolean("is_enabled")); https://gerrit.ovirt.org/#/c/39756/6/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 table: cluster_features Line 4: ----------------------------------------------------------------*/ Line 5: Create or replace function InsertClusterFeature(v_vds_group_id UUID, > Please remove 4 spaces alignment instead of tabs Done Line 6: v_feature VARCHAR(256), Line 7: v_is_enabled BOOLEAN) Line 8: RETURNS VOID Line 9: AS $procedure$ https://gerrit.ovirt.org/#/c/39756/6/packaging/dbscripts/upgrade/03_06_1250_add_cluster_features_table.sql File packaging/dbscripts/upgrade/03_06_1250_add_cluster_features_table.sql: Line 4: -- ---------------------------------------------------------------------- Line 5: Line 6: CREATE TABLE cluster_features Line 7: ( Line 8: vds_group_id UUID NOT NULL, > s/vds_group_id/cluster_id Done Line 9: feature varchar(256) NOT NULL, Line 10: is_enabled BOOLEAN, Line 11: CONSTRAINT PK_cluster_features PRIMARY KEY (vds_group_id, feature), Line 12: FOREIGN KEY (vds_group_id) REFERENCES vds_groups(vds_group_id) ON DELETE CASCADE -- 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: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> 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-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
