Piotr Kliczewski has uploaded a new change for review.
Change subject: jsonrpc: executor based thread factory ......................................................................
jsonrpc: executor based thread factory
Creating new thread for every request is not efficient so we introduce usage of the executor for request processing.
Change-Id: I56b307633a8bf7e4aad8f87cc97a4129c9ed0970 Signed-off-by: pkliczewski piotr.kliczewski@gmail.com --- M lib/vdsm/config.py.in M vdsm/rpc/bindingjsonrpc.py 2 files changed, 49 insertions(+), 5 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/59/43759/1
diff --git a/lib/vdsm/config.py.in b/lib/vdsm/config.py.in index 3221aab..bffa949 100644 --- a/lib/vdsm/config.py.in +++ b/lib/vdsm/config.py.in @@ -201,6 +201,12 @@
('connection_stats_timeout', '3600', 'Time in seconds defining how frequently we log transport stats'), + + ('worker_threads', '8', + 'Number of worker threads to serve jsonrpc server.'), + + ('tasks_per_worker', '100', + 'Max number of tasks which can be queued per workers.'), ]),
# Section: [mom] diff --git a/vdsm/rpc/bindingjsonrpc.py b/vdsm/rpc/bindingjsonrpc.py index 80e3388..ca15fde 100644 --- a/vdsm/rpc/bindingjsonrpc.py +++ b/vdsm/rpc/bindingjsonrpc.py @@ -19,18 +19,53 @@ from yajsonrpc import JsonRpcServer from yajsonrpc.stompreactor import StompReactor
+from vdsm import executor +from vdsm import schedule +from vdsm.config import config +from vdsm.utils import monotonic_time
-def _simpleThreadFactory(func): - t = threading.Thread(target=func) - t.setDaemon(False) - t.start() + +# TODO test what should be the default values +_THREADS = config.getint('vars', 'worker_threads') +_TASK_PER_WORKER = config.getint('vars', 'tasks_per_worker') +_TASKS = _THREADS * _TASK_PER_WORKER + + +_scheduler = schedule.Scheduler(name="jsonrpc.Scheduler", + clock=monotonic_time) + +_executor = executor.Executor(name="jsonrpc.Executor", + workers_count=_THREADS, + max_tasks=_TASKS, + scheduler=_scheduler) + + +class _RequestWorker(object): + + _log = logging.getLogger("jsonrpc._RequestWorker") + + def __init__(self, func): + self._func = func + + def __call__(self): + try: + self._func() + except Exception: + self._log.exception("%s processing request failed", self._func) + + +def _executorFactory(func): + _executor.dispatch(_RequestWorker(func))
class BindingJsonRpc(object): log = logging.getLogger('BindingJsonRpc')
def __init__(self, bridge, subs, timeout): - self._server = JsonRpcServer(bridge, timeout, _simpleThreadFactory) + _scheduler.start() + _executor.start() + + self._server = JsonRpcServer(bridge, timeout, _executorFactory) self._reactor = StompReactor(subs) self.startReactor()
@@ -60,3 +95,6 @@ def stop(self): self._server.stop() self._reactor.stop() + + _scheduler.stop() + _executor.stop()
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 1: Verified+1
Verified by using the latest engine and vdsm. Steps include: installing new host, configuring NFS storage, importing a template and running a vm.
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):
Nir Soffer has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/1/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
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.
But note that the executor may raise TooManyTasks - you must handle this exception in the jsonrpc server, and maybe send an error to the client, about server being too busy. This also require handling such errors in the engine side. Line 69: self._reactor = StompReactor(subs) Line 70: self.startReactor() Line 71: Line 72: def add_socket(self, reactor, client_socket):
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):
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/1/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
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
Thinking about it we can't explicitly send this error to the client due to backward compatibility.
With current code the exception is send to the engine as internal error containing information about the issue. The client (engine) takes different decisions for different commands if there is a failure.
We need to set proper pool size to mitigate failures based on TooManyTasks. Line 69: self._reactor = StompReactor(subs) Line 70: self.startReactor() Line 71: Line 72: def add_socket(self, reactor, client_socket):
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 2:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 2: Verified+1
Fixed comments. Patch split into 2. Verified by installing new host and adding NFS storage domain.
Nir Soffer has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/1/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
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)
Thinking about it we can't explicitly send this error to the client due to
It does not make sense to queue requests for ever. We should find a way to fail the requests that will be compatible with old engines. What will happen if we introduce a new "busy" error, and the engine side does not know this error?
Do we have a standard error (stomp?) we can use for this? Line 69: self._reactor = StompReactor(subs) Line 70: self.startReactor() Line 71: Line 72: def add_socket(self, reactor, client_socket):
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/1/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
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)
It does not make sense to queue requests for ever. We should find a way to
No there are no stomp specific errors.
We attempted to run threadFactory in following way:
try: self._threadFactory(partial(self._serveRequest, ctx, request)) except Exception as e: self.log.exception("could not allocate request thread") ctx.requestDone( JsonRpcResponse( None, JsonRpcInternalError( str(e) ), request.id ) )
and TooManyTasks will be processed in the same way now.
The engine tries to understand what happened (assuming non-zero code) based on error code. There is specific error handling logic which maps error code to specific exception (BrokerCommandBase#proceedProxyReturnValue) and handling of a specific exception is done of each Command(verb) level. Line 69: self._reactor = StompReactor(subs) Line 70: self.startReactor() Line 71: Line 72: def add_socket(self, reactor, client_socket):
Nir Soffer has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/1/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
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)
No there are no stomp specific errors.
Makes sense. Line 69: self._reactor = StompReactor(subs) Line 70: self.startReactor() Line 71: Line 72: def add_socket(self, reactor, client_socket):
Nir Soffer has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/2/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 24: Line 25: Line 26: # TODO test what should be the default values Line 27: _THREADS = config.getint('vars', 'worker_threads') Line 28: _TASK_PER_WORKER = config.getint('vars', 'tasks_per_worker') This is the configuration of the jsonrpc server - we should have a section for this in the config, instead of dumping everything in vars. Line 29: _TASKS = _THREADS * _TASK_PER_WORKER Line 30: Line 31: Line 32: class BindingJsonRpc(object):
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/2/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 24: Line 25: Line 26: # TODO test what should be the default values Line 27: _THREADS = config.getint('vars', 'worker_threads') Line 28: _TASK_PER_WORKER = config.getint('vars', 'tasks_per_worker')
This is the configuration of the jsonrpc server - we should have a section
OK. will update. Line 29: _TASKS = _THREADS * _TASK_PER_WORKER Line 30: Line 31: Line 32: class BindingJsonRpc(object):
Nir Soffer has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/2/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 24: Line 25: Line 26: # TODO test what should be the default values Line 27: _THREADS = config.getint('vars', 'worker_threads') Line 28: _TASK_PER_WORKER = config.getint('vars', 'tasks_per_worker')
OK. will update.
Since you plan to an executor for xmlrpc, we can consider having [rpc] section for specifying the number of threads/tasks. Line 29: _TASKS = _THREADS * _TASK_PER_WORKER Line 30: Line 31: Line 32: class BindingJsonRpc(object):
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 2:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/2/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 24: Line 25: Line 26: # TODO test what should be the default values Line 27: _THREADS = config.getint('vars', 'worker_threads') Line 28: _TASK_PER_WORKER = config.getint('vars', 'tasks_per_worker')
Since you plan to an executor for xmlrpc, we can consider having [rpc] sect
Good idea. Line 29: _TASKS = _THREADS * _TASK_PER_WORKER Line 30: Line 31: Line 32: class BindingJsonRpc(object):
Francesco Romani has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 2:
Seems fine once Nir's comments in v2 are addressed
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 2:
Will fix Nir's comments
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 3:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 4: Code-Review-1
Need to update config.py.in.
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 6:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 6: Verified+1
Verified by host deploying vdsm and seeing that there are no issues with communication.
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 7:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Nir Soffer has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
-1 for visibility
https://gerrit.ovirt.org/#/c/43759/7/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 37: workers_count=_THREADS, Line 38: max_tasks=_TASKS, Line 39: scheduler=scheduler) Line 40: Line 41: self._server = JsonRpcServer(bridge, timeout, self._executor.dispatch) dispatch may raise executor.TooManyTasks - is JsonRpcServer handling this exception correctly? Line 42: self._reactor = StompReactor(subs) Line 43: self.startReactor() Line 44: Line 45: def add_socket(self, reactor, client_socket):
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/7/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 37: workers_count=_THREADS, Line 38: max_tasks=_TASKS, Line 39: scheduler=scheduler) Line 40: Line 41: self._server = JsonRpcServer(bridge, timeout, self._executor.dispatch)
dispatch may raise executor.TooManyTasks - is JsonRpcServer handling this e
It will send it back to the client containing all the information provided. It is up to the client to decide what to do with it. Line 42: self._reactor = StompReactor(subs) Line 43: self.startReactor() Line 44: Line 45: def add_socket(self, reactor, client_socket):
Nir Soffer has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/7/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 37: workers_count=_THREADS, Line 38: max_tasks=_TASKS, Line 39: scheduler=scheduler) Line 40: Line 41: self._server = JsonRpcServer(bridge, timeout, self._executor.dispatch)
It will send it back to the client containing all the information provided.
Do we have a test for this? if not, maybe add it in this patch? Line 42: self._reactor = StompReactor(subs) Line 43: self.startReactor() Line 44: Line 45: def add_socket(self, reactor, client_socket):
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/7/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 37: workers_count=_THREADS, Line 38: max_tasks=_TASKS, Line 39: scheduler=scheduler) Line 40: Line 41: self._server = JsonRpcServer(bridge, timeout, self._executor.dispatch)
Do we have a test for this? if not, maybe add it in this patch?
Will add. Line 42: self._reactor = StompReactor(subs) Line 43: self.startReactor() Line 44: Line 45: def add_socket(self, reactor, client_socket):
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 8:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 8: Verified+1
Verified by host deploying and seeing that communication works.
Nir Soffer has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 8: Code-Review+1
Lets get Francesco opinion.
Francesco Romani has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 8: Code-Review+1
we add a bit of state to clientIF, but this seems a necessary evil. Overall I like this patch, and I don't see any problem with it.
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 9:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 10:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Yaniv Bronhaim has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 10: Code-Review+1
Yaniv Bronhaim has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 10:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/10/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 22: from vdsm import executor Line 23: from vdsm.config import config Line 24: Line 25: Line 26: # TODO test what should be the default values I'd write this todo in the config.py file Line 27: _THREADS = config.getint('rpc', 'worker_threads') Line 28: _TASK_PER_WORKER = config.getint('rpc', 'tasks_per_worker') Line 29: _TASKS = _THREADS * _TASK_PER_WORKER Line 30:
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 10:
(1 comment)
https://gerrit.ovirt.org/#/c/43759/10/vdsm/rpc/bindingjsonrpc.py File vdsm/rpc/bindingjsonrpc.py:
Line 22: from vdsm import executor Line 23: from vdsm.config import config Line 24: Line 25: Line 26: # TODO test what should be the default values
I'd write this todo in the config.py file
We need to have a bit of perf testing for it. We just need to remember that it needs to be done. Line 27: _THREADS = config.getint('rpc', 'worker_threads') Line 28: _TASK_PER_WORKER = config.getint('rpc', 'tasks_per_worker') Line 29: _TASKS = _THREADS * _TASK_PER_WORKER Line 30:
Dan Kenigsberg has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 10: Code-Review-1
so sorry:
CONFLICT (content): Merge conflict in lib/vdsm/config.py.in
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 11:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 11: Verified+1
Rebase only, no code changes. Copying verification flag from previous patch set.
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 12:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 12: Verified+1
Rebase glitch in previous patch. Copying verification flag.
Dan Kenigsberg has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 12: Code-Review+2
copying score
Dan Kenigsberg has submitted this change and it was merged.
Change subject: jsonrpc: executor based thread factory ......................................................................
jsonrpc: executor based thread factory
Creating new thread for every request is not efficient so we introduce usage of the executor for request processing.
Change-Id: I56b307633a8bf7e4aad8f87cc97a4129c9ed0970 Signed-off-by: pkliczewski piotr.kliczewski@gmail.com Reviewed-on: https://gerrit.ovirt.org/43759 Reviewed-by: Dan Kenigsberg danken@redhat.com Continuous-Integration: Jenkins CI --- M lib/vdsm/config.py.in M tests/integration/jsonRpcHelper.py M tests/integration/jsonRpcTests.py M vdsm/clientIF.py M vdsm/rpc/bindingjsonrpc.py M vdsm/vdsm 6 files changed, 70 insertions(+), 16 deletions(-)
Approvals: Piotr Kliczewski: Verified Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved
automation@ovirt.org has posted comments on this change.
Change subject: jsonrpc: executor based thread factory ......................................................................
Patch Set 13:
* Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found.
vdsm-patches@lists.fedorahosted.org