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

Reply via email to