Roy Golan has posted comments on this change.
Change subject: core, engine: New validations for host entrance in clusters
......................................................................
Patch Set 1:
(5 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HandleVdsCpuFlagsOrClusterChangedCommand.java
Line 22: @NonTransactiveCommandAttribute
Line 23: public class HandleVdsCpuFlagsOrClusterChangedCommand<T extends
VdsActionParameters> extends VdsCommand<T> {
Line 24:
Line 25: private boolean _hasFlags = true;
Line 26: private boolean _architectureMismatch = false;
1. I prefer positive checks. more intuitive and less effort to grasp
architectureMatch instead of the negative form.
2. please don't use _ prefix for private members.
_hasFlag should also be changed(not for this patch)
Line 27: private boolean foundCPU = true;
Line 28:
Line 29: public HandleVdsCpuFlagsOrClusterChangedCommand(T parameters) {
Line 30: super(parameters);
Line 61: .getName(), getVds().getCpuFlags());
Line 62: }
Line 63:
Line 64: // Checks whether the host and the cluster have the same
architecture
Line 65: if (_hasFlags && foundCPU &&
!grp.getArchitecture().equals(ArchitectureType.undefined) &&
use != Architecture.undefined.
the default in the DB is anyway 0 so we should hit NPE never
Line 66: !sc.getArchitecture().equals(grp.getArchitecture())) {
Line 67: _architectureMismatch = true;
Line 68:
Line 69: addCustomValue("VdsArchitecture",
sc.getArchitecture().name());
Line 62: }
Line 63:
Line 64: // Checks whether the host and the cluster have the same
architecture
Line 65: if (_hasFlags && foundCPU &&
!grp.getArchitecture().equals(ArchitectureType.undefined) &&
Line 66: !sc.getArchitecture().equals(grp.getArchitecture())) {
ServerCpu also shouldn't be null so != is safe to use. infact if it would be
null I want this to fail to we would know something is configured wrong
Line 67: _architectureMismatch = true;
Line 68:
Line 69: addCustomValue("VdsArchitecture",
sc.getArchitecture().name());
Line 70: addCustomValue("VdsGroupArchitecture",
grp.getArchitecture().name());
Line 78: ExecutionHandler.createInternalJobContext());
Line 79: }
Line 80:
Line 81: // if cluster doesn't have cpu then get the cpu from the vds
Line 82: if (_hasFlags && foundCPU && !_architectureMismatch &&
StringUtils.isEmpty(vdsGroupCpuName)) {
any chance to simplify this? if there is a match then the check of hasFlags and
foundCPU are already true due to the code in line 62
Line 83: // update group with the cpu name
Line 84:
Line 85: grp.setcpu_name(sc.getCpuName());
Line 86: grp.setArchitecture(null);
Line 82: if (_hasFlags && foundCPU && !_architectureMismatch &&
StringUtils.isEmpty(vdsGroupCpuName)) {
Line 83: // update group with the cpu name
Line 84:
Line 85: grp.setcpu_name(sc.getCpuName());
Line 86: grp.setArchitecture(null);
why null and not undefind? this not aligned with the db.
the UpdateVdsGroup.getArchitercture should be change - we shouldn't assume null
as a valid value.
we should always use the Architecture.undefined value. its preferred to just
null in any way.
Line 87:
Line 88: // use suppress in order to update group even if action
fails
Line 89: // (out of the transaction)
Line 90: VdsGroupOperationParameters tempVar = new
VdsGroupOperationParameters(grp);
--
To view, visit http://gerrit.ovirt.org/22029
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia617481b102b3d5383a0d38da03e10ff95afefe2
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vitor de Lima <[email protected]>
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa
<[email protected]>
Gerrit-Reviewer: Leonardo Bianconi <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Vitor de Lima <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
