Allon Mureinik has posted comments on this change.
Change subject: core: add the image block alignment scan
......................................................................
Patch Set 5: I would prefer that you didn't submit this
(6 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ScanDiskAlignmentCommand.java
Line 134: lunParameters.setLunGuid(lunDisk.getLun().getLUN_id());
Line 135:
Line 136: parameters = lunParameters;
Line 137: } else {
Line 138: throw new RuntimeException("Unknown DiskStorageType " +
getDiskType().toString());
consider throwing VdcBllError instead of RuntimeException
Line 139: }
Line 140:
Line 141: Boolean isDiskAligned = (Boolean) runVdsCommand(
Line 142: VDSCommandType.ScanDiskAlignment,
parameters).getReturnValue();
Line 191: protected boolean isImageLockNeeded() {
Line 192: /* In case the volume format is RAW (same as a direct LUN)
the image lock is not
Line 193: * needed since the alignment scan can run without any
interference by a concurrent
Line 194: * running VM.
Line 195: */
I disagree - you're not only locking this operation, you're locking the entire
disk.
Can a disk be deleted while you're scanning? moved? live-migrated? added a qcow
layer?
IMHO - no.
Line 196: return (getDiskType() == DiskStorageType.IMAGE &&
Line 197: ((DiskImage) getDisk()).getVolumeFormat() !=
VolumeFormat.RAW);
Line 198: }
Line 199:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/BaseDisk.java
Line 185: alignment = value;
Line 186: }
Line 187:
Line 188: public Date getLastAlignmentScan() {
Line 189: return (Date) lastAlignmentScan.clone();
sounds good to me.
Line 190: }
Line 191:
Line 192: public void setLastAlignmentScan(Date value) {
Line 193: lastAlignmentScan = (Date) value.clone();
Line 189: return (Date) lastAlignmentScan.clone();
Line 190: }
Line 191:
Line 192: public void setLastAlignmentScan(Date value) {
Line 193: lastAlignmentScan = (Date) value.clone();
sounds good to me
Line 194: }
Line 195:
Line 196: @Override
Line 197: public int hashCode() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskAlignment.java
Line 10: Misaligned(2),
Line 11: Aligned(3);
Line 12:
Line 13: private int intValue;
Line 14: final private static Map<Integer, DiskAlignment> mappings;
actually, the convention is private static final
Line 15:
Line 16: static {
Line 17: mappings = new HashMap<Integer, DiskAlignment>();
Line 18:
....................................................
Commit Message
Line 44:
Line 45: The result (with the scan execution timestamp) is stored in the
Line 46: base_disks table (as other similar properties such as the bootable
Line 47: and shareable flags).
Line 48:
nitpicking: consider adding a link to the RFE
Line 49: Change-Id: I4858b7bbfa453230fcafecfbc5358c715d5d825b
--
To view, visit http://gerrit.ovirt.org/11946
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4858b7bbfa453230fcafecfbc5358c715d5d825b
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
