Liron Aravot has posted comments on this change.
Change subject: core: Extract disk alias generation methods
......................................................................
Patch Set 4: (8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
Line 50: import org.ovirt.engine.core.dal.dbbroker.DbFacade;
Line 51: import org.ovirt.engine.core.utils.log.Log;
Line 52: import org.ovirt.engine.core.utils.log.LogFactory;
Line 53:
Line 54:
can this new line be removed?
Line 55: public final class ImagesHandler {
Line 56: public static final Guid BlankImageTemplateId = new
Guid("00000000-0000-0000-0000-000000000000");
Line 57: protected static final Log log =
LogFactory.getLog(ImagesHandler.class);
Line 58:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java
Line 54: import org.ovirt.engine.core.dao.StorageDomainStaticDAO;
Line 55: import org.ovirt.engine.core.utils.disks.DiskUtils;
Line 56: import org.ovirt.engine.core.utils.transaction.TransactionMethod;
Line 57: import org.ovirt.engine.core.utils.transaction.TransactionSupport;
Line 58:
can it be removed?
Line 59:
Line 60: @DisableInPrepareMode
Line 61: @NonTransactiveCommandAttribute(forceCompensation = true)
Line 62: public class ImportVmTemplateCommand extends
MoveOrCopyTemplateCommand<ImportVmTemplateParameters>
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/snapshots/SnapshotsManager.java
Line 41: import org.ovirt.engine.core.utils.log.LogFactory;
Line 42: import org.ovirt.engine.core.utils.ovf.OvfManager;
Line 43: import org.ovirt.engine.core.utils.ovf.OvfReaderException;
Line 44: import org.ovirt.engine.core.utils.ovf.VMStaticOvfLogHandler;
Line 45:
please remove the new line
Line 46:
Line 47: /**
Line 48: * The {@link Snapshot} manager is used to easily add/update/remove
snapshots.
Line 49: */
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/disks/DiskUtils.java
Line 7: import org.ovirt.engine.core.utils.log.LogFactory;
Line 8:
Line 9: /** A utility class for disk related functions */
Line 10: public class DiskUtils {
Line 11:
Currently we have ImagesHandler, not saying that it's the best name - but can't
we use this naming convention to avoid confusion?
Line 12: /** The logging category */
Line 13: private static final Log log = LogFactory.getLog(DiskUtils.class);
Line 14:
Line 15: /** The default drive name */
Line 13: private static final Log log = LogFactory.getLog(DiskUtils.class);
Line 14:
Line 15: /** The default drive name */
Line 16: private static final String DefaultDriveName = "1";
Line 17:
can this name be changed from default drive name? seems like 1 isn't actually a
drive name
Line 18: /** The default string to use between a disk alias' prefix and
suffix */
Line 19: public static final String DISK = "_Disk";
Line 20:
Line 21: /**
Line 16: private static final String DefaultDriveName = "1";
Line 17:
Line 18: /** The default string to use between a disk alias' prefix and
suffix */
Line 19: public static final String DISK = "_Disk";
Line 20:
why is it public?
Line 21: /**
Line 22: * Suggests an alias for a disk.
Line 23: * If the disk does not already have an alias, one will be
generated for it.
Line 24: *
Line 36: diskAlias);
Line 37: } else {
Line 38: diskAlias = disk.getDiskAlias();
Line 39: if (StringUtils.isEmpty(diskAlias)) {
Line 40: diskAlias = getDefaultDiskAlias(diskPrefix,
String.valueOf(count));
any reason to log in different severities (here and on line 35)? furthermore,
I'd remove the log prints here or print it in debug only..the disk alias isn't
really that important to log it in my opinion and will just flood the log.
Line 41: log.infoFormat("Disk alias retrieved from the client
is null or empty, the suggested default disk alias to be used is {0}",
Line 42: diskAlias);
Line 43: }
Line 44: }
Line 44: }
Line 45: return diskAlias;
Line 46: }
Line 47:
Line 48: public static String getDefaultDiskAlias(String prefix, String
suffix) {
the compiler will probably turn this into a stringbuilder, i'd just use
stringbuilder here instead in order to not count on the compiler to do that.
Line 49: return prefix + DISK + suffix;
Line 50: }
Line 51:
Line 52: public static boolean setDiskAlias(BaseDisk disk, VM vm) {
--
To view, visit http://gerrit.ovirt.org/9273
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf2a1147c022f4cdb7c5658818667af4b51e6aac
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
