Change in vdsm[master]: jsonrpc: executor based thread factory
piotr.kliczewski at gmail.com
piotr.kliczewski at gmail.com
Fri Jul 17 14:22:27 UTC 2015
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory
......................................................................
Patch Set 1:
(8 comments)
https://gerrit.ovirt.org/#/c/43759/1/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:
Line 204:
Line 205: ('worker_threads', '8',
Line 206: 'Number of worker threads to serve jsonrpc server.'),
Line 207:
Line 208: ('tasks_per_worker', '100',
> Why do you want to queue 800 requests? I would use 10 here :-)
Done
Line 209: 'Max number of tasks which can be queued per workers.'),
Line 210: ]),
Line 211:
Line 212: # Section: [mom]
https://gerrit.ovirt.org/#/c/43759/1/vdsm/rpc/bindingjsonrpc.py
File vdsm/rpc/bindingjsonrpc.py:
Line 31: _TASKS = _THREADS * _TASK_PER_WORKER
Line 32:
Line 33:
Line 34: _scheduler = schedule.Scheduler(name="jsonrpc.Scheduler",
Line 35: clock=monotonic_time)
> We already have a scheduler for virt periodic tasks, I think we can use sin
Done
Line 36:
Line 37: _executor = executor.Executor(name="jsonrpc.Executor",
Line 38: workers_count=_THREADS,
Line 39: max_tasks=_TASKS,
Line 36:
Line 37: _executor = executor.Executor(name="jsonrpc.Executor",
Line 38: workers_count=_THREADS,
Line 39: max_tasks=_TASKS,
Line 40: scheduler=_scheduler)
> The scheduler and the executor belongs to BindingJsonRpc class, just like t
Done
Line 41:
Line 42:
Line 43: class _RequestWorker(object):
Line 44:
Line 39: max_tasks=_TASKS,
Line 40: scheduler=_scheduler)
Line 41:
Line 42:
Line 43: class _RequestWorker(object):
> This class is not needed. Executor is supporting callable objects so you ca
Done
Line 44:
Line 45: _log = logging.getLogger("jsonrpc._RequestWorker")
Line 46:
Line 47: def __init__(self, func):
Line 50: def __call__(self):
Line 51: try:
Line 52: self._func()
Line 53: except Exception:
Line 54: self._log.exception("%s processing request failed", self._func)
> Executor handle exceptions in function, we don't need this. If executor exc
Done
Line 55:
Line 56:
Line 57: def _executorFactory(func):
Line 58: _executor.dispatch(_RequestWorker(func))
Line 54: self._log.exception("%s processing request failed", self._func)
Line 55:
Line 56:
Line 57: def _executorFactory(func):
Line 58: _executor.dispatch(_RequestWorker(func))
> This function is not needed.
Done
Line 59:
Line 60:
Line 61: class BindingJsonRpc(object):
Line 62: log = logging.getLogger('BindingJsonRpc')
Line 64: def __init__(self, bridge, subs, timeout):
Line 65: _scheduler.start()
Line 66: _executor.start()
Line 67:
Line 68: self._server = JsonRpcServer(bridge, timeout, _executorFactory)
> But note that the executor may raise TooManyTasks - you must handle this ex
Done
Line 69: self._reactor = StompReactor(subs)
Line 70: self.startReactor()
Line 71:
Line 72: def add_socket(self, reactor, client_socket):
Line 64: def __init__(self, bridge, subs, timeout):
Line 65: _scheduler.start()
Line 66: _executor.start()
Line 67:
Line 68: self._server = JsonRpcServer(bridge, timeout, _executorFactory)
> Use executor.dispatch as the threadFactory.
Done
Line 69: self._reactor = StompReactor(subs)
Line 70: self.startReactor()
Line 71:
Line 72: def add_socket(self, reactor, client_socket):
--
To view, visit https://gerrit.ovirt.org/43759
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I56b307633a8bf7e4aad8f87cc97a4129c9ed0970
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list