)]}'
{"nova/compute/rpcapi.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07a44389abde9eac87f14f718e9b495e460ca351","unresolved":false,"context_lines":[{"line_number":389,"context_line":"                           parameter for the remote method."},{"line_number":390,"context_line":"        :param host: This is the host to send the message to."},{"line_number":391,"context_line":"        \u0027\u0027\u0027"},{"line_number":392,"context_line":"        with apply_target_cell_by_host(ctxt, host):"},{"line_number":393,"context_line":"            version \u003d \u00274.0\u0027"},{"line_number":394,"context_line":"            cctxt \u003d self.get_cell_client(ctxt).prepare("},{"line_number":395,"context_line":"                    server\u003dhost, version\u003dversion)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_f48f6c72","line":392,"updated":"2016-06-08 21:06:41.000000000","message":"Why is this not symmetrical with apply_target_cell? (one is a decorator, the other a context manager) Arg position of host?","commit_id":"946d06537cad56771cf7f7b225266341168bce0b"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"1f0ac85b5a0bdac79d78754c7cc693dc7630a9ad","unresolved":false,"context_lines":[{"line_number":389,"context_line":"                           parameter for the remote method."},{"line_number":390,"context_line":"        :param host: This is the host to send the message to."},{"line_number":391,"context_line":"        \u0027\u0027\u0027"},{"line_number":392,"context_line":"        with apply_target_cell_by_host(ctxt, host):"},{"line_number":393,"context_line":"            version \u003d \u00274.0\u0027"},{"line_number":394,"context_line":"            cctxt \u003d self.get_cell_client(ctxt).prepare("},{"line_number":395,"context_line":"                    server\u003dhost, version\u003dversion)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_9a926af5","line":392,"in_reply_to":"7aa08908_f48f6c72","updated":"2016-06-09 14:39:09.000000000","message":"Yes, It\u0027s too different to use decorator. I\u0027ll try to update methods signature as alaski suggests.","commit_id":"946d06537cad56771cf7f7b225266341168bce0b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07a44389abde9eac87f14f718e9b495e460ca351","unresolved":false,"context_lines":[{"line_number":1097,"context_line":"            server\u003d_compute_host(None, instances[0]),"},{"line_number":1098,"context_line":"            version\u003d\u00274.0\u0027)"},{"line_number":1099,"context_line":"        cctxt.cast(ctxt, \u0027external_instance_event\u0027, instances\u003dinstances,"},{"line_number":1100,"context_line":"                   events\u003devents)"},{"line_number":1101,"context_line":""},{"line_number":1102,"context_line":"    @apply_target_cell"},{"line_number":1103,"context_line":"    def build_and_run_instance(self, ctxt, instance, host, image, request_spec,"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_d44350ad","line":1100,"updated":"2016-06-08 21:06:41.000000000","message":"I had a look at this API in nova/compute/api.py and what it does is send a list of instances per host, for each host. So really this seems like a host-based call, all instances sent along reside on the same host","commit_id":"946d06537cad56771cf7f7b225266341168bce0b"}],"nova/context.py":[{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"f2e6f4b688038f4f2867c6d3bd47efd688ef9750","unresolved":false,"context_lines":[{"line_number":317,"context_line":"def apply_target_cell(function):"},{"line_number":318,"context_line":"    \"\"\"Apply cell context if InstanceMapping is found \"\"\""},{"line_number":319,"context_line":"    @functools.wraps(function)"},{"line_number":320,"context_line":"    def wrapped(self, ctxt, *args, **kwargs):"},{"line_number":321,"context_line":"        instances \u003d set()"},{"line_number":322,"context_line":"        for arg in itertools.chain(args, kwargs.values()):"},{"line_number":323,"context_line":"            if isinstance(arg, collections.Iterable):"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_db947144","line":320,"updated":"2016-06-07 21:03:26.000000000","message":"Unless I missed something every place where this is used passes in instance right after ctxt. Rather than looking for instance below just capture it here as the third param.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"19ab5e38201f2e0c0cbdebd94f50a124fa856fd5","unresolved":false,"context_lines":[{"line_number":317,"context_line":"def apply_target_cell(function):"},{"line_number":318,"context_line":"    \"\"\"Apply cell context if InstanceMapping is found \"\"\""},{"line_number":319,"context_line":"    @functools.wraps(function)"},{"line_number":320,"context_line":"    def wrapped(self, ctxt, *args, **kwargs):"},{"line_number":321,"context_line":"        instances \u003d set()"},{"line_number":322,"context_line":"        for arg in itertools.chain(args, kwargs.values()):"},{"line_number":323,"context_line":"            if isinstance(arg, collections.Iterable):"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_dfded6db","line":320,"in_reply_to":"7aa08908_9fbc80bc","updated":"2016-06-08 17:48:57.000000000","message":"I agree that using instance is better.\n\nI think you should fix those methods to be (self, ctxt, instance, ...). We use that pattern all over the code base and make some assumptions that parameters are organized like that. If you do that it should be in a separate patch before this one.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"6695ca2a6baf7ef9bb5f16936744d532a2dec2a2","unresolved":false,"context_lines":[{"line_number":317,"context_line":"def apply_target_cell(function):"},{"line_number":318,"context_line":"    \"\"\"Apply cell context if InstanceMapping is found \"\"\""},{"line_number":319,"context_line":"    @functools.wraps(function)"},{"line_number":320,"context_line":"    def wrapped(self, ctxt, *args, **kwargs):"},{"line_number":321,"context_line":"        instances \u003d set()"},{"line_number":322,"context_line":"        for arg in itertools.chain(args, kwargs.values()):"},{"line_number":323,"context_line":"            if isinstance(arg, collections.Iterable):"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_9fbc80bc","line":320,"in_reply_to":"7aa08908_db947144","updated":"2016-06-08 07:55:33.000000000","message":"Firstly I did it with third param but I found several naughty\nfunctions:\n\n    708:    def prep_resize(self, ctxt, image, instance, instance_type, host,\n    792:    def remove_volume_connection(self, ctxt, volume_id, instance, host):\n   1065:    def refresh_instance_security_rules(self, ctxt, host, instance):\n\nThey have host param but I think we should use more precise determining by instance.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"f2e6f4b688038f4f2867c6d3bd47efd688ef9750","unresolved":false,"context_lines":[{"line_number":328,"context_line":"        if not instances:"},{"line_number":329,"context_line":"            raise ValueError(\u0027Can not find instance to determine cell mapping\u0027)"},{"line_number":330,"context_line":"        cell_mapping \u003d objects.InstanceMapping.get_cell_mapping_by_instances("},{"line_number":331,"context_line":"            ctxt, instances)"},{"line_number":332,"context_line":"        if cell_mapping:"},{"line_number":333,"context_line":"            with target_cell(ctxt, cell_mapping):"},{"line_number":334,"context_line":"                return function(self, ctxt, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_7bb8ddc8","line":331,"updated":"2016-06-07 21:03:26.000000000","message":"There should only ever be one instance here so you don\u0027t need to worry about handling multiple instances.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"19ab5e38201f2e0c0cbdebd94f50a124fa856fd5","unresolved":false,"context_lines":[{"line_number":328,"context_line":"        if not instances:"},{"line_number":329,"context_line":"            raise ValueError(\u0027Can not find instance to determine cell mapping\u0027)"},{"line_number":330,"context_line":"        cell_mapping \u003d objects.InstanceMapping.get_cell_mapping_by_instances("},{"line_number":331,"context_line":"            ctxt, instances)"},{"line_number":332,"context_line":"        if cell_mapping:"},{"line_number":333,"context_line":"            with target_cell(ctxt, cell_mapping):"},{"line_number":334,"context_line":"                return function(self, ctxt, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_3fc27ae9","line":331,"in_reply_to":"7aa08908_5fd698f1","updated":"2016-06-08 17:48:57.000000000","message":"Okay. I think you should handle that method as a special case. It\u0027s possible that the instances could be in different cells so you\u0027ll need to loop through that list. I would do it in external_instance_event and not as part of this wrapper.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"6695ca2a6baf7ef9bb5f16936744d532a2dec2a2","unresolved":false,"context_lines":[{"line_number":328,"context_line":"        if not instances:"},{"line_number":329,"context_line":"            raise ValueError(\u0027Can not find instance to determine cell mapping\u0027)"},{"line_number":330,"context_line":"        cell_mapping \u003d objects.InstanceMapping.get_cell_mapping_by_instances("},{"line_number":331,"context_line":"            ctxt, instances)"},{"line_number":332,"context_line":"        if cell_mapping:"},{"line_number":333,"context_line":"            with target_cell(ctxt, cell_mapping):"},{"line_number":334,"context_line":"                return function(self, ctxt, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_5fd698f1","line":331,"in_reply_to":"7aa08908_7bb8ddc8","updated":"2016-06-08 07:55:33.000000000","message":"Special case is:\n\n   1027:    def external_instance_event(self, ctxt, instances, events):\n\nWith several instances.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07a44389abde9eac87f14f718e9b495e460ca351","unresolved":false,"context_lines":[{"line_number":314,"context_line":"        context.mq_connection \u003d original_mq_connection"},{"line_number":315,"context_line":""},{"line_number":316,"context_line":""},{"line_number":317,"context_line":"def apply_target_cell(function):"},{"line_number":318,"context_line":"    \"\"\"Apply cell context if InstanceMapping is found \"\"\""},{"line_number":319,"context_line":"    @functools.wraps(function)"},{"line_number":320,"context_line":"    def wrapped(self, ctxt, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_54cc80ba","line":317,"range":{"start_line":317,"start_character":4,"end_line":317,"end_character":21},"updated":"2016-06-08 21:06:41.000000000","message":"I was thinking maybe this should be named \"apply_target_cell_instance\" since we have two apply_target_cell things","commit_id":"946d06537cad56771cf7f7b225266341168bce0b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07a44389abde9eac87f14f718e9b495e460ca351","unresolved":false,"context_lines":[{"line_number":333,"context_line":"            with target_cell(ctxt, cell_mapping):"},{"line_number":334,"context_line":"                return function(self, ctxt, *args, **kwargs)"},{"line_number":335,"context_line":"        return function(self, ctxt, *args, **kwargs)"},{"line_number":336,"context_line":"    return wrapped"},{"line_number":337,"context_line":""},{"line_number":338,"context_line":""},{"line_number":339,"context_line":"@contextmanager"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_147af887","line":336,"updated":"2016-06-08 21:06:41.000000000","message":"Agreed with alaski, I don\u0027t think this function should be considering lists of instances -- it should take only one instance. And the instance arg position raises a challenge here that isn\u0027t an issue if we don\u0027t go the decorator route.","commit_id":"946d06537cad56771cf7f7b225266341168bce0b"}],"nova/objects/instance_mapping.py":[{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"f2e6f4b688038f4f2867c6d3bd47efd688ef9750","unresolved":false,"context_lines":[{"line_number":132,"context_line":"        self._destroy_in_db(self._context, self.instance_uuid)"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"    @staticmethod"},{"line_number":135,"context_line":"    def get_cell_mapping_by_instances(ctxt, instances):"},{"line_number":136,"context_line":"        \"\"\"Return cell mapping for several instances if possible"},{"line_number":137,"context_line":""},{"line_number":138,"context_line":"        @returns: None if cells are not migrated to cellsv2"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_3b80158b","line":135,"updated":"2016-06-07 21:03:26.000000000","message":"This should not be necessary as there is no case where multiple instances are handled in one method as far as I know.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07a44389abde9eac87f14f718e9b495e460ca351","unresolved":false,"context_lines":[{"line_number":151,"context_line":"                raise ValueError("},{"line_number":152,"context_line":"                    \u0027Can not determine cell mapping. \u0027"},{"line_number":153,"context_line":"                    \u0027Instances have different cells.\u0027)"},{"line_number":154,"context_line":"        return cell_mapping"},{"line_number":155,"context_line":""},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"@base.NovaObjectRegistry.register"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_5485a0cc","line":154,"updated":"2016-06-08 21:06:41.000000000","message":"This was only needed to handle the external_instance_event case, right? I\u0027m thinking we may not need it since the API seems host-based IMO","commit_id":"946d06537cad56771cf7f7b225266341168bce0b"}],"nova/tests/unit/api/openstack/fakes.py":[{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"b273d20f7e56f3414abe29bcf7c4b0f837f1ac8c","unresolved":false,"context_lines":[{"line_number":723,"context_line":"    raise NotImplementedError()"},{"line_number":724,"context_line":""},{"line_number":725,"context_line":""},{"line_number":726,"context_line":"def stub_cell_mapping(test):"},{"line_number":727,"context_line":"    @staticmethod"},{"line_number":728,"context_line":"    def im_not_found(*args, **kwargs):"},{"line_number":729,"context_line":"        raise exc.InstanceMappingNotFound("}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_9f43ae6b","line":726,"updated":"2016-06-08 17:51:48.000000000","message":"This is much better. I think this would be better in a different file though. Since it\u0027s not used in api tests this probably fits better in tests/unit/compute/ somewhere.","commit_id":"946d06537cad56771cf7f7b225266341168bce0b"}],"nova/tests/unit/compute/common.py":[{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"f2e6f4b688038f4f2867c6d3bd47efd688ef9750","unresolved":false,"context_lines":[{"line_number":40,"context_line":"        self.hm_mock.stop()"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"cellmapping_patch \u003d CellMappingPatch()"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_46c38438","line":43,"updated":"2016-06-07 21:03:26.000000000","message":"This should be a fixture or follow the pattern for other things that are stubbed out like this. There are some examples in http://git.openstack.org/cgit/openstack/nova/tree/nova/tests/unit/api/openstack/fakes.py#n108. That avoids needing to add the stop call everywhere.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"6695ca2a6baf7ef9bb5f16936744d532a2dec2a2","unresolved":false,"context_lines":[{"line_number":40,"context_line":"        self.hm_mock.stop()"},{"line_number":41,"context_line":""},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"cellmapping_patch \u003d CellMappingPatch()"}],"source_content_type":"text/x-python","patch_set":5,"id":"7aa08908_5f8e98d0","line":43,"in_reply_to":"7aa08908_46c38438","updated":"2016-06-08 07:55:33.000000000","message":"Yes, thanks. I\u0027ll fix.","commit_id":"d2bbe9e9b24cb47980287583a7f9e0a33fa48bcb"}]}