Liron Ar has posted comments on this change.
Change subject: getAllTasksList\Status with spUUID retrieves info only if host
is the SPM
......................................................................
Patch Set 6: (6 inline comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1339:
Line 1340: @Reloadable
Line 1341: @TypeConverterAttribute(Boolean.class)
Line 1342: @DefaultValueAttribute("false")
Line 1343: ImprovedGetAllTasks(505),
I'd suggest to find a different name.
Line 1344:
Line 1345: Invalid(65535);
Line 1346:
Line 1347: private int intValue;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/FeatureSupported.java
Line 101: * Compatibility version to check for.
Line 102: * @return <code>true</code> if get hardware information is
supported for the version, <code>false</code> if it's not.
Line 103: */
Line 104: public static boolean improvedGetAllTasks(Version version) {
Line 105: return supportedInConfig(ConfigValues.ImprovedGetAllTasks,
version);
I'd suggest to find a better name config value/method.
Line 106: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/GetTaskListCommandParameters.java
Line 1: package org.ovirt.engine.core.common.vdscommands;
Line 2:
Line 3: import org.ovirt.engine.core.compat.Guid;
Line 4:
Line 5: public class GetTaskListCommandParameters extends
VdsIdVDSCommandParametersBase {
perhaps we can rename it to more general name and make use of it in other cases
as well
Line 6: public GetTaskListCommandParameters(Guid vdsId) {
Line 7: _vdsId = vdsId;
Line 8: _spUUID = null;
Line 9: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HSMGetAllTasksInfoVDSCommand.java
Line 27: // Check host compatibility, if command is supported by
host
Line 28: HashSet<Version> hostVersions = null;
Line 29: VDS vds =
DbFacade.getInstance().getVdsDao().get(getParameters().getVdsId());
Line 30: Version clusterCompatibility =
vds.getVdsGroupCompatibilityVersion();
Line 31: hostVersions = vds.getSupportedClusterVersionsSet();
1. do we need this? if the host is already in the cluster..it means that it's
already supported, not?
2. why don't we check just by the storage pool version? do we want different
behaviour between the pool hosts?
Line 32: if
(FeatureSupported.improvedGetAllTasks(clusterCompatibility) &&
Line 33: (hostVersions != null) &&
hostVersions.contains(clusterCompatibility)) {
Line 34: _result =
getBroker().getAllTasksInfo(getParameters().getStoragePoolId().toString());
Line 35: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HSMGetAllTasksStatusesVDSCommand.java
Line 36: hostVersions = vds.getSupportedClusterVersionsSet();
Line 37: if
(FeatureSupported.improvedGetAllTasks(clusterCompatibility) &&
Line 38: (hostVersions != null) &&
hostVersions.contains(clusterCompatibility)) {
Line 39: _result =
getBroker().getAllTasksStatuses(getParameters().getStoragePoolId().toString());
Line 40: }
1. perhaps the whole check can be moved to some helper method (from here and
the other classes)
2. please read the related comments in the previous file
Line 41: }
Line 42: if (_result == null) {
Line 43: _result = getBroker().getAllTasksStatuses();
Line 44: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsServerConnector.java
Line 137: public Map<String, Object> getTaskStatus(String taskUUID);
Line 138:
Line 139: public Map<String, Object> getAllTasksStatuses();
Line 140:
Line 141: public Map<String, Object> getAllTasksStatuses(String spUUID);
general - why did you choose to pass spUUID and not a boolean flag for checking
if spm?
Line 142:
Line 143: public Map<String, Object> getAllTasksInfo();
Line 144:
Line 145: public Map<String, Object> getAllTasksInfo(String spUUID);
--
To view, visit http://gerrit.ovirt.org/13450
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff7d8db4e4ad6b3f809085aff7216ac8a457b633
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: liron aravot <liron.ara...@gmail.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
