Martin Sivák has posted comments on this change. Change subject: add errors to the xmlrpc response ......................................................................
Patch Set 2: Code-Review-1 (4 comments) Most of the comments are just nitpicks. Also clean the Created by comments in java files. And if you have time :) think about writing a LoggingHandler that we could use to transparently report messages to the engine: - proxy would just call log.info/err/warn - the xmlrpc server will interrogate the handler for messages just before sending the result back We could then configure the levels and use the common API. I think that would be quite neat. http://gerrit.ovirt.org/#/c/24683/2/src/ovirtscheduler/request_handler.py File src/ovirtscheduler/request_handler.py: Line 167: Line 168: # handle missing filters Line 169: for f in missing_f: Line 170: log_adapter.warning("Filter requested but was not found: %s" % f) Line 171: #raise RuntimeError("plugin not found: " + f) I would probably kill the raise line... Line 172: result.pluginError(f, "plugin not found: '%s'" % f) Line 173: Line 174: # Prepare a generator "list" of runners Line 175: filterRunners = [ Line 292: log_adapter.info("got request: %s" % balance) Line 293: Line 294: if balance not in self._balancers: Line 295: log_adapter.warning( Line 296: "Load balance requested but was not found: %s" % balance) Please extract the string and convert it into variable that gets used in both calls. Line 297: result.pluginError(balance, Line 298: "Load balance requested but was not found: %s" Line 299: % balance) Line 300: return result http://gerrit.ovirt.org/#/c/24683/2/src/ovirtscheduler/result.py File src/ovirtscheduler/result.py: Line 20: ERRORS = "errors" Line 21: Line 22: Line 23: class Result(object): Line 24: def __init__(self): You could have used the standard class dictionary and vars() to convert it into dictionary. Or namedtuple since you have only two attributes. Line 25: self._result = {} Line 26: self._result["result_code"] = RESULT_OK # no error so far.. Line 27: self._result["result"] = [] Line 28: Line 32: def pluginError(self, pluginName, errorMsg): Line 33: self._result["result_code"] = RESULT_ERROR Line 34: pluginErrors = self._result.get(PLUGIN_ERRORS, {}) Line 35: pluginErrorMessages = pluginErrors.get(pluginName, []) Line 36: pluginErrorMessages.append(errorMsg) Comment here explaining the assignment might be nice. Line 37: pluginErrors[pluginName] = pluginErrorMessages Line 38: self._result[PLUGIN_ERRORS] = pluginErrors Line 39: Line 40: def error(self, errorMsg): -- To view, visit http://gerrit.ovirt.org/24683 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I765acfcc65555c046749ed51a21023418d870a35 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-scheduler-proxy Gerrit-Branch: master Gerrit-Owner: Jiří Moskovčák <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Jiří Moskovčák <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
