Liron Ar has posted comments on this change.
Change subject: core: [WIP] save memory state on live snapshot with memory
......................................................................
Patch Set 9: (8 inline comments)
partially reviewed..see comments inline
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
Line 132: public NGuid getStorageDomainId() {
Line 133: if (cachedStorageDomainId.equals(Guid.Empty) && getVm() !=
null) {
Line 134: long sizeNeeded = getVm().getTotalMemorySizeInBytes() /
BYTES_IN_GB;
Line 135: StorageDomain storageDomain =
VmHandler.findStorageDomainForMemory(getVm().getStoragePoolId(), sizeNeeded);
Line 136: if (storageDomain != null) {
perhaps set the cachedId to null of storageDomain returned as null , so that
we won't perform those queries each time this method is called later on.
Line 137: cachedStorageDomainId = storageDomain.getId();
Line 138: }
Line 139: }
Line 140: return cachedStorageDomainId;
Line 217: String[] strings =
getVm().getHibernationVolHandle().split(",");
Line 218: Guid[] guids = new Guid[strings.length];
Line 219: for (int i=0; i<strings.length; ++i) {
Line 220: guids[i] = new Guid(strings[i]);
Line 221: }
please replace this with GuidUtils.getGuidListFromString()
Line 222:
Line 223: VDSReturnValue vdsRetValue1 = runVdsCommand(
Line 224: VDSCommandType.DeleteImageGroup,
Line 225: new
DeleteImageGroupVDSCommandParameters(guids[1], guids[0], guids[2],
Line 224: VDSCommandType.DeleteImageGroup,
Line 225: new
DeleteImageGroupVDSCommandParameters(guids[1], guids[0], guids[2],
Line 226: false, false,
getVm().getVdsGroupCompatibilityVersion().toString()));
Line 227:
Line 228: if (!vdsRetValue1.getSucceeded()) {
1. if the delete fails, exception would be thrown and we will continue to try
to end the command..you should please change to try-catch here .
2. a task is being created here and not added..so it will never be cleaned.
Line 229: // log..
Line 230: }
Line 231: }
Line 232: } else {
Line 315: }
Line 316: else {
Line 317: return new
SnapshotVDSCommandParameters(getVm().getRunOnVds().getValue(),
Line 318: getVm().getId(),
Line 319: ImagesHandler.filterImageDisks(pluggedDisks,
false, true));
perhaps filter the list before the if-else blocks and then just pass it..
Line 320: }
Line 321: }
Line 322:
Line 323: private void handleVdsLiveSnapshotFailure(VdcBLLException e) {
Line 477:
Line 478: return list;
Line 479: }
Line 480:
Line 481: private interface MemoryImageBuilder {
I don't think that we need those..we can just perform the operation if it's
supported and needed.
I'm in favour to separate those to other classes so they can be reused, but
while they all in the command - I'm against it..consider changing/separating.
Line 482: void build();
Line 483: String getVolumeStringRepresentation();
Line 484: }
Line 485:
Line 482: void build();
Line 483: String getVolumeStringRepresentation();
Line 484: }
Line 485:
Line 486: private class DefaultMemoryImageBuilder implements
MemoryImageBuilder {
IMO - better to export this from this file.
Line 487: private Guid storageDomainId;
Line 488: private Guid storagePoolId;
Line 489: private Guid memoryDumpImageGroupId;
Line 490: private Guid memoryDumpVolumeId;
Line 501: }
Line 502:
Line 503: @Override
Line 504: public void build() {
Line 505: createImageForMemoryDump();
please switch the order, the metadata is much smaller then the memory so it's
better to create it first in case of an error between the executions.
Line 506: createImageForVmMetaData();
Line 507: }
Line 508:
Line 509: private void createImageForVmMetaData() {
Line 576: vmConfVolumeId);
Line 577: }
Line 578: }
Line 579:
Line 580: private class NullableImageBuilder implements MemoryImageBuilder {
please export this from this file,
and if it stays - please add here static modifier, it doesn't need access to
the enclosing class instance.
Line 581: @Override
Line 582: public void build() {
Line 583: //no op
Line 584: }
--
To view, visit http://gerrit.ovirt.org/14291
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbb28efda1b63e506233a88399488a256e6ab1c8
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Arik Hadas <aha...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
