Liron Ar has posted comments on this change.
Change subject: core: add the image block alignment scan
......................................................................
Patch Set 8: (12 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetDiskAlignmentCommand.java
Line 47: public class GetDiskAlignmentCommand<T extends
GetDiskAlignmentParameters> extends CommandBase<T> {
Line 48: private static final long serialVersionUID = -7266894047095142486L;
Line 49:
Line 50: private Disk diskToScan;
Line 51: private Guid vdsInPool, storagePoolId, vdsGroupId;
Do we need those? there are one's defined higher in the hirerchy
Line 52: private VM diskVm;
Line 53: private List<PermissionSubject> permsList;
Line 54:
Line 55: public GetDiskAlignmentCommand(T parameters) {
Line 75: }
Line 76:
Line 77: private String getDiskIsUsedByGetAlignment() {
Line 78: return new
StringBuilder(VdcBllMessages.ACTION_TYPE_FAILED_DISK_IS_USED_BY_GET_ALIGNMENT.name())
Line 79: .append(String.format("DiskAlias %1$s",
getDiskAlias()))
1. missing $ before DiskAlias?
2. perhaps we can just eliminate the DiskAlias from the message to avoid
constructing this string each time, the user should knows about what he clicked
:) ..whatever you prefer.
Line 80: .toString();
Line 81: }
Line 82:
Line 83: @Override
Line 228: }
Line 229: return storagePoolId;
Line 230: }
Line 231:
Line 232: public Guid getVdsGroupId() {
seems to me that this method can be removed, take a look at the base
implementation.
if you do need to leave it, please add @Override
Line 233: if (vdsGroupId == null && getVm() != null) {
Line 234: vdsGroupId = getVm().getVdsGroupId();
Line 235: }
Line 236: return vdsGroupId;
Line 235: }
Line 236: return vdsGroupId;
Line 237: }
Line 238:
Line 239: protected Guid getVdsIdInPool() {
the name should be changed now..along with the CDA message :)
Line 240: if (vdsInPool == null && getVdsGroupId() != null) {
Line 241: List<VDS> vdsInPoolList =
getVdsDAO().getAllForVdsGroupWithStatus(getVdsGroupId(), VDSStatus.Up);
Line 242: if (!vdsInPoolList.isEmpty()) {
Line 243: vdsInPool = vdsInPoolList.get(0).getId();
Line 263: }
Line 264:
Line 265: public VM getVm() {
Line 266: if (diskVm == null && getDisk() != null) {
Line 267: for (VM vm :
getVmDAO().getVmsListForDisk(getDisk().getId())) {
we need to get vm that the disk is plugged to..the disk might not be plugged to
the selected vm
Line 268: diskVm = vm;
Line 269: break;
Line 270: }
Line 271: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/BaseDisk.java
Line 203: }
Line 204:
Line 205: public void setLastAlignmentScan(Date value) {
Line 206: lastAlignmentScan = (value != null) ? (Date) value.clone() :
null;
Line 207: }
I'm not sure that we have a usecase that requires the clone, it's indeed
correct programming - but what will happen actually is that on every db load we
will create two date objects each time - but let's hear other opinions as well
Line 208:
Line 209: @Override
Line 210: public int hashCode() {
Line 211: final int prime = 31;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/DiskAlignment.java
Line 2:
Line 3: import java.util.HashMap;
Line 4: import java.util.Map;
Line 5:
Line 6: public enum DiskAlignment {
please change accordingly to the comments in previous patchset
Line 7:
Line 8: Unknown(0),
Line 9: NotApplicable(1), // future use, e.g. disks with no partition table
Line 10: Misaligned(2),
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/GetDiskImageAlignmentVDSCommandParameters.java
Line 8:
Line 9: public class GetDiskImageAlignmentVDSCommandParameters extends
GetDiskAlignmentVDSCommandParameters {
Line 10: private Guid poolId, domainId, imageGroupId, imageId;
Line 11:
Line 12: public GetDiskImageAlignmentVDSCommandParameters(Guid vdsId, Guid
vmId) {
consider having the ctor receiving all the parameters
Line 13: super(vdsId, vmId);
Line 14: }
Line 15:
Line 16: public void setPoolId(Guid poolId) {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/GetDiskLunAlignmentVDSCommandParameters.java
Line 8:
Line 9: public class GetDiskLunAlignmentVDSCommandParameters extends
GetDiskAlignmentVDSCommandParameters {
Line 10: private String lunId;
Line 11:
Line 12: public GetDiskLunAlignmentVDSCommandParameters(Guid vdsId, Guid
vmId) {
consider having here all the parameters
Line 13: super(vdsId, vmId);
Line 14: }
Line 15:
Line 16: public void setLunId(String lunId) {
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 806: ACTION_TYPE_FAILED_VM_SNAPSHOT_NOT_IN_PREVIEW=Cannot ${action}
${type} to a Snapshot that is not being previewed. Please select the correct
Snapshot to restore to: Either the one being previewed, or the one before the
preview.
Line 807: ACTION_TYPE_FAILED_SHAREABLE_DISK_NOT_SUPPORTED=Cannot ${action} a
shareable ${type} (${diskAliases}). This operation is not supported.
Line 808: ACTION_TYPE_FAILED_DISK_NOT_EXIST=Cannot ${action} ${type}. The
specified disk does not exist.
Line 809: ACTION_TYPE_FAILED_NO_DISKS_SPECIFIED=Cannot ${action} ${type}. No
disks have been specified.
Line 810: ACTION_TYPE_FAILED_DISK_IS_NOT_VM_DISK=Cannot ${action} ${type}. The
following disks are not attached to any VM: ${diskAliases}.
/s/disks/disk(s)
Line 811: ACTION_TYPE_FAILED_DISK_IS_NOT_TEMPLATE_DISK=Cannot ${action}
${type}. The selected disk is not a template disk. Only template disks can be
copied.
Line 812: ACTION_TYPE_FAILED_SOURCE_AND_TARGET_SAME=Cannot ${action} ${type}.
The source and target storage domains are the same.
Line 813: ACTION_TYPE_FAILED_CANNOT_MOVE_TEMPLATE_DISK=Cannot ${action}
${type}. Template disks cannot be moved.
Line 814:
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties
Line 96: job.DisableGlusterHook=Disabling Gluster Hook ${GlusterHookName}
Line 97: job.UpdateGlusterHook=Updating Gluster Hook ${GlusterHookName} on
conflicting servers in Cluster ${Cluster}
Line 98: job.AddGlusterHook=Adding Gluster Hook ${GlusterHookName} on
conflicting servers in Cluster ${Cluster}
Line 99: job.RemoveGlusterHook=Removing Gluster Hook ${GlusterHookName} from
all servers in Cluster ${Cluster}
Line 100: job.GetDiskAlignment=Scanning Block Alignment on Disk ${DiskAlias}
/s/Block/block
/s/Alignment/alignment
Line 101:
Line 102: # Step types
Line 103: step.VALIDATING=Validating
Line 104: step.EXECUTING=Executing
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/GetDiskAlignmentVDSCommand.java
Line 9: super(parameters);
Line 10: }
Line 11:
Line 12: @Override
Line 13: protected void ExecuteVdsBrokerCommand() {
please add a log here with the call information
Line 14: diskAlignment =
getBroker().getDiskAlignment(getParameters().getVmId().toString(),
getParameters().getDriveSpecs());
Line 15: ProceedProxyReturnValue();
Line 16:
Line 17: // At the moment we only check that all the partition are
aligned.
--
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: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
