Change in vdsm[master]: jsonrpc: executor based thread factory

nsoffer at redhat.com nsoffer at redhat.com
Fri Jul 17 14:12:13 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: jsonrpc: executor based thread factory
......................................................................


Patch Set 1:

(7 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 :-)
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 single scheduler per process, it can easily scale to 1000's of timeouts.

So better create one scheduler when starting vdsm, and use it fro both virt executor and jsonrpc, and probably other places.
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 the server and reactor.
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 can dispatch function or objects implementing  _call__. Objects are required only if you want to add arguments for the call.
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 exception handling is not good enough, improve it instead of introducing such wrappers.
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.
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)
Use executor.dispatch as the threadFactory.
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