Moti Asayag has posted comments on this change.
Change subject: core: add FullListVdsCommand support
......................................................................
Patch Set 1: (9 inline comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/FullListVDSCommandParameters.java
Line 14: * seperated Vm Id list)
this isn't accurate, you don't pass a comma delimited list of id, but a list of
ids as string (the interface with VDSM is array of VMs)
Line 16: */
please remove the jaxb annotations - they aren't required.
Line 19: public class FullListVDSCommandParameters extends
VdsIdAndVdsVDSCommandParametersBase {
I'm not sure the FullList... serves well the purpose of this parameter class,
perhaps just VmListVDSCommandParameters ?
Line 31:
please move the c'tor below prior to the getters.
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FullListVdsCommand.java
Line 11: @Logged(executionLevel = LogLevel.TRACE)
was the Trace put here for debug and left ?
Line 12: public class FullListVdsCommand<P extends
VdsIdAndVdsVDSCommandParametersBase> extends VdsBrokerCommand<P> {
* Is there a change you intend to call this method with
VdsIdAndVdsVDSCommandParametersBase ? If no, replace the class signature with:
public class FullListVdsCommand<P extends FullListVDSCommandParameters> extends
VdsBrokerCommand<P>
* Same issue about command name.
Line 20: protected void ExecuteVdsBrokerCommand() {
once applied changing the class signature, this casting can be avoided.
Update should be done in VdsUpdateRunTimeInfo.getVmInfo where creating the
command instance.
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/IVdsServer.java
Line 32:
the name miss-lead. The value as I see it from the invocation of it is:
getBroker().list("true", vmIdsArray);
I think that changing the parameter name to isFull (despite its string type)
will be more clear.
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsServerConnector.java
Line 27: public Map<String, Object> list(String mode, String[] vmIds);
same regarding name mode vs isFull
--
To view, visit http://gerrit.ovirt.org/2406
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieda51634469d3925a9afa89970d60a5427144e98
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
