)]}'
{"nova/image/glance.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"55e1071c86c1e0ab21d30b6ec630230d196b11f9","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_e5db0eec","line":251,"updated":"2016-03-15 20:25:44.000000000","message":"Is it worth using isgenerator here? Is it more readable to always list(result) whether it is a generator or list?","commit_id":"4bab8e4e10d48f5d083c4fe3d9e9a7728180384a"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"66cc76edb2666b269009a2a9aaeb80c27a0f5113","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_480ad7d3","line":251,"in_reply_to":"1af94dfe_284cab6f","updated":"2016-03-15 20:36:56.000000000","message":"Ah got it, thanks.","commit_id":"4bab8e4e10d48f5d083c4fe3d9e9a7728180384a"},{"author":{"_account_id":16907,"name":"Diana Clarke","email":"diana.joan.clarke@gmail.com","username":"diana-clarke"},"change_message_id":"9d448bb2ee9efdc31641e5f8f8204012a27d09e7","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1af94dfe_284cab6f","line":251,"in_reply_to":"1af94dfe_e5db0eec","updated":"2016-03-15 20:32:48.000000000","message":"The result is not always an iterable. For example, client.create() returns an image.","commit_id":"4bab8e4e10d48f5d083c4fe3d9e9a7728180384a"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"e10100812ce29748733cebd561785fb026b8856a","unresolved":false,"context_lines":[{"line_number":18,"context_line":"from __future__ import absolute_import"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"import copy"},{"line_number":21,"context_line":"import inspect"},{"line_number":22,"context_line":"import itertools"},{"line_number":23,"context_line":"import random"},{"line_number":24,"context_line":"import sys"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_36c3c158","line":21,"range":{"start_line":21,"start_character":0,"end_line":21,"end_character":14},"updated":"2016-03-18 15:48:58.000000000","message":"Personally I\u0027d prefer to use types.GeneratorType but I just checked and they are equivalent as the inspect.isgenerator does: return isinstance(object, types.GeneratorType)","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"797c4b39c60401fe6f34597cf788b5d4ca94dbe2","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_94e29f2d","line":251,"range":{"start_line":251,"start_character":16,"end_line":251,"end_character":47},"updated":"2016-03-18 11:30:27.000000000","message":"that looks a bit cumbersome, we should not look at that level of detail for the return value of a client call. I think the fix should go in the glanceclient, where it can check the return code of the API call and raise an exception if the API is down","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":16907,"name":"Diana Clarke","email":"diana.joan.clarke@gmail.com","username":"diana-clarke"},"change_message_id":"f839beba4eff131a73fdbd9aad7ee66ac47c00c0","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_cccf7b54","line":251,"range":{"start_line":251,"start_character":16,"end_line":251,"end_character":47},"in_reply_to":"1af94dfe_94e29f2d","updated":"2016-03-18 13:26:30.000000000","message":"Thanks for the review, Andrea.\n\nI did at first consider submitting a patch to glanceclient rather than nova, and if I were writing glanceclient again today probably I wouldn\u0027t use a generator at this API boundary, but it\u0027s been that way since 2012 and I think there are now too many consumers for backward incompatible changes.\n\nhttp://codesearch.openstack.org/?q\u003dfrom%20glanceclient\u0026i\u003dnope\u0026files\u003d\u0026repos\u003d\n\nhttp://codesearch.openstack.org/?q\u003dimport%20glanceclient\u0026i\u003dnope\u0026files\u003d\u0026repos\u003d\n\nI also suspect it may have been a design decision since it returns a generator rather than a list in both v1 and v2, and it\u0027s even explicitly documented in the code to return a generator in v2.\n\nhttps://github.com/openstack/python-glanceclient/blob/d59e341a4cd99a8488d5cf41052d9b218379ac87/glanceclient/v2/images.py#L89\n\nAlso note that the user guide explicitly documents and discusses the fact that `images.list()` returns a generator, and then goes on to show how to convert that generator result to a list:\n\nhttp://docs.openstack.org/user-guide/sdk_manage_images.html\n\n    \"\"\"\n    The images method returns a Python generator, as shown in the following interaction with the Python interpreter:\n\n    \u003e\u003e\u003e images \u003d glance.images.list()\n    \u003e\u003e\u003e images\n    \u003cgenerator object list at 0x105e9c2d0\u003e\n    \u003e\u003e\u003e list(images)\n    \"\"\"\n\nIn short, I think that ship has sailed and nova needs to deal with the fact that it returns a generator when it\u0027s attempting retries.\n\nAs for glanceclient doing the retries rather than nova, someone could certainly propose that feature request to glanceclient, but I think that\u0027s out of scope for this bug (and probably unlikely to land). Nova has been doing retries for all sorts of glanceclient calls since 2011 (show(), get(), etc), and it\u0027s just broken in the list() case.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"fdf6bc24c53dc79d838bb33d1d954095dabeb7d0","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_46a7ccbc","line":251,"in_reply_to":"1af94dfe_94e29f2d","updated":"2016-03-18 12:32:38.000000000","message":"We can\u0027t at this stage decide whether glance developers have to consider this as a but or not, as an example from my point of view it\u0027s totally acceptable to have a generator returned from the client. We are decorating the client by our own mechanism to handle retry call.\n\nThis is probably not good reason to add -1 on this patch, but a discussion can probably be open in ML.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":2750,"name":"Sean Dague","email":"sean@dague.net","username":"sdague"},"change_message_id":"22610c4fc53d6665b5566371007175fa634f4c8a","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_cf291b9d","line":251,"range":{"start_line":251,"start_character":16,"end_line":251,"end_character":47},"in_reply_to":"1af94dfe_969e15b7","updated":"2016-03-18 18:08:59.000000000","message":"Part of the reason we do the retry is we can encode a list of glance servers on our side, and I think we hop on retry.\n\nWhich... is it\u0027s own issue. The generator from glance is probably because of a clever thought around paging, which never happened.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":16907,"name":"Diana Clarke","email":"diana.joan.clarke@gmail.com","username":"diana-clarke"},"change_message_id":"d2cd14f890322419d6c9e831a4c0ace796d55736","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_a5f5dded","line":251,"range":{"start_line":251,"start_character":16,"end_line":251,"end_character":47},"in_reply_to":"1af94dfe_a2595337","updated":"2016-03-18 14:59:54.000000000","message":"Exceptions are raised, that\u0027s not the issue here. The problem is that the exception is raised too late for a retry in the case of a generator result. This patch just forces generator results to a list so that any exceptions get raised sooner rather than later, allowing nova to catch the exception and retry the call against another glance api server.\n\nLine 255 will be executed in all the non-list cases like images.get() and images.create() which is why your above suggestion won\u0027t work (and will actually break a bunch of things). For example, the get() call returns an Image, not an iterable.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"e10100812ce29748733cebd561785fb026b8856a","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_969e15b7","line":251,"range":{"start_line":251,"start_character":16,"end_line":251,"end_character":47},"in_reply_to":"1af94dfe_a5f5dded","updated":"2016-03-18 15:48:58.000000000","message":"you are right I forgot that the method is used for different client calls as well.\nAs regards the exception I know that they are raised but we got it just when we consume the generator and that happens when we are off the retry logic, my note was for the glanceclient fix, I was wondering if they return a generator only if the client can talk with the API otherwise fast fail raising an exception.\n\nAs I said I am going to see if I can get in touch with some glance guys.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"a25c20b1c8947fcff76356edc78fe2efd4f7c214","unresolved":false,"context_lines":[{"line_number":248,"context_line":"                                                                version)"},{"line_number":249,"context_line":"            try:"},{"line_number":250,"context_line":"                result \u003d getattr(client.images, method)(*args, **kwargs)"},{"line_number":251,"context_line":"                if inspect.isgenerator(result):"},{"line_number":252,"context_line":"                    # Convert generator results to a list, so that we can"},{"line_number":253,"context_line":"                    # catch any potential exceptions now and retry the call."},{"line_number":254,"context_line":"                    return list(result)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_a2595337","line":251,"range":{"start_line":251,"start_character":16,"end_line":251,"end_character":47},"in_reply_to":"1af94dfe_cccf7b54","updated":"2016-03-18 14:41:55.000000000","message":"Thanks for the reply. I am not saying that it is wrong to return a generator, I am happy with that, I just think that it should return an exception when it can\u0027t contact the API.\nI\u0027ll try to open a bug for glance and see what they think, in the meantime as we know that it is a generator what about fix it like:\n\nreturn list(getattr(client.images, method)(*args, **kwargs))\n\nas the line 255 won\u0027t be never executed","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"}],"nova/tests/unit/image/test_glance.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ad0d299f646467d2a08aa5defe0d0804101a84c9","unresolved":false,"context_lines":[{"line_number":383,"context_line":""},{"line_number":384,"context_line":"    @mock.patch(\u0027random.shuffle\u0027)"},{"line_number":385,"context_line":"    @mock.patch(\u0027time.sleep\u0027)"},{"line_number":386,"context_line":"    @mock.patch(\u0027nova.image.glance._glanceclient_from_endpoint\u0027)"},{"line_number":387,"context_line":"    def test_retry_works_with_generators(self, create_client_mock,"},{"line_number":388,"context_line":"                                         sleep_mock, shuffle_mock):"},{"line_number":389,"context_line":"        def some_generator(exception):"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_1421d8b4","line":386,"updated":"2016-03-16 16:49:29.000000000","message":"nit: I\u0027d add autospec\u003dTrue to all of these. It occasionally picks things up.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ad0d299f646467d2a08aa5defe0d0804101a84c9","unresolved":false,"context_lines":[{"line_number":396,"context_line":"            \u0027https://host2:9293\u0027,"},{"line_number":397,"context_line":"            \u0027http://host3:9294\u0027"},{"line_number":398,"context_line":"        ]"},{"line_number":399,"context_line":"        client_mock \u003d mock.MagicMock()"},{"line_number":400,"context_line":"        images_mock \u003d mock.MagicMock()"},{"line_number":401,"context_line":"        images_mock.list.side_effect \u003d ["},{"line_number":402,"context_line":"            some_generator(exception\u003dTrue),"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_9434c8fb","line":399,"updated":"2016-03-16 16:49:29.000000000","message":"Personal preference: You could ditch this line and replace one below by writing:\n\nclient_mock \u003d create_client_mock.return_value\n\nReads slightly better to me, but this is fine.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"97a9cfc9ded59e26659800abf4b8716413909c16","unresolved":false,"context_lines":[{"line_number":397,"context_line":"            \u0027http://host3:9294\u0027"},{"line_number":398,"context_line":"        ]"},{"line_number":399,"context_line":"        client_mock \u003d mock.MagicMock()"},{"line_number":400,"context_line":"        images_mock \u003d mock.MagicMock()"},{"line_number":401,"context_line":"        images_mock.list.side_effect \u003d ["},{"line_number":402,"context_line":"            some_generator(exception\u003dTrue),"},{"line_number":403,"context_line":"            some_generator(exception\u003dFalse),"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_7cd770bf","line":400,"updated":"2016-03-16 13:46:11.000000000","message":"When we can, we prefer to be more specific by using mock.Mock(spec\u003d)","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ad0d299f646467d2a08aa5defe0d0804101a84c9","unresolved":false,"context_lines":[{"line_number":398,"context_line":"        ]"},{"line_number":399,"context_line":"        client_mock \u003d mock.MagicMock()"},{"line_number":400,"context_line":"        images_mock \u003d mock.MagicMock()"},{"line_number":401,"context_line":"        images_mock.list.side_effect \u003d ["},{"line_number":402,"context_line":"            some_generator(exception\u003dTrue),"},{"line_number":403,"context_line":"            some_generator(exception\u003dFalse),"},{"line_number":404,"context_line":"        ]"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_1f5a9150","line":401,"updated":"2016-03-16 16:49:29.000000000","message":"Personal preference: You can just do here:\n\n        images_mock.list.side_effect \u003d [\n            glanceclient.exc.CommunicationError(\u0027Boom!\u0027),\n            (x for x in [\u0027something\u0027])\n        ]\n\nThis is a bit more explicit, and you can ditch the fake generator function above.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":16907,"name":"Diana Clarke","email":"diana.joan.clarke@gmail.com","username":"diana-clarke"},"change_message_id":"da1eebe23e782713427663a8824049014470af06","unresolved":false,"context_lines":[{"line_number":398,"context_line":"        ]"},{"line_number":399,"context_line":"        client_mock \u003d mock.MagicMock()"},{"line_number":400,"context_line":"        images_mock \u003d mock.MagicMock()"},{"line_number":401,"context_line":"        images_mock.list.side_effect \u003d ["},{"line_number":402,"context_line":"            some_generator(exception\u003dTrue),"},{"line_number":403,"context_line":"            some_generator(exception\u003dFalse),"},{"line_number":404,"context_line":"        ]"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_7142e93f","line":401,"in_reply_to":"1af94dfe_1f5a9150","updated":"2016-03-16 17:30:21.000000000","message":"That doesn\u0027t exercise the bug in question though. It needs to be a generator that raises an exception when consumed, not an exception followed by a result that happens to be a generator.\n\nGenerator expression are indeed cool, but I used a generator function so that I could explicitly give it a name for clarity.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ad0d299f646467d2a08aa5defe0d0804101a84c9","unresolved":false,"context_lines":[{"line_number":402,"context_line":"            some_generator(exception\u003dTrue),"},{"line_number":403,"context_line":"            some_generator(exception\u003dFalse),"},{"line_number":404,"context_line":"        ]"},{"line_number":405,"context_line":"        type(client_mock).images \u003d mock.PropertyMock(return_value\u003dimages_mock)"},{"line_number":406,"context_line":"        create_client_mock.return_value \u003d client_mock"},{"line_number":407,"context_line":""},{"line_number":408,"context_line":"        self.flags(num_retries\u003d1, group\u003d\u0027glance\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_3f978d91","line":405,"updated":"2016-03-16 16:49:29.000000000","message":"TIL. I didn\u0027t realise mock objects each had their own class.","commit_id":"ae6d868e2f13f90d9f97c982fdbbccdc6fb8b9c9"}]}