Martin Mucha has posted comments on this change. Change subject: core: MacPool related Commands ......................................................................
Patch Set 16: (3 comments) http://gerrit.ovirt.org/#/c/28705/16/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveMacPoolByIdParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/RemoveMacPoolByIdParameters.java: Line 4: Line 5: import org.ovirt.engine.core.common.validation.group.UpdateEntity; Line 6: import org.ovirt.engine.core.compat.Guid; Line 7: Line 8: public class RemoveMacPoolByIdParameters extends VdcActionParametersBase { > You can (it makes perfect sense for all commands using it) and should do it no problem, I did it. This patch will use this RemoveMacPoolByIdParameters and following patch will fix IdParameters and will delete RemoveMacPoolByIdParameters. It should be pushed into gerrit, I've added you as a reviewer. Line 9: Line 10: @NotNull(groups = {UpdateEntity.class }) Line 11: private Guid macPoolId; Line 12: http://gerrit.ovirt.org/#/c/28705/16/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/auditloghandling/AuditLogableBase.java: Line 570: public StoragePoolDAO getStoragePoolDAO() { Line 571: return getDbFacade().getStoragePoolDao(); Line 572: } Line 573: Line 574: public MacPoolDao getMacPoolDao() { > I see. I'll fix this, but then should be AuditLogableBase parent of all commands? Since macPool commands are interrested only in like 9% of dao getters. Also like almost every field is of no interrest of most of its descendants. This does not seem to be proper design. Additionally DI is available now(at least in some modules), so all these getters should be nuked away from every class. Line 575: return getDbFacade().getMacPoolDao(); Line 576: } Line 577: Line 578: public StorageDomainStaticDAO getStorageDomainStaticDAO() { http://gerrit.ovirt.org/#/c/28705/16/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 8: IRS_NETWORK_ERROR=Storage Manager Service not responding. Line 9: IRS_PROTOCOL_ERROR=Storage Manager protocol error. Line 10: IRS_RESPONSE_ERROR=Storage Manager response error. Line 11: MAC_POOL_NOT_ENOUGH_MAC_ADDRESSES=Not enough MAC addresses left in MAC Address Pool. Line 12: ACTION_TYPE_FAILED_CANNOT_REMOVE_STILL_USED_MAC_POOL=Cannot ${action} ${type}. Mac pool cannot be removed, because some DataCenter is still using it. > It would be better to say what DCs are using the MAC pool, so that users ca this seems easy now. But if we consider that there already are requests to be able to attach MacPool to different entities than just DC, this will be difficult to implement. This will scale badly, and will not be easily translated (in given localization implementation). So question: if there's quite real possibility of using mac pools with something else than DC, do we really want to introduce method, which will be rather problematic in that future? If we really do, then how? Amend previous DB patch (new query) and this path, or add new simple patch improving message? Line 13: ACTION_TYPE_FAILED_CANNOT_REMOVE_DEFAULT_MAC_POOL=Cannot ${action} ${type}. Default MAC Pool cannot be removed. Line 14: ACTION_TYPE_FAILED_MAC_POOL_DOES_NOT_EXIST=Cannot ${action} ${type}. Mac pool does not exist. Line 15: ACTION_TYPE_FAILED_MAC_POOL_MUST_HAVE_NAME=Cannot ${action} ${type}. Mac pool cannot be created. Shared MAC pools must have names. Line 16: ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED=Cannot ${action} ${type}. Changing default MAC Pool is not supported. -- To view, visit http://gerrit.ovirt.org/28705 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c0c3657e3e53699bcafa375befdce848b01a2f3 Gerrit-PatchSet: 16 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[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
