Moti Asayag has posted comments on this change.

Change subject: core: add support for tunnel migration
......................................................................


Patch Set 3: (5 inline comments)

....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBase.java
Line 151:     private boolean allowConsoleReconnect;
Line 152: 
Line 153:     /**
Line 154:      * if this field is null then value should be taken from cluster
Line 155:      */
hibernate annotations could be spared.
Line 156:     @Column(name = "tunnel_migration")
Line 157:     private Boolean tunnelMigration;
Line 158: 
Line 159:     /**


Line 811:     public void setOvfVersion(String ovfVersion) {
Line 812:         this.ovfVersion = ovfVersion;
Line 813:     }
Line 814: 
Line 815:     public Boolean isTunnelMigration() {
since this method returns an object and not a primitive boolean, I'd rather 
call if getTunnelMigration so if used from if statements the developer will be 
aware of the possibility the attribute wasn't initialized and consider checking 
if it value is null.
Line 816:         return tunnelMigration;
Line 817:     }
Line 818: 
Line 819:     public void setTunnelMigration(Boolean value) {


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java
Line 1493:     public boolean isNotRunning() {
Line 1494:         return getStatus().isNotRunning();
Line 1495:     }
Line 1496: 
Line 1497:     public Boolean isTunnelMigration() {
Please change method name to getTunnelMigration().

See blown explanation on VmBase.
Line 1498:         return vmStatic.isTunnelMigration();
Line 1499:     }
Line 1500: 
Line 1501:     public void setTunnelMigration(Boolean value) {


....................................................
File 
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/MigrateBrokerVDSCommand.java
Line 20:         migrationInfo.put(VdsProperties.src, String.format("%1$s", 
parameters.getSrcHost()));
Line 21:         migrationInfo.put(VdsProperties.dst, String.format("%1$s", 
parameters.getDstHost()));
Line 22:         migrationInfo.put(VdsProperties.method, migMethod);
Line 23:         if (parameters.getTunnelMigration() != null) {
Line 24:             migrationInfo.put(VdsProperties.tunneled, 
String.valueOf(parameters.getTunnelMigration()));
since you already verified parameters.getTunnelMigration() is not null you can 
use parameters.getTunnelMigration().toString()
Line 25:         }
Line 26:     }
Line 27: 
Line 28:     @Override


....................................................
File 
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsProperties.java
Line 252:     public static final String offline = "offline";
Line 253:     public static final String online = "online";
Line 254:     public static final String domains = "storageDomains";
Line 255:     public static final String hooks = "hooks";
Line 256:     public static final String tunneled = "tunneled";
Please change constant name to upper-case so it complies with java naming 
convention.
Line 257: 
Line 258:     // storage domains
Line 259:     public static final String code = "code";
Line 260:     public static final String lastCheck = "lastCheck";


--
To view, visit http://gerrit.ovirt.org/11062
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6fa684df7376f8470fe729d590ce0e63d39c4d1e
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to