Allon Mureinik has posted comments on this change.
Change subject: <core>: WipeAfterDelete defaults to true in GUI, but false in
REST API.
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 285: ImagesHandler.setDiskAlias(getParameters().getDiskInfo(),
getVm());
Line 286: if(!getParameters().getDiskInfo().isWipeAfterDeleteSet()) {
Line 287: StorageType storageType =
getStorageDomain().getstorage_type();
Line 288:
getParameters().getDiskInfo().setWipeAfterDelete(WipeAfterDeleteUtils.getDefaultWipeAfterDeleteFlag(storageType));
Line 289: }
consider adding another blank line before this block in order to separate both
ifs.
Line 290: if (DiskStorageType.IMAGE ==
getParameters().getDiskInfo().getDiskStorageType()) {
Line 291: createDiskBasedOnImage();
Line 292: } else {
Line 293: createDiskBasedOnLun();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/WipeAfterDeleteUtils.java
Line 1: /*
Line 2: * To change this template, choose Tools | Templates
Line 3: * and open the template in the editor.
Line 4: */
This comment should be removed.
Line 5: package org.ovirt.engine.core.bll.utils;
Line 6:
Line 7: import org.ovirt.engine.core.common.businessentities.StorageType;
Line 8: import org.ovirt.engine.core.common.config.Config;
Line 17: public static synchronized boolean
getDefaultWipeAfterDeleteFlag(final StorageType storageType) {
Line 18: if(storageType.isBlockDomain()) {
Line 19: if(wipeAfterDeleteBlockStorageDomain == null) {
Line 20: wipeAfterDeleteBlockStorageDomain =
Line 21:
Config.<Boolean>GetValue(ConfigValues.SANWipeAfterDelete);
Config already caches it's values, so would be double caching.
moreover, you're preventing the ability to have this as a reloadable
configuration.
This entire function can be considerably simplified:
(psuedocode)
if blockDoamin:
return Config.getValue(SANWipeAfterDelete);
else // if it's not a block domain, it's a file domain
return the WIPE_AFTER_DELETE_FILE_DOMAIN constant;
Line 22: }
Line 23: return wipeAfterDeleteBlockStorageDomain;
Line 24: }
Line 25: if(storageType.isFileDomain()) {
Line 20: wipeAfterDeleteBlockStorageDomain =
Line 21:
Config.<Boolean>GetValue(ConfigValues.SANWipeAfterDelete);
Line 22: }
Line 23: return wipeAfterDeleteBlockStorageDomain;
Line 24: }
consider adding a blank line here.
Line 25: if(storageType.isFileDomain()) {
Line 26: return wipeAfterDeleteFileStorageDomain;
Line 27: }
Line 28: return false;
....................................................
Commit Message
Line 3: AuthorDate: 2012-10-15 17:41:37 +0200
Line 4: Commit: Ravi Nori <rn...@redhat.com>
Line 5: CommitDate: 2012-10-16 17:00:06 +0200
Line 6:
Line 7: <core>: WipeAfterDelete defaults to true in GUI, but false in REST API.
s/<core>/core/
Line 8:
Line 9: WipeAfterDelete in gui default to false for File Storage Domains and
for Block storage domains the value is defined by SANWipeAfterDelete'
configuration value. The backend logic has been changed to mimic this behaviour.
Line 10:
Line 11: Change-Id: I1aeeb7d30a604e1647446021e3a96f35f3b4e2b7
--
To view, visit http://gerrit.ovirt.org/8570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1aeeb7d30a604e1647446021e3a96f35f3b4e2b7
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
