On 01-09-2016 13:21, Aline Manera wrote:
On 09/01/2016 01:08 PM, Lucio Correia wrote:
On 01-09-2016 10:45, Aline Manera wrote:
On 08/31/2016 06:06 PM, Lucio Correia wrote:
* Add plugin and log_id attributes to AsyncTask * Create _log() function to log AsyncTask exit status * Create auxiliary method to save log entry ID for a task * Handle WokException separately in order to add it in translatable form to user log
Signed-off-by: Lucio Correia <luciojhc@linux.vnet.ibm.com> --- src/wok/asynctask.py | 46 ++++++++++++++++++++++++++++++++++++++++------ src/wok/i18n.py | 2 ++ 2 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/src/wok/asynctask.py b/src/wok/asynctask.py index 021b76c..633c96f 100644 --- a/src/wok/asynctask.py +++ b/src/wok/asynctask.py @@ -25,10 +25,13 @@ import time import traceback import uuid
- -from wok.exception import InvalidOperation, OperationFailed +from wok.exception import InvalidOperation, OperationFailed, WokException +from wok.reqlogger import RequestRecord, ASYNCTASK_REQUEST_METHOD +from wok.utils import get_plugin_from_uri
+MSG_FAILED = 'WOKASYNC0002L' +MSG_SUCCESS = 'WOKASYNC0001L' tasks_queue = {}
@@ -43,27 +46,54 @@ def clean_old_tasks(): task.remove()
+def save_request_log_id(log_id, task_id): + tasks_queue[task_id].log_id = log_id + +
You can use "tasks_queue[task_id].log_id = log_id " directly in the code. You are replacing one line code by one function. I don't see much gain on it specially while reviewing the code where you can grab the function to simply to understand it.
Yes I could. But I did prefer to create this function due to code maintenance. This is the module that knows how to deal with tasks, so if that changes (we just did that moving tasks from objstore to memory), we change only code here and not through all the code.
Ok. I got your point.
It's also good to avoid another module to access a control structure like tasks_queue directly and do some bad stuff.
tasks_queue is a global variable =)
class AsyncTask(object): def __init__(self, target_uri, fn, opaque=None, kill_cb=None): + # task info self.id = str(uuid.uuid1()) self.target_uri = target_uri self.fn = fn self.kill_cb = kill_cb + self.log_id = None
+ self.plugin = get_plugin_from_uri(target_uri)
The target_uri may be anything! Usually on Kimchi, we have a pattern to be the the plugin base URI + resource URI but it may be not true to every plugin. I suggest to get that info from cherrypy
I see. I will try to use cherrypy, but I'm not sure it will work due to my past work with user logs and requests.
It does not work, any other idea? In worst case I keep it based on target_uri for now and add a TODO note to fix it later? ====================================================================== ERROR: test_async_tasks (test_tasks.AsyncTaskTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "test_tasks.py", line 61, in test_async_tasks taskid = AsyncTask('', self._quick_op, 'Hello').id File "/home/dev/wok/src/wok/asynctask.py", line 61, in __init__ self.plugin = cherrypy.request.app.root.domain AttributeError: 'NoneType' object has no attribute 'root'
+ self.timestamp = time.time() + + # task context self.status = 'running' self.message = 'The request is being processing.' - self.timestamp = time.time() self._cp_request = cherrypy.serving.request self.thread = threading.Thread(target=self._run_helper, args=(opaque, self._status_cb)) self.thread.setDaemon(True) self.thread.start() + # let's prevent memory leak in tasks_queue clean_old_tasks() tasks_queue[self.id] = self
- def _status_cb(self, message, success=None):
+ def _log(self, code, status, exception=None): + RequestRecord( + {'code': code, 'params': {'target_uri': self.target_uri}}, + exception, + self.log_id, + app=self.plugin, + req=ASYNCTASK_REQUEST_METHOD, + status=status, + user='', + ip='', + ).log() +
Isn't this function the same you created on Patch 1? Can't you reuse the other one?
Yes, now that that function is in control/utils.py I believe I can use it here.
+ def _status_cb(self, message, success=None, exception=None): if success is not None: - self.status = 'finished' if success else 'failed' + if success: + self._log(MSG_SUCCESS, 200) + self.status = 'finished' + else: + self._log(MSG_FAILED, 400, exception) + self.status = 'failed'
if message.strip(): self.message = message @@ -72,10 +102,14 @@ class AsyncTask(object): cherrypy.serving.request = self._cp_request try: self.fn(cb, opaque) + except WokException, e: + cherrypy.log.error_log.error("Error in async_task %s " % self.id) + cherrypy.log.error_log.error(traceback.format_exc()) + cb(e.message, success=False, exception=e) except Exception, e: cherrypy.log.error_log.error("Error in async_task %s " % self.id) cherrypy.log.error_log.error(traceback.format_exc()) - cb(e.message, False) + cb(e.message, success=False)
def remove(self): try: diff --git a/src/wok/i18n.py b/src/wok/i18n.py index 7ba7c24..4893be7 100644 --- a/src/wok/i18n.py +++ b/src/wok/i18n.py @@ -59,6 +59,8 @@ messages = { "WOKPROXY0001E": _("Unable to (re)start system's nginx.service. Details: '%(error)s'"),
# These messages (ending with L) are for user log purposes + "WOKASYNC0001L": _("Successfully completed task '%(target_uri)s'"), + "WOKASYNC0002L": _("Failed to complete task '%(target_uri)s'"), "WOKCOL0001L": _("Request made on collection"), "WOKRES0001L": _("Request made on resource"), "WOKROOT0001L": _("User '%(username)s' login"),
-- Lucio Correia Software Engineer IBM LTC Brazil