Alissa Bonas has posted comments on this change.
Change subject: engine: Remove getImageDomainsList API
......................................................................
Patch Set 2: (12 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 444: if (!getParameters().isImportAsNewEntity() &&
atLeastOneImageFoundInStorageDomain(imageList,
Line 445: getParameters().getDestDomainId())) {
Line 446: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_ALREADY_CONTAINS_DISK);
Line 447: }
Line 448: } catch (RuntimeException e) {
shouldn't this catch part also make the cando method fail by calling
failCanDoAction with appropriate error message?
Line 449: // There is nothing to do in case of VDSM failure to
retrieve images from the given storage domain
Line 450: // except of writing the log file. The command itself
will probably fail during the execution
Line 451: // and then user will get meaningful error message
Line 452: log.error("Failed to validate if target storage domain
already contains one of the template disks! " +
Line 445: getParameters().getDestDomainId())) {
Line 446: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_ALREADY_CONTAINS_DISK);
Line 447: }
Line 448: } catch (RuntimeException e) {
Line 449: // There is nothing to do in case of VDSM failure to
retrieve images from the given storage domain
Why proceed to executeCommand if this part fails, and count on the fact that it
will probably fail?
Line 450: // except of writing the log file. The command itself
will probably fail during the execution
Line 451: // and then user will get meaningful error message
Line 452: log.error("Failed to validate if target storage domain
already contains one of the template disks! " +
Line 453: e.getMessage());
Line 449: // There is nothing to do in case of VDSM failure to
retrieve images from the given storage domain
Line 450: // except of writing the log file. The command itself
will probably fail during the execution
Line 451: // and then user will get meaningful error message
Line 452: log.error("Failed to validate if target storage domain
already contains one of the template disks! " +
Line 453: e.getMessage());
I recommend to add more information to the message - name/id of the target
storage domain, template disk id/name. Otherwise it's just a general message in
the log there's nothing much to do about it.
Line 454: }
Line 455:
Line 456: if (getParameters().getCopyCollapse() ||
VmTemplateHandler.BlankVmTemplateId.equals(getVm().getVmtGuid())) {
Line 457: return true;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyTemplateCommand.java
Line 213: :
AuditLogType.USER_COPIED_TEMPLATE_FINISHED_FAILURE;
Line 214: }
Line 215: }
Line 216:
Line 217: protected boolean
allImagesFoundInStorageDomain(Iterable<DiskImage> images, Guid stDomainId) {
stDomainId is a very unclear name. You probably meant storageDomainId ? - it
will be more clear to name it like that.
Line 218: boolean res = true;
Line 219: List<Guid> imagesInStorageDomain =
getImagesFromStorageDomain(stDomainId);
Line 220: for (DiskImage image : images) {
Line 221: if (res &&
!imagesInStorageDomain.contains(image.getId())) {
Line 217: protected boolean
allImagesFoundInStorageDomain(Iterable<DiskImage> images, Guid stDomainId) {
Line 218: boolean res = true;
Line 219: List<Guid> imagesInStorageDomain =
getImagesFromStorageDomain(stDomainId);
Line 220: for (DiskImage image : images) {
Line 221: if (res &&
!imagesInStorageDomain.contains(image.getId())) {
it seems here that you are going over all images in the for loop when it's not
really needed - it's enough to have even one to set it to false , and it
doesn't enter the if statement anymore. why not just exit after the first one
sets the result to false?
Line 222: res = false;
Line 223: }
Line 224: }
Line 225: return res;
Line 228: protected boolean
atLeastOneImageFoundInStorageDomain(Iterable<DiskImage> images, Guid
stDomainId) {
Line 229: boolean res = false;
Line 230: List<Guid> imagesInStorageDomain =
getImagesFromStorageDomain(stDomainId);
Line 231: for (DiskImage image : images) {
Line 232: if (res || imagesInStorageDomain.contains(image.getId()))
{
no need here to iterate when the res is set to true the first time - it's time
to exit the for loop and stop all the iterations.
Line 233: res = true;
Line 234: }
Line 235: }
Line 236: return res;
Line 236: return res;
Line 237: }
Line 238:
Line 239: protected List<Guid> getImagesFromStorageDomain(Guid stDomainId) {
Line 240: VDSReturnValue runVdsCommand = getBackend()
the variable name is not clear - this is the return value of the vds command,
its type is VDSReturn value - why not call it something like
vdsCommandReturnValue?
Line 241: .getResourceManager()
Line 242: .RunVdsCommand(
Line 243: VDSCommandType.GetImagesList,
Line 244: new
GetImagesListVDSCommandParameters(stDomainId)
Line 246:
Line 247: if (runVdsCommand.getSucceeded()) {
Line 248: return (List<Guid>) runVdsCommand.getReturnValue();
Line 249: } else {
Line 250: throw new RuntimeException("Failed to retrieve list of
images from storage domain '" + stDomainId + "'!");
1. Why not extract the error message returned from vdsm and return it instead
of a general RuntimeException?
2. Anyway, please add logging of the exception to allow a more clear
troubleshooting.
You cannot count on the fact nor control that every caller of this method in
every command will log it, it's best to log it at the moment the exception
occurs.
Line 251: }
Line 252: }
Line 253:
Line 254: protected void endMoveOrCopyCommand() {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmCommandTest.java
Line 53: import static org.mockito.Matchers.anyString;
Line 54: import static org.mockito.Mockito.doAnswer;
Line 55: import static org.mockito.Mockito.doReturn;
Line 56: import static org.mockito.Mockito.spy;
Line 57: import static org.mockito.Mockito.when;
1. seems that your code formatter is not tuned. statics usually come before
regular imports...
2. This is a test for addVmCommand, however the command itself was not changed
in this patch. Is that on purpose?
Line 58:
Line 59: @RunWith(MockitoJUnitRunner.class)
Line 60: @SuppressWarnings("serial")
Line 61: public class AddVmCommandTest {
....................................................
Commit Message
Line 5: CommitDate: 2013-06-26 15:05:34 +0300
Line 6:
Line 7: engine: Remove getImageDomainsList API
Line 8:
Line 9: We decided to refactor ImportVmCommand to make flow more simple and
reduce
It's not clear who is "we", and not sure it's important in the context of the
commit message :)
I suggest to just start the sentence with "refactor ImportVmCommand..."
Also, there are more commands that are influenced in this patch. Please mention
them as well in commit message/explain why it is ok to change their logic too.
Line 10: the number of calls to VDSM. The first step is removing
Line 11: getImagesDomainsList API which takes storagePoolId and imageId as an
Line 12: input and returns all storage domains from the pool containing that
Line 13: image. What we are really need is to check the image existence in the
Line 8:
Line 9: We decided to refactor ImportVmCommand to make flow more simple and
reduce
Line 10: the number of calls to VDSM. The first step is removing
Line 11: getImagesDomainsList API which takes storagePoolId and imageId as an
Line 12: input and returns all storage domains from the pool containing that
double space between "domains" and "from"
Line 13: image. What we are really need is to check the image existence in the
Line 14: specific storage domain therefore more appropriate API to use is
Line 15: getImagesList which returns all image ids contained by the
Line 16: given storage domain.
Line 9: We decided to refactor ImportVmCommand to make flow more simple and
reduce
Line 10: the number of calls to VDSM. The first step is removing
Line 11: getImagesDomainsList API which takes storagePoolId and imageId as an
Line 12: input and returns all storage domains from the pool containing that
Line 13: image. What we are really need is to check the image existence in the
we are really need-->we really need
Line 14: specific storage domain therefore more appropriate API to use is
Line 15: getImagesList which returns all image ids contained by the
Line 16: given storage domain.
Line 17:
--
To view, visit http://gerrit.ovirt.org/15951
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I33069871db34e8b129c4d80098523654cd2a0629
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: liron aravot <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
