Allon Mureinik has posted comments on this change. Change subject: engine-core: [autorecovery] Autorecovery logic ......................................................................
Patch Set 11: (8 inline comments) [reviewed on yzaslavs' request] A general comment: This patch is quite big - perhaps it would be a good idea to separate it to several patches (e.g., one for refactoring and one for the new logic). Also have some inline comments - nothing major. .................................................... File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AutoRecoveryManagerTest.java Line 53: AutoRecoveryManager.getInstance().onTimer(); I'd add some verification on the backend mock that the correct actions were really fired. .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdsActionParameters.java Line 33: see comment about runSilent in the StorageDomainParameteres. .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/storage_domains.java Line 371: consider extracting this behaviour to a common AutoRecoverableEntity interface. .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/storage_domain_static.java Line 218: consider extracting this behaviour to a common AutoRecoverableEntity interface. .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsStatic.java Line 137: private boolean autoRecoverable = true; Consider extracting this to an AutoRecoverableEntity interface. .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java Line 516: it's unclear to me what's the purpose of this refactoring. can you elaborate? .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageDomainDAOWrapperImpl.java Line 201: // TODO Auto-generated method stub consider throwing NotImplementedException - may be easier to deubg if someone uses this DAO by mistake. .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAOWrapperImpl.java Line 279: // TODO Auto-generated method stub consider throwing NotImplementedException -- To view, visit http://gerrit.ovirt.org/2011 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I544ff69bad946fd3d4edd2cf815ad10392958d26 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Laszlo Hornyak <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Laszlo Hornyak <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Michael Kublin <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
