Allon Mureinik has posted comments on this change.
Change subject: core: Introduce Change quota command - disks (#848310)
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(9 inline comments)
see inline.
Also, adding Omer to the reviewers list.
Omer - please take a look at my MLA related comments and ack/nack.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeQuotaForDiskCommand.java
Line 17: }
Line 18:
Line 19: @Override
Line 20: protected boolean canDoAction() {
Line 21: // TODO check if disk exist or has quota (not lun)
OK, please do :-)
Note that this can't be a lun - _disk is a DiskImageBase
Line 22: return super.canDoAction();
Line 23: }
Line 24:
Line 25: @Override
Line 57: private long getDiskSize() {
Line 58: return _disk.getSizeInGigabytes();
Line 59: }
Line 60:
Line 61: DiskImageBase getDisk() {
why isn't this private?
Line 62: if (_disk == null) {
Line 63: _disk = (DiskImageBase)
getDbFacade().getDiskDao().get(getParameters().getObjectId());
Line 64: }
Line 65: return _disk;
Line 59: }
Line 60:
Line 61: DiskImageBase getDisk() {
Line 62: if (_disk == null) {
Line 63: _disk = (DiskImageBase)
getDbFacade().getDiskDao().get(getParameters().getObjectId());
why not just use DiskImageDao?
Line 64: }
Line 65: return _disk;
Line 66: }
Line 67:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/ChangeQuotaCommand.java
Line 48: public List<PermissionSubject> getPermissionCheckSubjects() {
Line 49: List<PermissionSubject> permissionList = new
ArrayList<PermissionSubject>();
Line 50: permissionList.add(new
PermissionSubject(getParameters().getQuotaId(),
Line 51: VdcObjectType.Quota,
Line 52: ActionGroup.CONSUME_QUOTA));
I don't think this is correct.
Consider changing a disk to use QuotaA instead of QuotaB.
I'd assume you need ManipulateDisk on the disk, and consume on QuotaB, no?
Line 53: return permissionList;
Line 54: }
Line 55:
Line 56: @Override
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ChangeQuotaParameters.java
Line 8:
Line 9: private Guid storagePoolId;
Line 10: private Guid containerId;
Line 11: private Guid quotaId;
Line 12: private Guid objectId;
bless you.
Line 13:
Line 14: /**
Line 15: * @param quotaId the new quota id
Line 16: * @param entityId vm or disk
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 158: // Quota
Line 159: AddQuota(601, ActionGroup.CONFIGURE_QUOTA, false),
Line 160: UpdateQuota(602, ActionGroup.CONFIGURE_QUOTA, false),
Line 161: RemoveQuota(603, ActionGroup.CONFIGURE_QUOTA, false),
Line 162: ChangeQuotaForDisk(604, false),
why doesn't this have ActionGroup.CONFIGURE_QUOTA?
Line 163:
Line 164: // bookmarks
Line 165: AddBookmark(701),
Line 166: RemoveBookmark(702),
....................................................
Commit Message
Line 3: AuthorDate: 2012-08-20 19:50:40 +0300
Line 4: Commit: Gilad Chaplik <gchap...@redhat.com>
Line 5: CommitDate: 2012-08-20 19:50:40 +0300
Line 6:
Line 7: core: Introduce Change quota command - disks (#848310)
either "change quota command" or ChangeQuoteCommand.
don't mix upper and lower cases.
Line 8:
Line 9: https://bugzilla.redhat.com/848310
Line 10:
Line 11: We want the abillity to change quota for disks and vms.
Line 8:
Line 9: https://bugzilla.redhat.com/848310
Line 10:
Line 11: We want the abillity to change quota for disks and vms.
Line 12: added an abstract command (ChangeQuotaCommand), that resposnible for
that.
s/that/that's/
Line 13: Currently implemented the ChangeQuotaForDisks, which gets the disk and
quota
Line 14: and tries to change the disk quota, ChangeQuotaForVm will be added in
the near
Line 15: future.
Line 16:
Line 10:
Line 11: We want the abillity to change quota for disks and vms.
Line 12: added an abstract command (ChangeQuotaCommand), that resposnible for
that.
Line 13: Currently implemented the ChangeQuotaForDisks, which gets the disk and
quota
Line 14: and tries to change the disk quota, ChangeQuotaForVm will be added in
the near
ChangeQuotaForDisks is in plural and ChangeQuotaForVm is in singular - choose a
convention and stick to it.
Line 15: future.
Line 16:
Line 17: Change-Id: I0017a18109faecd1acab52897bba5474b6600390
--
To view, visit http://gerrit.ovirt.org/7357
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0017a18109faecd1acab52897bba5474b6600390
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
