Mike Kolesnik has posted comments on this change.
Change subject: engine: introducing GetCapabilitiesCommand
......................................................................
Patch Set 4: (6 inline comments)
Looks good, just a few small comments.
Also we will have to see about the non-op status, since as a user I'd expect
the host to be re-activated if everything is fine (maybe in that case you
should call activate host command?).
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RefreshHostCapabilitiesCommand.java
Line 26: try {
Line 27:
ResourceManager.getInstance().GetVdsManager(getVdsId()).refreshHost(getVds());
Line 28: setSucceeded(true);
Line 29: } finally {
Line 30: releaseMonitorLock(monitoringLock, "Get Capabilities");
Should be "Refresh host capabilities"
Line 31: }
Line 32: }
Line 33:
Line 34: @Override
Line 32: }
Line 33:
Line 34: @Override
Line 35: protected boolean canDoAction() {
Line 36: if (getVds() == null) {
Do you mind to write it to utilize the ValidationResult API?
Line 37: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST);
Line 38: }
Line 39: if (!isValidVdsStatus()) {
Line 40:
addCanDoActionMessage(VdcBllMessages.VAR__HOST_STATUS__UP_MAINTENANCE_OR_NON_OPERATIONAL);
Line 43: }
Line 44: return true;
Line 45: }
Line 46:
Line 47: private boolean isValidVdsStatus() {
Do you mind rewriting this to utilize the ValidationResult API (same as we do
in validators)?
Also Host not Vds :)
Line 48: VDSStatus hostStatus = getVds().getStatus();
Line 49: return ((hostStatus == VDSStatus.Maintenance) || (hostStatus
== VDSStatus.Up) || (hostStatus == VDSStatus.NonOperational));
Line 50: }
Line 51:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsCommand.java
Line 199:
getReturnValue().getFault().setMessage(returnValue.getVdsError().getMessage());
Line 200:
getReturnValue().getExecuteFailedMessages().add(returnValue.getVdsError().getMessage());
Line 201: }
Line 202:
Line 203: protected EngineLock aquireMonitorLock() {
Should be a*c*quire
Line 204: final VDS vds = getVds();
Line 205: EngineLock monitoringLock =
Line 206: new
EngineLock(Collections.singletonMap(getParameters().getVdsId().toString(),
Line 207: new Pair<String,
String>(LockingGroup.VDS_INIT.name(), "")), null);
Line 216: return monitoringLock;
Line 217: }
Line 218:
Line 219: protected void releaseMonitorLock(EngineLock monitoringLock,
String commandName) {
Line 220: final VDS vds = getVds();
This line should come after the lock is released, since it's not actually used
until then.
Line 221: getLockManager().releaseLock(monitoringLock);
Line 222: log.infoFormat(commandName + " finished. Lock released.
Monitoring can run now for host {0} from data-center {1}",
Line 223: vds.getName(),
Line 224: vds.getStoragePoolName());
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java
Line 75: // Proxy host selection
Line 76: PROXY_HOST_SELECTION(605),
Line 77:
Line 78: HOST_REFRESHED_CAPABILITIES(606),
Line 79: HOST_REFRESHED_CAPABILITIES_FAILED(607),
I think "HOST_REFRESH_CAPABILITIES_FAILED" without the -ed suffix in refresh
would make more sense.
Line 80:
Line 81: // -- IRS Log types --
Line 82: IRS_FAILURE(22, AuditLogTimeInterval.HOUR.getValue() * 12),
Line 83: IRS_DISK_SPACE_LOW(26, AuditLogTimeInterval.HOUR.getValue() * 12),
--
To view, visit http://gerrit.ovirt.org/15640
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba25577a7b66f3acedc3b32b258560861d19c621
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
