Maor Lipchuk has posted comments on this change.
Change subject: getAllTasksList\Status with spUUID retrieves info only if host
is the SPM
......................................................................
Patch Set 4: (11 inline comments)
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/SPMGetAllTasksInfoVDSCommand.java
Line 12:
Line 13: public class SPMGetAllTasksInfoVDSCommand<P extends
IrsBaseVDSCommandParameters> extends IrsBrokerCommand<P> {
Line 14:
Line 15: private static Log log =
LogFactory.getLog(SPMGetAllTasksInfoVDSCommand.class);
Line 16: private TaskInfoListReturnForXmlRpc _result;
please avoid using _ at the beginning of the variable name
Line 17:
Line 18: public SPMGetAllTasksInfoVDSCommand(P parameters) {
Line 19: super(parameters);
Line 20: }
Line 30: ProceedProxyReturnValue();
Line 31: setReturnValue(ParseTaskInfoList(_result.TaskInfoList));
Line 32: }
Line 33:
Line 34: private static final String VERB_KEY = "verb";
Perhaps it would be more compatible to delare this verb at IrsProperties
Line 35:
Line 36: protected java.util.ArrayList<AsyncTaskCreationInfo>
ParseTaskInfoList(Map<String, Map<String, String>> taskInfoList) {
Line 37: try {
Line 38: java.util.ArrayList<AsyncTaskCreationInfo> result = new
java.util.ArrayList<AsyncTaskCreationInfo>(
Line 32: }
Line 33:
Line 34: private static final String VERB_KEY = "verb";
Line 35:
Line 36: protected java.util.ArrayList<AsyncTaskCreationInfo>
ParseTaskInfoList(Map<String, Map<String, String>> taskInfoList) {
1) please replace java.util.ArrayList with List
2) Use private static instead of protected
Line 37: try {
Line 38: java.util.ArrayList<AsyncTaskCreationInfo> result = new
java.util.ArrayList<AsyncTaskCreationInfo>(
Line 39: taskInfoList.size());
Line 40: for (java.util.Map.Entry<String, java.util.Map<String,
String>> entry : taskInfoList.entrySet()) {
Line 34: private static final String VERB_KEY = "verb";
Line 35:
Line 36: protected java.util.ArrayList<AsyncTaskCreationInfo>
ParseTaskInfoList(Map<String, Map<String, String>> taskInfoList) {
Line 37: try {
Line 38: java.util.ArrayList<AsyncTaskCreationInfo> result = new
java.util.ArrayList<AsyncTaskCreationInfo>(
Better to use List interface as a parameter instead ArrayList.
Also avoid using the full path in the code, and simply import List and ArrayList
Line 39: taskInfoList.size());
Line 40: for (java.util.Map.Entry<String, java.util.Map<String,
String>> entry : taskInfoList.entrySet()) {
Line 41: Guid taskID = new Guid(entry.getKey());
Line 42: Map<String, String> taskInfo = entry.getValue();
Line 36: protected java.util.ArrayList<AsyncTaskCreationInfo>
ParseTaskInfoList(Map<String, Map<String, String>> taskInfoList) {
Line 37: try {
Line 38: java.util.ArrayList<AsyncTaskCreationInfo> result = new
java.util.ArrayList<AsyncTaskCreationInfo>(
Line 39: taskInfoList.size());
Line 40: for (java.util.Map.Entry<String, java.util.Map<String,
String>> entry : taskInfoList.entrySet()) {
You already imported java.util.Map you can just use here Map.Entry or even
better simply import it explicitly
Line 41: Guid taskID = new Guid(entry.getKey());
Line 42: Map<String, String> taskInfo = entry.getValue();
Line 43: AsyncTaskCreationInfo task = ParseTaskInfo(taskInfo,
taskID);
Line 44: if (task != null) {
Line 51: throw exp;
Line 52: }
Line 53: }
Line 54:
Line 55: protected AsyncTaskCreationInfo ParseTaskInfo(Map<String, String>
taskInfo, Guid taskID) {
1) /s/ParseTaskInfo/parseTaskInfo
2) switch protected to private static
Line 56: try {
Line 57: String deTaskType = taskInfo.get(VERB_KEY);
Line 58: AsyncTaskType taskType;
Line 59: try {
Line 53: }
Line 54:
Line 55: protected AsyncTaskCreationInfo ParseTaskInfo(Map<String, String>
taskInfo, Guid taskID) {
Line 56: try {
Line 57: String deTaskType = taskInfo.get(VERB_KEY);
What deTaskType means?
Line 58: AsyncTaskType taskType;
Line 59: try {
Line 60: taskType = AsyncTaskType.valueOf(deTaskType);
Line 61: } catch (Exception e) {
Line 57: String deTaskType = taskInfo.get(VERB_KEY);
Line 58: AsyncTaskType taskType;
Line 59: try {
Line 60: taskType = AsyncTaskType.valueOf(deTaskType);
Line 61: } catch (Exception e) {
How can we get other exception then NullPointerException, this could only
happen if VDSM will start to run a new type of tasks which engine does not
aware of.
Why not use catch only for NullPointerException
At any chance we can get IllegalArgumentException, then probably at least you
can catch RuntimeException instead exception.
Line 62: taskType = AsyncTaskType.unknown;
Line 63: log.warn("The task type in the vdsm response is " +
deTaskType
Line 64: + " and does not appear in the AsyncTaskType
enum");
Line 65: }
Line 59: try {
Line 60: taskType = AsyncTaskType.valueOf(deTaskType);
Line 61: } catch (Exception e) {
Line 62: taskType = AsyncTaskType.unknown;
Line 63: log.warn("The task type in the vdsm response is " +
deTaskType
Better to use warnFormat as follow:
log.warnFormat("The task type in the vdsm response is {0} and does not appear
in the AsyncTaskType enum", deTaskType)
Line 64: + " and does not appear in the AsyncTaskType
enum");
Line 65: }
Line 66:
Line 67: AsyncTaskCreationInfo tempVar = new
AsyncTaskCreationInfo();
Line 66:
Line 67: AsyncTaskCreationInfo tempVar = new
AsyncTaskCreationInfo();
Line 68: tempVar.setTaskID(taskID);
Line 69: tempVar.setTaskType(taskType);
Line 70: AsyncTaskCreationInfo task = tempVar;
I'm not sure I follow, why creating a tempVar and assign it to a new task and
not simply use the one you already created.
Also you could simply write here :
return new AsyncTaskCreationInfo(taskId,TaskType,null);
Line 71:
Line 72: return task;
Line 73: }
Line 74:
Line 71:
Line 72: return task;
Line 73: }
Line 74:
Line 75: catch (RuntimeException e) {
How can we get here?
Line 76: log.error(String.format(
Line 77: "Could not parse single task info: '%1$s'
(possibly specific task should not be monitored).",
Line 78: taskInfo), e);
Line 79: return null;
--
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: liron aravot <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
