From 0ed860d948632d71ca9e9e2ef908ce0b3ebc6e47 Mon Sep 17 00:00:00 2001 From: Yuming Zhu Date: Jun 04 2018 05:53:31 +0000 Subject: [PATCH 1/9] cli: add batch option in multiCall --- diff --git a/koji/__init__.py b/koji/__init__.py index 9d44488..a0325ec 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2521,7 +2521,7 @@ class ClientSession(object): time.sleep(interval) #not reached - def multiCall(self, strict=False): + def multiCall(self, strict=False, batch=0): """Execute a multicall (multiple function calls passed to the server and executed at the same time, with results being returned in a batch). Before calling this method, the self.multicall field must have @@ -2533,7 +2533,8 @@ class ClientSession(object): for each method added to the multicall, in the order it was added to the multicall. Each element of the list will be either a one-element list containing the result of the method call, or a map containing "faultCode" and "faultString" keys, describing the - error that occurred during the method call.""" + error that occurred during the method call. + if batch is bigger than 0, calls will be separated into chunks of calls.""" if not self.multicall: raise GenericError('ClientSession.multicall must be set to True before calling multiCall()') self.multicall = False @@ -2542,7 +2543,16 @@ class ClientSession(object): calls = self._calls self._calls = [] - ret = self._callMethod('multiCall', (calls,), {}) + if batch > 0: + ret = [] + callgrp = [calls[i:i + batch] for i in + range(0, len(calls), batch)] + self.logger.debug("MultiCall with batch size %s, calls/groups(%s/%s)", + batch, len(calls), len(callgrp)) + for c in callgrp: + ret.extend(self._callMethod('multiCall', (c,), {})) + else: + ret = self._callMethod('multiCall', (calls,), {}) if strict: #check for faults and raise first one for entry in ret: From 5bc21aa491be5725d00c21590b6d050b251addce Mon Sep 17 00:00:00 2001 From: Yuming Zhu Date: Jun 04 2018 05:53:31 +0000 Subject: [PATCH 2/9] cli: enable batch for clone-tag --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index bd47ed4..690bebd 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -3234,6 +3234,8 @@ def handle_clone_tag(goptions, session, args): parser.add_option("-f","--force", action="store_true", help=_("override tag locks if necessary")) parser.add_option("-n","--test", action="store_true", help=_("test mode")) + parser.add_option("--batch", type='int', default=1000, + help=_("batch size of multicalls")) (options, args) = parser.parse_args(args) if len(args) != 2: @@ -3249,6 +3251,9 @@ def handle_clone_tag(goptions, session, args): sys.stdout.write('Source and destination tags must be different.\n') return + if options.batch <= 0: + parser.error(_("batch size must be bigger than zero")) + if options.all: options.config = options.groups = options.pkgs = options.builds = True @@ -3300,7 +3305,7 @@ def handle_clone_tag(goptions, session, args): owner=pkgs['owner_name'],block=pkgs['blocked'], extra_arches=pkgs['extra_arches']) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) if options.builds: # get --all latest builds from src tag builds = session.listTagged(srctag['id'], event=event.get('id'), @@ -3317,7 +3322,7 @@ def handle_clone_tag(goptions, session, args): if not options.test: session.tagBuildBypass(newtag['name'], build, force=options.force) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) if options.groups: # Copy the group data srcgroups = session.getTagGroups(srctag['name'], event=event.get('id')) @@ -3332,7 +3337,7 @@ def handle_clone_tag(goptions, session, args): pkg['package'], block=pkg['blocked']) chggrplist.append(('[new]', pkg['package'], group['name'])) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # case of existing dst-tag. if dsttag: # get fresh list of packages & builds into maps. @@ -3423,7 +3428,7 @@ def handle_clone_tag(goptions, session, args): block=pkg['blocked'], extra_arches=pkg['extra_arches']) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # ADD builds. if not options.test: session.multicall = True @@ -3436,7 +3441,7 @@ def handle_clone_tag(goptions, session, args): if not options.test: session.tagBuildBypass(dsttag['name'], build, force=options.force) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # ADD groups. if not options.test: session.multicall = True @@ -3448,7 +3453,7 @@ def handle_clone_tag(goptions, session, args): session.groupPackageListAdd(dsttag['name'], group['name'], pkg['package'], force=options.force) chggrplist.append(('[new]', pkg['package'], group['name'])) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # ADD group pkgs. if not options.test: session.multicall = True @@ -3458,7 +3463,7 @@ def handle_clone_tag(goptions, session, args): if not options.test: session.groupPackageListAdd(dsttag['name'], group, pkg, force=options.force) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # DEL builds. if not options.test: session.multicall = True @@ -3473,7 +3478,7 @@ def handle_clone_tag(goptions, session, args): if not options.test: session.untagBuildBypass(dsttag['name'], build, force=options.force) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # DEL packages. ninhrtpdellist = [] inhrtpdellist = [] @@ -3512,7 +3517,7 @@ def handle_clone_tag(goptions, session, args): if not options.test: session.packageListBlock(dsttag['name'], pkg['package_name']) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # DEL groups. if not options.test: session.multicall = True @@ -3530,7 +3535,7 @@ def handle_clone_tag(goptions, session, args): for pkg in group['packagelist']: chggrplist.append(('[blk]', pkg['package'], group['name'])) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # DEL group pkgs. if not options.test: session.multicall = True @@ -3546,7 +3551,7 @@ def handle_clone_tag(goptions, session, args): if not options.test: session.groupPackageListBlock(dsttag['name'], group, pkg) if not options.test: - session.multiCall() + session.multiCall(batch=options.batch) # print final list of actions. if options.verbose: pfmt=' %-7s %-28s %-10s %-10s %-10s\n' From 09ff7ee6735855c211adca8007b961b71151ab58 Mon Sep 17 00:00:00 2001 From: Yuming Zhu Date: Jun 04 2018 05:53:31 +0000 Subject: [PATCH 3/9] cli: unittest for multiCall --- diff --git a/tests/test_lib/test_client_session.py b/tests/test_lib/test_client_session.py index 961421a..21bc795 100644 --- a/tests/test_lib/test_client_session.py +++ b/tests/test_lib/test_client_session.py @@ -4,6 +4,7 @@ import unittest import six import koji +from koji.xmlrpcplus import Fault class TestClientSession(unittest.TestCase): @@ -167,3 +168,59 @@ class TestFastUpload(unittest.TestCase): kwargs = call[0][2] self.assertTrue('volume' in kwargs) self.assertEqual(kwargs['volume'], 'foobar') + + +class TestMultiCall(unittest.TestCase): + + def setUp(self): + self.ksession = koji.ClientSession('http://koji.example.com/kojihub') + # mocks + self.ksession._sendCall = mock.MagicMock() + + def tearDown(self): + del self.ksession + + def test_multiCall_disable(self): + with self.assertRaises(koji.GenericError) as cm: + self.ksession.multiCall() + self.assertEqual(cm.exception.args[0], + "ClientSession.multicall must be set to True" + " before calling multiCall()") + + def test_multiCall_empty(self): + self.ksession.multicall = True + ret = self.ksession.multiCall() + self.assertEqual([], ret) + self.ksession._sendCall.assert_not_called() + + def test_multiCall_strict(self): + self.ksession._sendCall.return_value = [[], {'faultCode': 1000, + 'faultString': 'msg'}] + self.ksession.multicall = True + self.ksession.methodA('a', 'b', c='c') + self.ksession.methodB(1, 2, 3) + with self.assertRaises(koji.GenericError): + self.ksession.multiCall(strict=True) + + def test_multiCall_not_strict(self): + self.ksession._sendCall.return_value = [[], {'faultCode': 1000, + 'faultString': 'msg'}] + self.ksession.multicall = True + self.ksession.methodA('a', 'b', c='c') + self.ksession.methodB(1, 2, 3) + ret = self.ksession.multiCall() + self.assertFalse(self.ksession.multicall) + self.assertEqual([[], {'faultCode': 1000, 'faultString': 'msg'}], ret) + + def test_multiCall_batch(self): + self.ksession._sendCall.side_effect = [[['a', 'b', 'c']], + [{'faultCode': 1000, + 'faultString': 'msg'}]] + self.ksession.multicall = True + self.ksession.methodA('a', 'b', c='c') + self.ksession.methodB(1, 2, 3) + ret = self.ksession.multiCall(batch=1) + self.assertFalse(self.ksession.multicall) + self.assertEqual(2, self.ksession._sendCall.call_count) + self.assertEqual([['a', 'b', 'c'], + {'faultCode': 1000, 'faultString': 'msg'}], ret) From 66e1c921a96a81ec0ef9c5fc6170fe50d4b0c0bd Mon Sep 17 00:00:00 2001 From: Yuming Zhu Date: Jun 04 2018 05:53:31 +0000 Subject: [PATCH 4/9] set default batch to None and use generator instead of list --- diff --git a/koji/__init__.py b/koji/__init__.py index a0325ec..3b15235 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2521,7 +2521,7 @@ class ClientSession(object): time.sleep(interval) #not reached - def multiCall(self, strict=False, batch=0): + def multiCall(self, strict=False, batch=None): """Execute a multicall (multiple function calls passed to the server and executed at the same time, with results being returned in a batch). Before calling this method, the self.multicall field must have @@ -2543,12 +2543,11 @@ class ClientSession(object): calls = self._calls self._calls = [] - if batch > 0: + if batch is not None and batch > 0: ret = [] - callgrp = [calls[i:i + batch] for i in - range(0, len(calls), batch)] - self.logger.debug("MultiCall with batch size %s, calls/groups(%s/%s)", - batch, len(calls), len(callgrp)) + callgrp = (calls[i:i + batch] for i in range(0, len(calls), batch)) + self.logger.debug("MultiCall with batch size %i, calls/groups(%i/%i)", + batch, len(calls), round(len(calls) / batch)) for c in callgrp: ret.extend(self._callMethod('multiCall', (c,), {})) else: diff --git a/tests/test_lib/test_client_session.py b/tests/test_lib/test_client_session.py index 21bc795..ab40b80 100644 --- a/tests/test_lib/test_client_session.py +++ b/tests/test_lib/test_client_session.py @@ -2,6 +2,7 @@ from __future__ import absolute_import import mock import unittest import six +import logging import koji from koji.xmlrpcplus import Fault From 2d334d3fb9b23a16382500dc1dedc6e740c7f0cb Mon Sep 17 00:00:00 2001 From: Yuming Zhu Date: Jun 04 2018 05:53:31 +0000 Subject: [PATCH 5/9] add warning for batch usage in clone-tag --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index 690bebd..3bac475 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -37,7 +37,7 @@ from koji_cli.lib import _, OptionParser, activate_session, parse_arches, \ _unique_path, _running_in_bg, _progress_callback, watch_tasks, \ arg_filter, linked_upload, list_task_output_all_volumes, \ print_task_headers, print_task_recurse, download_file, watch_logs, \ - error, greetings, _list_tasks + warn, error, greetings, _list_tasks def _printable_unicode(s): @@ -3253,6 +3253,9 @@ def handle_clone_tag(goptions, session, args): if options.batch <= 0: parser.error(_("batch size must be bigger than zero")) + elif options.batch > 0: + warn(_('WARNING: Please notice that using batch which separates one ' + 'multicall to many transactions may cause data inconsistency.')) if options.all: options.config = options.groups = options.pkgs = options.builds = True diff --git a/koji/__init__.py b/koji/__init__.py index 3b15235..f32d8f7 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2534,7 +2534,10 @@ class ClientSession(object): Each element of the list will be either a one-element list containing the result of the method call, or a map containing "faultCode" and "faultString" keys, describing the error that occurred during the method call. - if batch is bigger than 0, calls will be separated into chunks of calls.""" + If batch is bigger than 0, calls will be separated into chunks of calls. + Please notice that the operations in one multicall request will be executed + in one single DB transaction. Using batch which separates this transaction + to many, may cause data inconsistency.""" if not self.multicall: raise GenericError('ClientSession.multicall must be set to True before calling multiCall()') self.multicall = False From 1cf4aced76a3d6937be8d7bf9e2cc053307d7f5f Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jun 04 2018 05:53:31 +0000 Subject: [PATCH 6/9] drop unecessary warning --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index 3bac475..690bebd 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -37,7 +37,7 @@ from koji_cli.lib import _, OptionParser, activate_session, parse_arches, \ _unique_path, _running_in_bg, _progress_callback, watch_tasks, \ arg_filter, linked_upload, list_task_output_all_volumes, \ print_task_headers, print_task_recurse, download_file, watch_logs, \ - warn, error, greetings, _list_tasks + error, greetings, _list_tasks def _printable_unicode(s): @@ -3253,9 +3253,6 @@ def handle_clone_tag(goptions, session, args): if options.batch <= 0: parser.error(_("batch size must be bigger than zero")) - elif options.batch > 0: - warn(_('WARNING: Please notice that using batch which separates one ' - 'multicall to many transactions may cause data inconsistency.')) if options.all: options.config = options.groups = options.pkgs = options.builds = True From 31466627040cb509ff809ef35e19bb240c18446b Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jun 04 2018 05:53:31 +0000 Subject: [PATCH 7/9] clean up docstring for multiCall() --- diff --git a/koji/__init__.py b/koji/__init__.py index f32d8f7..3f9d0f1 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2522,22 +2522,40 @@ class ClientSession(object): #not reached def multiCall(self, strict=False, batch=None): - """Execute a multicall (multiple function calls passed to the server - and executed at the same time, with results being returned in a batch). - Before calling this method, the self.multicall field must have - been set to True, and then one or more methods must have been called on - the current session (those method calls will return None). On executing - the multicall, the self.multicall field will be reset to False - (so subsequent method calls will be executed immediately) - and results will be returned in a list. The list will contain one element - for each method added to the multicall, in the order it was added to the multicall. - Each element of the list will be either a one-element list containing the result of the - method call, or a map containing "faultCode" and "faultString" keys, describing the - error that occurred during the method call. - If batch is bigger than 0, calls will be separated into chunks of calls. - Please notice that the operations in one multicall request will be executed - in one single DB transaction. Using batch which separates this transaction - to many, may cause data inconsistency.""" + """Execute a prepared multicall + + In a multicall, a number of calls are combined into a single RPC call + and handled by the server in a batch. This can improve throughput. + + The server handles a multicall as a single database transaction (though + see the note about the batch option below). + + To prepare a multicall: + 1. set the multicall attribute to True + 2. issue one or more calls in the normal fashion + + When multicall is True, the call parameters are stored rather than + passed to the server. Each call will return the special value + MultiCallInProgress, since the return is not yet known. + + This method executes the prepared multicall, resets the multicall + attribute to False (so subsequent calls will work normally), and + returns the results of the calls as a list. + + The result list will contain one element for each call added to the + multicall, in the order it was added. Each element will be either: + - a one-element list containing the result of the method call + - a map containing "faultCode" and "faultString" keys, describing + the error that occurred during the call. + + If the strict option is set to True, then this call will raise the + first error it encounters, if any. + + If the batch option is set to a number greater than zero, the calls + will be spread across multiple multicall batches of at most this + number. Note that each such batch will be a separate database + transaction. + """ if not self.multicall: raise GenericError('ClientSession.multicall must be set to True before calling multiCall()') self.multicall = False From e3d4e10d086473c4524e04867e8e906b43b94e7a Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jun 04 2018 05:53:31 +0000 Subject: [PATCH 8/9] give user option to use avoid using batches --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index 690bebd..95ec9ee 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -3234,8 +3234,8 @@ def handle_clone_tag(goptions, session, args): parser.add_option("-f","--force", action="store_true", help=_("override tag locks if necessary")) parser.add_option("-n","--test", action="store_true", help=_("test mode")) - parser.add_option("--batch", type='int', default=1000, - help=_("batch size of multicalls")) + parser.add_option("--batch", type='int', default=1000, metavar='SIZE', + help=_("batch size of multicalls [0 to disable, default: %default]")) (options, args) = parser.parse_args(args) if len(args) != 2: @@ -3251,7 +3251,7 @@ def handle_clone_tag(goptions, session, args): sys.stdout.write('Source and destination tags must be different.\n') return - if options.batch <= 0: + if options.batch < 0: parser.error(_("batch size must be bigger than zero")) if options.all: From da4467e4e9e314cdd40a6227600f20570e9da68e Mon Sep 17 00:00:00 2001 From: Yuming Zhu Date: Jun 04 2018 05:59:51 +0000 Subject: [PATCH 9/9] rebase bad reference change --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index 95ec9ee..666b1ad 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -3492,7 +3492,7 @@ def handle_clone_tag(goptions, session, args): for pkg in ninhrtpdellist: # check if package have owned builds inside. session.listTagged(dsttag['name'], package=pkg['package_name'], inherit=False) - bump_builds = session.multiCall() + bump_builds = session.multiCall(batch=options.batch) if not options.test: session.multicall = True for pkg, [builds] in zip(ninhrtpdellist, bump_builds):