Roy Golan has posted comments on this change.

Change subject: core: Disable balloon by default - Patch 1 of 2
......................................................................


Patch Set 8: Code-Review-1

(3 comments)

-1 mainly for the osRepository API verb 

could the topic be prefixed with ppc so it would be easier to track?

....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/osinfo/OsRepository.java
Line 102:     /**
Line 103:      * @return map (osId -> compatibility version -> Boolean) that 
indicates balloon disabled for all OSs and
Line 104:      * compatibility versions
Line 105:      */
Line 106:     public Map<Integer, Map<Version, Boolean>> 
getBalloonDisabledByDefaultMatrix();
change name too getBalloonSupportMap like nicHotPlugSupport etc
Line 107: 
Line 108:     /**
Line 109:      * Checks if is recommended disable the OS balloon.
Line 110:      * @param osId


Line 110:      * @param osId
Line 111:      * @param version
Line 112:      * @return an boolean
Line 113:      */
Line 114:     public boolean isBalloonDisabledByDefault(int osId, Version 
version);
again a much better verb is isBallonEnabled . the word "default" here is not 
adding information I guess, unless I'm missing the point.

its also easier to consume the api - 
  
 (osRepository.isBalloonEnabled(x,y)) {
     ....  
 } 


instead of 


(! osRepository.isBalloonDisabledByDefault(x,y)) {
    ....
}
Line 115: 
Line 116:     /**
Line 117:      * Checks if that OS network devices support hotplug.
Line 118:      * @param osId


....................................................
File packaging/conf/osinfo-defaults.properties
Line 55: 
Line 56: os.other.devices.disk.hotpluggableInterfaces.value = VirtIO_SCSI, 
VirtIO
Line 57: os.other.devices.disk.hotpluggableInterfaces.value.3.0 =
Line 58: 
Line 59: os.other.devices.balloonDisabledByDefault.value = false
probably meant  enabledByDefault.value = false ? :) or I'm missing something.

also I want this to adhere to the general devices.{device}.properties 
name-sapce 

 os.{id}.devices.balloon.enabled.value = false
 os.{id}.devices.balloon.otherProps.value = x
Line 60: os.other.devices.audio.value = ich6
Line 61: # See VmInterfaceType.java
Line 62: os.other.devices.network.value =  rtl8139, e1000, pv
Line 63: os.other.devices.network.hotplugSupport.value = true


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b048b3d54e5e1732f8a13de8c9d4c1df4b6d8e5
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gustavo Frederico Temple Pedrosa <gustavo.pedr...@eldorado.org.br>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Gustavo Frederico Temple Pedrosa 
<gustavo.pedr...@eldorado.org.br>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Leonardo Bianconi <leonardo.bianc...@eldorado.org.br>
Gerrit-Reviewer: Martin Sivák <msi...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Vitor de Lima <vitor.l...@eldorado.org.br>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-Reviewer: ofri masad <oma...@redhat.com>
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to