Yair Zaslavsky has posted comments on this change.
Change subject: oVirt Node Upgrade: Support N configuration
......................................................................
Patch Set 13: (7 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
Line 79: }
Line 80:
Line 81: @Override
Line 82: protected boolean canDoAction() {
Line 83: if (getVdsId() == null || getVdsId().equals(Guid.Empty)) {
Validator usage here is a bit wrong IMHO.
As I said, I do not want to block the patch on this, but still... here are some
ideas...
Line 84: return validate(new
ValidationResult(VdcBllMessages.VDS_INVALID_SERVER_ID));
Line 85: }
Line 86: if (getVds() == null) {
Line 87: return validate(new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST));
Line 80:
Line 81: @Override
Line 82: protected boolean canDoAction() {
Line 83: if (getVdsId() == null || getVdsId().equals(Guid.Empty)) {
Line 84: return validate(new
ValidationResult(VdcBllMessages.VDS_INVALID_SERVER_ID));
1. VdsValidator should have "ValidationResult isValidVdsId" , but - can we have
a flow in which vdsId is empty? Engine generates this id, why should it even
happen?
Line 85: }
Line 86: if (getVds() == null) {
Line 87: return validate(new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST));
Line 88: }
Line 82: protected boolean canDoAction() {
Line 83: if (getVdsId() == null || getVdsId().equals(Guid.Empty)) {
Line 84: return validate(new
ValidationResult(VdcBllMessages.VDS_INVALID_SERVER_ID));
Line 85: }
Line 86: if (getVds() == null) {
2. I have a patch for ActivateVds which introduces a method for validating the
vds is not null. I can split the patch even more and just send a validator
patch, unfortunately, beisdes mperina nobody reviewed it, i would love to see
more comments.
Line 87: return validate(new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST));
Line 88: }
Line 89: if (isOvirtReInstallOrUpgrade()) {
Line 90: // Block re-install on non-operational Host
Line 104: String.format("$IsoVersion %1$s",
ovirtHostOsVersion.getMajor())
Line 105: ));
Line 106: }
Line 107:
Line 108: _iso = iso;
a canDo method that changes state in the command is really a bummer. does it
have to be this way? yes, i know method likes "getVds" also change it, this is
also wrong IMHO.
Line 109: }
Line 110: return validate(ValidationResult.VALID);
Line 111: }
Line 112:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OVirtNodeInfo.java
Line 9: import org.ovirt.engine.core.utils.log.LogFactory;
Line 10: import org.ovirt.engine.core.common.config.Config;
Line 11: import org.ovirt.engine.core.common.config.ConfigValues;
Line 12:
Line 13: public class OVirtNodeInfo {
I have some debts here from previous rounds , please review my 2 inline
comments.
Line 14: public Pattern pattern;
Line 15: public File path;
Line 16: public String minimumVersion;
Line 17:
Line 22: this.path = path;
Line 23: this.minimumVersion = minimumVersion;
Line 24: }
Line 25:
Line 26: public static List<OVirtNodeInfo> get() {
1. We debated whether the "get" logic should reside in some other class.
If possible, it would be nicer (matter of taste) to have
OvirtNodeInfoFactory.getInstance().create() for example
Line 27: final String delimiter = ":";
Line 28: final String[] path =
Config.resolveOVirtISOsRepositoryPath().split(delimiter);
Line 29: final String minimumVersion[] = Config.<String>
GetValue(ConfigValues.OvirtInitialSupportedIsoVersion).split(delimiter);
Line 30:
Line 47: log.debugFormat("NodeInfo: regex {0} path {1},
minimumVersion {2}",
Line 48: regex[i], path[i], minimumVersion[i]);
Line 49: info.add(new OVirtNodeInfo(Pattern.compile(regex[i]), new
File(path[i]), minimumVersion[i]));
Line 50: }
Line 51: return info;
2. The reason I wanted list to be returned is because toArray allocates more
memory, yes, I know we java people love to spend memory, but still... :)
Line 52: }
--
To view, visit http://gerrit.ovirt.org/14756
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfb9dc5d0dc8780b519107acbe0ae866831f782c
Gerrit-PatchSet: 13
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Michael Burns <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
