Liron Aravot has posted comments on this change.
Change subject: engine:Validate maximum number of hosts in DC.(#771699)
......................................................................
Patch Set 4: (12 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsCommand.java
Line 54: import org.ovirt.engine.core.utils.transaction.TransactionMethod;
Line 55: import org.ovirt.engine.core.utils.transaction.TransactionSupport;
Line 56:
Line 57: @NonTransactiveCommandAttribute(forceCompensation = true)
Line 58: @LockIdNameAttribute(isWait=true)
Done
Line 59: public class AddVdsCommand<T extends AddVdsActionParameters> extends
VdsCommand<T> {
Line 60:
Line 61: static {
Line 62: CommandParametersInitializer initializer = new
CommandParametersInitializer();
Line 243: @Override
Line 244: public AuditLogType getAuditLogTypeValue() {
Line 245: return getSucceeded() ? AuditLogType.USER_ADD_VDS : errorType;
Line 246: }
Line 247:
same answer as comment in different class, if we use compensation the release
of the lock might be too soon, isn't it?
Line 248: @Override
Line 249: protected Map<String, String> getExclusiveLocks() {
Line 250: return
Collections.singletonMap(getVdsGroup().getstorage_pool_id().toString(),
LockingGroup.POOL_ADD_VDS.name());
Line 251: }
Line 309: returnValue = false;
Line 310: } else if (getVdsGroup().getstorage_pool_id() != null
Line 311: &&
reachedMaxNumOfVdsInPool(getVdsGroup().getstorage_pool_id().getValue())) {
Line 312:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_EXCEEDED_MAXIMUM_NUMBER_OF_HOSTS_IN_DATA_CENTER);
Line 313: returnValue = false;
Done
Line 314: } else {
Line 315: returnValue = returnValue &&
validateSingleHostAttachedToLocalStorage();
Line 316:
Line 317: if (Config.<Boolean>
GetValue(ConfigValues.UseSecureConnectionWithServers)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVDSClusterCommand.java
Line 68:
Line 69: if (getTargetCluster() == null) {
Line 70:
addCanDoActionMessage(VdcBllMessages.VDS_CLUSTER_IS_NOT_VALID);
Line 71: return false;
Line 72: }
Done
Line 73: if (getVdsGroup().getstorage_pool_id() != null &&
!getVdsGroup().getstorage_pool_id().equals(getTargetCluster().getId())
Line 74: &&
reachedMaxNumOfVdsInPool(getTargetCluster().getId().getValue())) {
Line 75:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_EXCEEDED_MAXIMUM_NUMBER_OF_HOSTS_IN_DATA_CENTER);
Line 76: return false;
Line 133:
DbFacade.getInstance().getVdsStaticDAO().update(staticData);
Line 134: getCompensationContext().stateChanged();
Line 135: return null;
Line 136: }
Line 137: });
what if we fail in the VDS commands that occur after this add to DB section?
Line 138:
Line 139: if (getSourceCluster().supportsGlusterService() &&
getClusterUtils().hasServers(getSourceCluster().getId())) {
Line 140: if (!glusterHostRemove(getSourceCluster().getId())) {
Line 141: return;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsCommand.java
Line 190: * @param storagePoolId - The storage pool id
Line 191: * @return True if number of hosts has exceeded, False otherwise.
Line 192: */
Line 193: protected boolean reachedMaxNumOfVdsInPool(Guid storagePoolId) {
Line 194: return (Config.<Integer>
GetValue(ConfigValues.MaxNumberOfHostsInStoragePool).equals(getVdsDAO().GetVdsCountByStoragePoolId(storagePoolId)));
Done
Line 195: }
Line 196:
Line 197: @Override
Line 198: public List<PermissionSubject> getPermissionCheckSubjects() {
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java
Line 148: }
Line 149:
Line 150: @Override
Line 151: public Integer GetVdsCountByStoragePoolId(Guid storagePool) {
Line 152: ParameterizedRowMapper<Integer> mapper = new
ParameterizedRowMapper<Integer>() {
I prefer to not 'export' it to a permanent class and keep the use of anonymous
classes as it's not commonly used, let me know what you think.
Line 153: @Override
Line 154: public Integer mapRow(ResultSet rs, int rowNum) throws
SQLException {
Line 155: Integer result = rs.getInt(1);
Line 156: return result;
Line 150: @Override
Line 151: public Integer GetVdsCountByStoragePoolId(Guid storagePool) {
Line 152: ParameterizedRowMapper<Integer> mapper = new
ParameterizedRowMapper<Integer>() {
Line 153: @Override
Line 154: public Integer mapRow(ResultSet rs, int rowNum) throws
SQLException {
auto-boxing will occur anyway as this method return an Integer, so no reason in
my opinion the declare it as an int.
Line 155: Integer result = rs.getInt(1);
Line 156: return result;
Line 157: }
Line 158: };
Line 156: return result;
Line 157: }
Line 158: };
Line 159:
Line 160: Map<String, Object> resultsMap =
getCallsHandler().executeReadAndReturnMap("GetVdsCountByStoragePoolId",
Done
Line 161: mapper,
Line 162: getCustomMapSqlParameterSource()
Line 163: .addValue("storage_pool_id", storagePool)
Line 164: .addValue("user_id", null)
Line 161: mapper,
Line 162: getCustomMapSqlParameterSource()
Line 163: .addValue("storage_pool_id", storagePool)
Line 164: .addValue("user_id", null)
Line 165: .addValue("is_filtered", false));
I don't agree about that one, this stored procedure is using
getAllForStoragePool sp, which can get those parameters. those are default
values and we might want to make use of them someday (as we have an
implementation of getAllForStoragePool which makes use of them which is called
from this sp, I think that it's better than auto set them in the DB side.
Line 166: return (Integer)((List<?>)resultsMap
Line 167: .get(BaseDAODbFacade.RETURN_VALUE_PARAMETER)).get(0);
Line 168: }
Line 169:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAO.java
Line 162: *
Line 163: * @param storagePool The storage pool's ID
Line 164: * @return vds count in the given storage pool
Line 165: */
Line 166: Integer GetVdsCountByStoragePoolId(Guid storagePool);
Integer is returned from the row mapper (use of Generics), as we already have
an object i prefer to return it so one can take use of the Integer methods. we
know that it cannot be a null as it's a count.
Line 167: /**
Line 168: * Retrieves all VDS instances in the given Storage Pool, that
are in status "UP"
Line 169: * ordered by their vds_spm_priority, not including -1
Line 170: * @return the list of VDS instances
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/VdsDAOTest.java
Line 345: * Asserts that the correct number hosts is returned for a
storage pool with hosts
Line 346: */
Line 347: @Test
Line 348: public void testGetVdsCountByStoragePoolId() {
Line 349: Integer result =
dao.GetVdsCountByStoragePoolId(existingVds.getstorage_pool_id());
it's just a 'sanity' check because its a tester, as we already have an Integer
i prefer to return it the to user of the dao.
Line 350: assertNotNull(result);
Line 351: assertEquals(new
Integer(getAllForStoragePool(existingVds).size()), result);
Line 352: }
Line 353:
--
To view, visit http://gerrit.ovirt.org/6514
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1ef2ce160bc9ee4855b310c9d170ef7c14a0a17
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
