Mike Kolesnik has posted comments on this change.
Change subject: engine: introducing GetCapabilitiesCommand
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RefreshHostCapabilitiesCommand.java
Line 31: log.infoFormat("Before acquiring lock in order to prevent
monitoring for host {0} from data-center {1}",
Line 32: vds.getName(),
Line 33: vds.getStoragePoolName());
Line 34: getLockManager().acquireLockWait(monitoringLock);
Line 35: log.infoFormat("Lock acquired, from now a monitoring of host
will be skipped for host {0} from data-center {1}",
I see this code is shared with ActivateVdsCommand, so perhaps it's better to
extract this to some static method?
Line 36: vds.getName(),
Line 37: vds.getStoragePoolName());
Line 38: try {
Line 39:
ResourceManager.getInstance().GetVdsManager(getVdsId()).refreshCapabilities(new
AtomicBoolean(), getVds());
Line 35: log.infoFormat("Lock acquired, from now a monitoring of host
will be skipped for host {0} from data-center {1}",
Line 36: vds.getName(),
Line 37: vds.getStoragePoolName());
Line 38: try {
Line 39:
ResourceManager.getInstance().GetVdsManager(getVdsId()).refreshCapabilities(new
AtomicBoolean(), getVds());
I'm not sure that this code will refresh anything but the networking
capabilities (i.e. only the networking capabilities are persisted in the DB),
is that your intent?
Line 40: setSucceeded(true);
Line 41: } catch (Exception e) {
Line 42: setSucceeded(false);
Line 43: } finally {
Line 38: try {
Line 39:
ResourceManager.getInstance().GetVdsManager(getVdsId()).refreshCapabilities(new
AtomicBoolean(), getVds());
Line 40: setSucceeded(true);
Line 41: } catch (Exception e) {
Line 42: setSucceeded(false);
You should either re-throw the exception, not catch it (since succeeded is
false by default), or throw a new exception to let know what happened.
Generally, catching Exception is not a good idea as it is too broad, and you
should try to be more specific (if you decide to catch it).
Line 43: } finally {
Line 44: getLockManager().releaseLock(monitoringLock);
Line 45: log.infoFormat("Get Capabilities finished. Lock released.
Monitoring can run now for host {0} from data-center {1}",
Line 46: vds.getName(),
Line 40: setSucceeded(true);
Line 41: } catch (Exception e) {
Line 42: setSucceeded(false);
Line 43: } finally {
Line 44: getLockManager().releaseLock(monitoringLock);
This code can also be in a static method..
Line 45: log.infoFormat("Get Capabilities finished. Lock released.
Monitoring can run now for host {0} from data-center {1}",
Line 46: vds.getName(),
Line 47: vds.getStoragePoolName());
Line 48: }
Line 53: if (getVds() == null) {
Line 54: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_HOST_NOT_EXIST);
Line 55: }
Line 56: if (!isValidVdsStatus()) {
Line 57:
addCanDoActionMessage(VdcBllMessages.VAR__HOST_STATUS__UP_MAINTENANCE_OR_NON_OPERATIONAL);
So if host is non-operational and you refresh it and it is compliant, does it
go back up by itself?
Line 58:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Line 59: return false;
Line 60: }
Line 61: return true;
Line 79: }
Line 80:
Line 81: @Override
Line 82: public AuditLogType getAuditLogTypeValue() {
Line 83: return getSucceeded() ? AuditLogType.VDS_GET_CAPABILITIES :
AuditLogType.VDS_GET_CAPABILITIES_FAILED;
Probably more correctly phrased as HOST_REFRESHED_CAPABILITIES and
HOST_REFRESH_CAPABILITIES_FAILED
Line 84: }
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 182: VDS_ACTIVATE=Host ${VdsName} was activated by ${UserName}.
Line 183: VDS_ACTIVATE_ASYNC=Host ${VdsName} was autorecovered.
Line 184: VDS_ACTIVATE_FAILED=Failed to activate Host ${VdsName}.(User:
${UserName}).
Line 185: VDS_ACTIVATE_FAILED_ASYNC=Failed to autorecover Host ${VdsName}.
Line 186: VDS_GET_CAPABILITIES=Host ${VdsName} refresh capabilities finished
successfully.
Probably better phrased as:
Successfully refreshed the capabilities of host ${VdsName}.
Line 187: VDS_GET_CAPABILITIES_FAILED=Failed to refresh Host ${VdsName}
capabilities.
Line 188: VDS_DETECTED=Detected new Host ${VdsName}. Host state was set to
${VdsStatus}.
Line 189: VDS_FAILURE=Host ${VdsName} is non-responsive.
Line 190: VDS_MAINTENANCE=Host ${VdsName} was switched to Maintenance Mode.
Line 183: VDS_ACTIVATE_ASYNC=Host ${VdsName} was autorecovered.
Line 184: VDS_ACTIVATE_FAILED=Failed to activate Host ${VdsName}.(User:
${UserName}).
Line 185: VDS_ACTIVATE_FAILED_ASYNC=Failed to autorecover Host ${VdsName}.
Line 186: VDS_GET_CAPABILITIES=Host ${VdsName} refresh capabilities finished
successfully.
Line 187: VDS_GET_CAPABILITIES_FAILED=Failed to refresh Host ${VdsName}
capabilities.
Probably better phrased as:
Failed to refresh the capabilities of host ${VdsName}.
Line 188: VDS_DETECTED=Detected new Host ${VdsName}. Host state was set to
${VdsStatus}.
Line 189: VDS_FAILURE=Host ${VdsName} is non-responsive.
Line 190: VDS_MAINTENANCE=Host ${VdsName} was switched to Maintenance Mode.
Line 191: VDS_MAINTENANCE_FAILED=Failed to switch Host ${VdsName} to
Maintenance mode.
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/LocalizedEnums.java
Line 186: String AuditLogType___VDS_MAINTENANCE_FAILED();
Line 187:
Line 188: String AuditLogType___VDS_ACTIVATE_FAILED();
Line 189:
Line 190: String AuditLogType___VDS_GET_CAPABILITIES();
Is this necessary here?
I thought the UI get these values already as text not as enum keys.
Additionally, on oVirt.org its not documented:
http://www.ovirt.org/Engine_Adding_Messages#Audit_Log_Messages
Line 191:
Line 192: String AuditLogType___VDS_GET_CAPABILITIES_FAILED();
Line 193:
Line 194: String AuditLogType___VDS_RECOVER_FAILED();
--
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: 3
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
