Ayal Baron has posted comments on this change.
Change subject: Refactor UpdateVdsGroupCommand
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(22 inline comments)
As in the other patch, only substantial thing is the difference in behaviour
between the previous aggregation of canDoAction messages and current single
validation which returns immediately upon failure.
Note that I was not able to read the old code to make sure that functionality
is the same.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java
Line 161: return false;
Again, idea is to aggregate all validations to be able to tell user everything
that needs to be fixed.
Line 191: getVdsGroup().getstorage_pool_id().getValue());
s/getVdsGroup()/vdsGroup/ ?
Line 217: VDSGroup vdsGroup = getVdsGroup();
why is this not passed as param to the function?
Line 218: NGuid storagePoolId = vdsGroup.getstorage_pool_id();
again, storagePool could be passed as param (it's used in the calling mehod)
Line 219: List<VDSGroup> groupsForPool =
groupDao.getAllForStoragePool(storagePoolId.getValue());
getAllWhat? grrrr
Line 224:
.add(VdcBllMessages.VDS_GROUP_CANNOT_ADD_MORE_THEN_ONE_HOST_TO_LOCAL_STORAGE
s/THEN/THAN/? (different patch, nm)
Line 233:
.add(VdcBllMessages.VDS_GROUP_SELECTION_ALGORITHM_MUST_BE_SET_TO_NONE_ON_LOCAL_STORAGE
Again, not this patch, but why is it none and not just always returns the local
host in local storage?
what if using the new functionality I mark it with -1 priority for SPM (because
I deliberately do not want it to be SPM once I setup a few VMs because I don't
want it to eat up my resources), how would that be handled without a selection
algorithm?
Line 246: private boolean validateMemberVmsState(VDSGroup oldGroup) {
the name doesn't really explain what the function does and reading it makes it
clear that this is because it does multiple things.
Maybe split into 3?
getVMsStates
validateCompatVersion
validateCpuProperties
or something?
Line 247: SearchParameters searchParams = new SearchParameters("vms:
cluster = "
if search type is VM then why do we need "vms: " prefix hard coded here?
In addition, why isn't the prefix in general a constant taken from some place
else? (not to mention just have the param type passed and not concatenate
strings like this which is error prone)
Line 258: // the vm cluster id is the same as the cluster.id
So why is the search by cluster name and not by cluster id?
Line 310: // cannot not be hibernated
cannot not?
Line 322: if (oldGroup.getstorage_pool_id() == null) {
what's the convention about using underscores? between every 2 words except for
the first 2? according to number of characters? (yes I know the function name
predates this patch)
Line 330:
addCanDoActionMessage(VdcBllMessages.VDS_GROUP_CANNOT_CHANGE_STORAGE_POOL);
how does this message relate to the validation?
Line 341: .runInternalQuery(VdcQueryType.Search,
params).getReturnValue();
Vdc? didn't we get rid of this?
Line 343: ArrayList<String> actionMessages =
getReturnValue().getCanDoActionMessages();
Why isn't this passed as parameter? (all over the place). Defining
actionMessages again and again seems redundant
Line 352:
VdcBllMessages.VDS_GROUP_CANNOT_UPDATE_COMPATIBILITY_VERSION_WITH_LOWER_HOSTS
I'm sure there is a good reason why this function is called
validateCpuFlagsChange yet we are validating cluster version support but it is
not clear (to me) from reading the code.
Line 366: private boolean validateCompatibilityVersionChange(VDSGroup
oldGroup) {
why is it 'oldGroup' and not 'currGroup'?
Line 374: Version oldCompatVersion =
oldGroup.getcompatibility_version();
same as above, why is it 'old' and not 'curr'?
Line 376: if (compatVersion.compareTo(oldCompatVersion) < 0) {
what's wrong with compatVersion < oldCompatVersion?
Line 406: // There is no old cpu so all new values are valid
%s/old/curr/g ???
Line 412: int vdsNum =
DbFacade.getInstance().getVdsStaticDAO().getAllForVdsGroup(getVdsGroup().getId())
s/getVdsGroup/vdsGroup
Line 415:
addCanDoActionMessage(VdcBllMessages.VDS_GROUP_CANNOT_UPDATE_CPU_ILLEGAL);
ILLEGAL what?
--
To view, visit http://gerrit.ovirt.org/2271
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I47300fec2f6f5fabdbc75e8c78332c5aef2508a2
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Haim Ateya <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
