)]}'
{"nova/context.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e0ac64e3e9123c68682c69cac2a7b16ace052739","unresolved":false,"context_lines":[{"line_number":440,"context_line":"            with target_cell(context, cell_mapping) as cctxt:"},{"line_number":441,"context_line":"                result \u003d fn(cctxt, *args, **kwargs)"},{"line_number":442,"context_line":"        except Exception as e:"},{"line_number":443,"context_line":"            LOG.exception(\u0027Error gathering result from cell %s\u0027, cell_uuid)"},{"line_number":444,"context_line":"            result \u003d e.__class__(e.args)"},{"line_number":445,"context_line":"        # The queue is already synchronized."},{"line_number":446,"context_line":"        queue.put((cell_uuid, result))"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_4b28067e","side":"PARENT","line":443,"updated":"2018-11-21 16:04:47.000000000","message":"I feel like we might be losing some detail here that is pretty useful in a real failure. How about at least catching NovaException for the not-log case and continuing to log for the anything-else case? I was actually kinda expecting to just catch NotFound and maybe some other likely things for the not log case.","commit_id":"47bcc39cd633cdbdca97bf8b3d94bccd127b940f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"13a847fd5e63736fb104175ee82f123da6ecee90","unresolved":false,"context_lines":[{"line_number":440,"context_line":"            with target_cell(context, cell_mapping) as cctxt:"},{"line_number":441,"context_line":"                result \u003d fn(cctxt, *args, **kwargs)"},{"line_number":442,"context_line":"        except Exception as e:"},{"line_number":443,"context_line":"            LOG.exception(\u0027Error gathering result from cell %s\u0027, cell_uuid)"},{"line_number":444,"context_line":"            result \u003d e.__class__(e.args)"},{"line_number":445,"context_line":"        # The queue is already synchronized."},{"line_number":446,"context_line":"        queue.put((cell_uuid, result))"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_6eab901e","side":"PARENT","line":443,"in_reply_to":"3f79a3b5_4b28067e","updated":"2018-11-21 17:21:10.000000000","message":"\u003e How about at least catching NovaException for the not-log case and continuing to log for the anything-else case?\n\nI can do that.","commit_id":"47bcc39cd633cdbdca97bf8b3d94bccd127b940f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dec966d9fa4476aad0a264ce0ec96d2b018adfee","unresolved":false,"context_lines":[{"line_number":442,"context_line":"        except Exception as e:"},{"line_number":443,"context_line":"            # Only log the exception traceback for non-nova exceptions."},{"line_number":444,"context_line":"            if not isinstance(e, exception.NovaException):"},{"line_number":445,"context_line":"                LOG.exception(\u0027Error gathering result from cell %s\u0027, cell_uuid)"},{"line_number":446,"context_line":"            result \u003d e.__class__(e.args)"},{"line_number":447,"context_line":"        # The queue is already synchronized."},{"line_number":448,"context_line":"        queue.put((cell_uuid, result))"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_957f13bb","line":445,"updated":"2018-11-21 22:24:11.000000000","message":"I think this change could make sense, but want to point out this will completely hide raised NovaExceptions that are not handled by callers. Have we audited those? Or is that intended to be a followup activity?\n\nFor example, I noticed in a previously merged review, that without this log message, we use a blanket \"cell is not responding\" message when any exception other than InstanceNotFound was raised:\n\nhttps://github.com/openstack/nova/blob/1d444704a24a7103fb3cb73e451e4bff292f6467/nova/compute/api.py#L2412\n\nand we should probably do something like log the exception message in that case given this patch.","commit_id":"939a61d1c6054f24b6d47b5a6f1fa90fadc0c690"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"438bfe5e88791aff1e3c28bc6b631db9b07b1df4","unresolved":false,"context_lines":[{"line_number":442,"context_line":"        except Exception as e:"},{"line_number":443,"context_line":"            # Only log the exception traceback for non-nova exceptions."},{"line_number":444,"context_line":"            if not isinstance(e, exception.NovaException):"},{"line_number":445,"context_line":"                LOG.exception(\u0027Error gathering result from cell %s\u0027, cell_uuid)"},{"line_number":446,"context_line":"            result \u003d e.__class__(e.args)"},{"line_number":447,"context_line":"        # The queue is already synchronized."},{"line_number":448,"context_line":"        queue.put((cell_uuid, result))"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_55bd18b0","line":445,"in_reply_to":"3f79a3b5_957f13bb","updated":"2018-12-04 21:23:15.000000000","message":"\u003e I think this change could make sense, but want to point out this will completely hide raised NovaExceptions that are not handled by callers. Have we audited those? Or is that intended to be a followup activity?\n\nNo I haven\u0027t audited all the callers and wasn\u0027t really planning on doing that. The callers should be handling failures already, and if you look at https://review.openstack.org/#/c/607934/ and what call points needed to be changed, lots of them are logging something already. There are other cases that they aren\u0027t, and maybe don\u0027t need to.\n\nEarlier in the review Dan suggested just handling NotFound for the not-log case:\n\nhttps://review.openstack.org/#/c/619110/1/nova/context.py@a443\n\nWhich we can do, but that might also become a whack-a-mole issue over time. If people want to be more conservative and risk tracing exceptions in the logs for expected scenarios, and then add to the list over time, we can do that too. This is just very generic code and it doesn\u0027t seem right to me for this code to assume that any exception is grounds for tracing when this method doesn\u0027t have context on what\u0027s going on.","commit_id":"939a61d1c6054f24b6d47b5a6f1fa90fadc0c690"}],"nova/tests/unit/test_context.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b974950293623c150de97cde40a91a1af4aaaf13","unresolved":false,"context_lines":[{"line_number":430,"context_line":"            ctxt, mappings, 30, objects.InstanceList.get_by_filters)"},{"line_number":431,"context_line":"        self.assertEqual(2, len(results))"},{"line_number":432,"context_line":"        self.assertIn(mock.sentinel.instances, results.values())"},{"line_number":433,"context_line":"        isinstance(results.values(), Exception)"},{"line_number":434,"context_line":"        self.assertTrue(mock_log_exception.called)"},{"line_number":435,"context_line":""},{"line_number":436,"context_line":"    @mock.patch(\u0027nova.context.scatter_gather_cells\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_4160e59e","side":"PARENT","line":433,"range":{"start_line":433,"start_character":8,"end_line":433,"end_character":47},"updated":"2018-11-21 17:45:40.000000000","message":"This was always False, but didn\u0027t do anything. So I fixed it.","commit_id":"47bcc39cd633cdbdca97bf8b3d94bccd127b940f"}]}