Allon Mureinik has posted comments on this change. Change subject: core: Cleanup in RemoveImageCommand and anscentors ......................................................................
Patch Set 10: (15 inline comments) comments on Ayal's review. wherever I agreed, a new patchset will be uploaded. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseImagesCommand.java Line 45: if (parameters instanceof ImagesContainterParametersBase) { Of course. every ImNage must has a ContainTer. I'll address this in a separate patch. Line 54: setStoragePoolId(getDiskImage().getstorage_pool_id().getValue()); this is called from within the constructor - canDoAction hasn't run yet, so we do not know we have a valid disk. Not that we aren't setting getstorage_pool_id(), but getstorage_pool_id().getValue() - if the id is null, an NPE will be thrown. Line 179: DiskImage image = (DiskImage) runVdsCommand( Aparently not. Line 364: protected void SetImageStatus(DiskImage diskImage, ImageStatus imageStatus) { agreed. The disk image should have been supplied from within the class - my bad. Line 399: if (hasDiskImage()) { you're right - this entire block should have been removed. Line 401: UnLockImage(); Nice catch - will move to the two appropriate commands. CreateImageTemplateCommand - adding unlocking. RemoveImageCommand - already handled MoveOrCopyImmageCommand - added. Line 419: if (hasDiskImage()) { not sure you're correct - if the command fails, who knows what state you're left in. don't want to issue an unlock regardless. Line 435: // Default action is no action matter of style. I like to stress the fact that is is intended as a no-op, and not just an oversight. Line 448: public void GetImageChildren(Guid snapshot, List<Guid> children) { not sure - the DAO is now accessed via the instance, allowing injection, inversion of control, etc. .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommand.java Line 44: setDiskImage(((getParameters().getDiskImage()) != null) ? getParameters().getDiskImage() : getDiskImage()); This is a (probably false) optimization in RemoveImageParameters. The base class allows giving an ID, while here you can supply the object itself. This is in need of a clenaup, but frankly, I'm not sure I want to deal with this now, in this patch. Line 48: if (GuidUtils.isEmpty(getStoragePoolId())) { The same if guid != null and !Guid.Empty.equals(guid) is repeated several times - I wraped it in a method, so it becomes more readable. Line 55: if (GuidUtils.isEmpty(getParameters().getStorageDomainId()) && hasDiskImage()) { just extracted methods here - didn't give much thought to organizing their internals - the previous one defaults to Empty, this one doesn;t. Not sure if it's worth the risk. thoughts? Line 97: if (diskImage != null) { Indeed. Lost this fix in githell. Line 148: UnLockImage(); re shouldBeLocked - agreed. re its place - the method name should be changed, but I prefer it in a single method - do whatever magic you need to make the image disapear. re it's call in BaseImagesCommand - thre EndSuccessfuly is overriden - it isn't called. re bonus - agreed. It is importAnt to have CoRect capitaliZation in our CoDe. .................................................... File backend/manager/modules/compat/src/main/java/org/ovirt/engine/core/compat/NGuid.java Line 96: public boolean equalsGuid(NGuid otherGuid) { an improvement suggested by laravot in patchset 5 or 6, for performance. Don't really mind either way. Your thoughts? -- To view, visit http://gerrit.ovirt.org/6024 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie66b450571d680e556dc812d9926161d9f190221 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches