)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":13203,"name":"Wilson Liu","email":"295988511@qq.com","username":"Bob-OpenStack"},"change_message_id":"42115d15028204df75201edc94d0961699ba44e2","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Snapshot`s list method can raise SnapshotNotFound exception."},{"line_number":10,"context_line":"It happens because of race condition:"},{"line_number":11,"context_line":"1. List method gets all snapshots from DB."},{"line_number":12,"context_line":"2. At the same time one of the snapshots is being deleted from DB"},{"line_number":13,"context_line":"3. List method gets snapshot metadata from DB and checks"},{"line_number":14,"context_line":"   that snapshot exists in DB and raises the exception."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"fa32b979_c6494090","line":12,"updated":"2015-06-22 03:04:27.000000000","message":"\"from DB\" -\u003e \"from DB.\"","commit_id":"0733c1019bbc49fa31ea9d909904664572bf102f"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a65a2a4753ec5ebcd01bd7885d7107c5e7abbfaa","unresolved":false,"context_lines":[{"line_number":13,"context_line":"3. List method gets snapshot metadata from DB and checks"},{"line_number":14,"context_line":"   that snapshot exists in DB and raises the exception."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"First change removes getting metadata from DB for each found snapshot."},{"line_number":17,"context_line":"Instead of this it copies metadata from query result. This query"},{"line_number":18,"context_line":"is already joined with metadata table."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"fa32b979_4b71ed43","line":16,"updated":"2015-06-25 19:46:46.000000000","message":"First change?  You mean this patch?","commit_id":"4f23e9063b5d6dab72c44f427beb733295844dc3"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"4c8390d0f55e4dc44cb9b7f6a6a38f0e39c87673","unresolved":false,"context_lines":[{"line_number":13,"context_line":"3. List method gets snapshot metadata from DB and checks"},{"line_number":14,"context_line":"   that snapshot exists in DB and raises the exception."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"First change removes getting metadata from DB for each found snapshot."},{"line_number":17,"context_line":"Instead of this it copies metadata from query result. This query"},{"line_number":18,"context_line":"is already joined with metadata table."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"fa32b979_5692e64c","line":16,"in_reply_to":"fa32b979_4b71ed43","updated":"2015-06-26 05:06:42.000000000","message":"yes. In general now this patch has two changes. both are intended for speed up. But I don\u0027t like such solutions and prefer to leave only one that fixes original bug.","commit_id":"4f23e9063b5d6dab72c44f427beb733295844dc3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"392c6a0ae44a224e03060651dbe003687bb82a37","unresolved":false,"context_lines":[{"line_number":13,"context_line":"3. List method gets snapshot metadata from DB and checks"},{"line_number":14,"context_line":"   that snapshot exists in DB and raises the exception."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"First change removes getting metadata from DB for each found snapshot."},{"line_number":17,"context_line":"Instead of this it copies metadata from query result. This query"},{"line_number":18,"context_line":"is already joined with metadata table."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"fa32b979_65496a60","line":16,"in_reply_to":"fa32b979_5692e64c","updated":"2015-06-26 07:56:10.000000000","message":"Talking about a first change implies that this patch has a second change, which is not correct.  The second change is a suggestion for a change.","commit_id":"4f23e9063b5d6dab72c44f427beb733295844dc3"},{"author":{"_account_id":5997,"name":"Walt","display_name":"Hemna","email":"waboring@hemna.com","username":"walter-boring","status":"SAP"},"change_message_id":"aaab46efaa056834937844fd971053891001ec8a","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"First change removes getting metadata from DB for each found snapshot."},{"line_number":17,"context_line":"Instead of this it copies metadata from query result. This query"},{"line_number":18,"context_line":"is already joined with metadata table."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"And I suggest to remove checking of snapshot existence for getting"},{"line_number":21,"context_line":"metadata for private method because all public methods already have"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"ba3cc151_824a5a82","line":18,"updated":"2015-06-29 17:32:53.000000000","message":"Won\u0027t this introduce problems later on with someone relying on the metadata that is produced here living in the DB, when in fact it\u0027s gone from the DB?   Seems like copying data that is now gone from the DB would cause problems later on.  \n\nShouldn\u0027t we be putting a try block around the existing code and detected if it\u0027s gone, then remove it from the list and not adding metadata?","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"b625ecc30f1d1dedd0d04bf85f33f4b75482f1a4","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"First change removes getting metadata from DB for each found snapshot."},{"line_number":17,"context_line":"Instead of this it copies metadata from query result. This query"},{"line_number":18,"context_line":"is already joined with metadata table."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"And I suggest to remove checking of snapshot existence for getting"},{"line_number":21,"context_line":"metadata for private method because all public methods already have"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"ba3cc151_cb771d01","line":18,"in_reply_to":"ba3cc151_824a5a82","updated":"2015-06-29 18:35:52.000000000","message":"Code now checks that DB object has metadata in it and runs old code if not. If someone removes join then old code will work.\nSure, this problem will come again.\nIn this way I suggest to remove decorator \u0027@require_snapshot_exists\u0027 from db api(from public method get_snapshot_metadata). So in case of this bug we will have \u0027deleting\u0027 snapshot without metadata. I did it in first patch but Duncan preferred current fix.\n\nSecond way is to remove such snapshot from list. But this way require rework of list creating algorithm. Now this algorithm uses oslo_versionedobject library to create objects and fill list. Cinder code can only filter result list in each method of getting list of snapshot by any filter. So this way is more complicated and controversial.","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":5997,"name":"Walt","display_name":"Hemna","email":"waboring@hemna.com","username":"walter-boring","status":"SAP"},"change_message_id":"aaab46efaa056834937844fd971053891001ec8a","unresolved":false,"context_lines":[{"line_number":19,"context_line":""},{"line_number":20,"context_line":"And I suggest to remove checking of snapshot existence for getting"},{"line_number":21,"context_line":"metadata for private method because all public methods already have"},{"line_number":22,"context_line":"such decorator. Using second decorator will slow public method."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"Change-Id: I7f743638d9be4c01e18315a3459aecd2b3e9fd87"},{"line_number":25,"context_line":"Closes-Bug: #1462453"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"ba3cc151_42d1b2cc","line":22,"updated":"2015-06-29 17:32:53.000000000","message":"This doesn\u0027t look like part of the patch, so it shouldn\u0027t be in the commit message.","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"b625ecc30f1d1dedd0d04bf85f33f4b75482f1a4","unresolved":false,"context_lines":[{"line_number":19,"context_line":""},{"line_number":20,"context_line":"And I suggest to remove checking of snapshot existence for getting"},{"line_number":21,"context_line":"metadata for private method because all public methods already have"},{"line_number":22,"context_line":"such decorator. Using second decorator will slow public method."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"Change-Id: I7f743638d9be4c01e18315a3459aecd2b3e9fd87"},{"line_number":25,"context_line":"Closes-Bug: #1462453"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"ba3cc151_2b11e952","line":22,"in_reply_to":"ba3cc151_42d1b2cc","updated":"2015-06-29 18:35:52.000000000","message":"This is in db/sqlalchemy/api.py\nShould I revert api.py ?","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"34936ae2108816842bcb403126c48432cbad134d","unresolved":false,"context_lines":[{"line_number":13,"context_line":"3. List method gets snapshot metadata from DB and checks"},{"line_number":14,"context_line":"   that snapshot exists in DB and raises the exception."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"This review changes behaviour of getting snapshot metadata from DB."},{"line_number":17,"context_line":"Code now gets metadata from db_snapshot object that was queried"},{"line_number":18,"context_line":"by joined query from DB instead of second query for each snapshot"},{"line_number":19,"context_line":"for metadata."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"ba3cc151_6f2a0492","line":16,"updated":"2015-07-02 12:44:23.000000000","message":"This review?  You mean this patch?","commit_id":"11ab8f39d675cc146a6a989055734e8388370b30"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"34936ae2108816842bcb403126c48432cbad134d","unresolved":false,"context_lines":[{"line_number":18,"context_line":"by joined query from DB instead of second query for each snapshot"},{"line_number":19,"context_line":"for metadata."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"And I suggest to remove checking of snapshot existence for getting"},{"line_number":22,"context_line":"metadata for private method because all public methods already have"},{"line_number":23,"context_line":"such decorator. Using second decorator will slow public method."},{"line_number":24,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"ba3cc151_8afbb6c0","line":21,"updated":"2015-07-02 12:44:23.000000000","message":"It looks like you are actually removing it, not just suggesting it, by deleting \t2097 of db/sqlalchemy/api.py file. ;-)","commit_id":"11ab8f39d675cc146a6a989055734e8388370b30"}],"cinder/db/sqlalchemy/api.py":[{"author":{"_account_id":1736,"name":"Ivan Kolodyazhny","email":"e0ne@e0ne.info","username":"e0ne"},"change_message_id":"84498aadf7e8e8eaa5eea508a93eaeff79c70000","unresolved":false,"context_lines":[{"line_number":2078,"context_line":"        filter_by(snapshot_id\u003dsnapshot_id)"},{"line_number":2079,"context_line":""},{"line_number":2080,"context_line":""},{"line_number":2081,"context_line":"@require_context"},{"line_number":2082,"context_line":"@require_snapshot_exists"},{"line_number":2083,"context_line":"def _snapshot_metadata_get(context, snapshot_id, session\u003dNone):"},{"line_number":2084,"context_line":"    rows \u003d _snapshot_metadata_get_query(context, snapshot_id, session).all()"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_89d93af7","side":"PARENT","line":2081,"updated":"2015-06-25 22:16:19.000000000","message":"Why you don\u0027t use something like https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L538?","commit_id":"a874f951830e35a2a9080f51e992476e6ebfcd49"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"4c8390d0f55e4dc44cb9b7f6a6a38f0e39c87673","unresolved":false,"context_lines":[{"line_number":2078,"context_line":"        filter_by(snapshot_id\u003dsnapshot_id)"},{"line_number":2079,"context_line":""},{"line_number":2080,"context_line":""},{"line_number":2081,"context_line":"@require_context"},{"line_number":2082,"context_line":"@require_snapshot_exists"},{"line_number":2083,"context_line":"def _snapshot_metadata_get(context, snapshot_id, session\u003dNone):"},{"line_number":2084,"context_line":"    rows \u003d _snapshot_metadata_get_query(context, snapshot_id, session).all()"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_36dba223","side":"PARENT","line":2081,"in_reply_to":"fa32b979_89d93af7","updated":"2015-06-26 05:06:42.000000000","message":"because here is a read operation not write. \"retry\" here is useless because we can read information from DB but this information is absent in it.","commit_id":"a874f951830e35a2a9080f51e992476e6ebfcd49"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"07f8195d4b4aec88015ae09f32416cbec9e5793a","unresolved":false,"context_lines":[{"line_number":2089,"context_line":""},{"line_number":2090,"context_line":""},{"line_number":2091,"context_line":"@require_context"},{"line_number":2092,"context_line":"@require_snapshot_exists"},{"line_number":2093,"context_line":"def snapshot_metadata_get(context, snapshot_id):"},{"line_number":2094,"context_line":"    return _snapshot_metadata_get(context, snapshot_id)"},{"line_number":2095,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_48c33358","line":2092,"updated":"2015-06-25 19:19:02.000000000","message":"Why is @require_snapshot_exists back?","commit_id":"4f23e9063b5d6dab72c44f427beb733295844dc3"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"3bd9d60b884ba3fadfbef0d341cfa0697599563c","unresolved":false,"context_lines":[{"line_number":2089,"context_line":""},{"line_number":2090,"context_line":""},{"line_number":2091,"context_line":"@require_context"},{"line_number":2092,"context_line":"@require_snapshot_exists"},{"line_number":2093,"context_line":"def snapshot_metadata_get(context, snapshot_id):"},{"line_number":2094,"context_line":"    return _snapshot_metadata_get(context, snapshot_id)"},{"line_number":2095,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_48f7b359","line":2092,"in_reply_to":"fa32b979_48c33358","updated":"2015-06-25 19:28:02.000000000","message":"Because current solution is more safest. Because I think that this is not good to do several changes in one review. Because now it doesn\u0027t affect to bug that I\u0027m trying to fix.\nAnd this decorator will be used by all consumers who wants to retrieve only metadata.","commit_id":"4f23e9063b5d6dab72c44f427beb733295844dc3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a65a2a4753ec5ebcd01bd7885d7107c5e7abbfaa","unresolved":false,"context_lines":[{"line_number":2089,"context_line":""},{"line_number":2090,"context_line":""},{"line_number":2091,"context_line":"@require_context"},{"line_number":2092,"context_line":"@require_snapshot_exists"},{"line_number":2093,"context_line":"def snapshot_metadata_get(context, snapshot_id):"},{"line_number":2094,"context_line":"    return _snapshot_metadata_get(context, snapshot_id)"},{"line_number":2095,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_cb597d37","line":2092,"in_reply_to":"fa32b979_48f7b359","updated":"2015-06-25 19:46:46.000000000","message":"I\u0027m ok leaving this, and when you talk about \"all consumers who want to retrieve only metadata\" is exactly what I was referring to when I mentioned before if you had looked at all possible scenarios. ;-)","commit_id":"4f23e9063b5d6dab72c44f427beb733295844dc3"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"4c8390d0f55e4dc44cb9b7f6a6a38f0e39c87673","unresolved":false,"context_lines":[{"line_number":2089,"context_line":""},{"line_number":2090,"context_line":""},{"line_number":2091,"context_line":"@require_context"},{"line_number":2092,"context_line":"@require_snapshot_exists"},{"line_number":2093,"context_line":"def snapshot_metadata_get(context, snapshot_id):"},{"line_number":2094,"context_line":"    return _snapshot_metadata_get(context, snapshot_id)"},{"line_number":2095,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa32b979_56074681","line":2092,"in_reply_to":"fa32b979_cb597d37","updated":"2015-06-26 05:06:42.000000000","message":"Yeah, I\u0027ve looked at all scenarios and haven\u0027t found any objection to remove this but now I prefer to leave this decorator because it doesn\u0027t affect fix of the bug.","commit_id":"4f23e9063b5d6dab72c44f427beb733295844dc3"}],"cinder/objects/snapshot.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"20f4f9b8a45bf187c8bd126e5957332d2c9e0009","unresolved":false,"context_lines":[{"line_number":117,"context_line":"            snapshot.volume \u003d volume"},{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if  metadata:"},{"line_number":121,"context_line":"                snapshot.metadata \u003d dict()"},{"line_number":122,"context_line":"                for item in metadata:"},{"line_number":123,"context_line":"                    snapshot.metadata[item.get(\u0027key\u0027)] \u003d item.get(\u0027value\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa32b979_e1680724","line":120,"updated":"2015-06-26 10:51:44.000000000","message":"extra space before metadata","commit_id":"75e4456a9c9dd49716facb8f4b6fee6bce2317e3"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"20f4f9b8a45bf187c8bd126e5957332d2c9e0009","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            if  metadata:"},{"line_number":121,"context_line":"                snapshot.metadata \u003d dict()"},{"line_number":122,"context_line":"                for item in metadata:"},{"line_number":123,"context_line":"                    snapshot.metadata[item.get(\u0027key\u0027)] \u003d item.get(\u0027value\u0027)"},{"line_number":124,"context_line":"            else:"},{"line_number":125,"context_line":"                snapshot.metadata \u003d db.snapshot_metadata_get(context,"},{"line_number":126,"context_line":"                                                             db_snapshot[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":5,"id":"fa32b979_e17247e3","line":123,"updated":"2015-06-26 10:51:44.000000000","message":"Why not use dict comprehension?","commit_id":"75e4456a9c9dd49716facb8f4b6fee6bce2317e3"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"cf9390116f93e9c63f43cc67711a22b98efaab74","unresolved":false,"context_lines":[{"line_number":116,"context_line":"            volume._from_db_object(context, volume, db_snapshot[\u0027volume\u0027])"},{"line_number":117,"context_line":"            snapshot.volume \u003d volume"},{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if metadata:"},{"line_number":121,"context_line":"                snapshot.metadata \u003d dict()"},{"line_number":122,"context_line":"                for item in metadata:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa32b979_edb47536","line":119,"updated":"2015-06-26 13:09:16.000000000","message":"Instead of below if-else condition you can simply use\n\ndict((item.key, item.value) for item in metadata)\n\nwhich will also return an empty dictionary in case of metadata is not present for a snapshot.","commit_id":"edfd9b19735d37b28f0bcb39dbc2251e9e2115a5"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"55bc11b2c2889984fb97fe70d0a87813b8b149dd","unresolved":false,"context_lines":[{"line_number":116,"context_line":"            volume._from_db_object(context, volume, db_snapshot[\u0027volume\u0027])"},{"line_number":117,"context_line":"            snapshot.volume \u003d volume"},{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if metadata:"},{"line_number":121,"context_line":"                snapshot.metadata \u003d dict()"},{"line_number":122,"context_line":"                for item in metadata:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa32b979_583f6187","line":119,"in_reply_to":"fa32b979_186799eb","updated":"2015-06-26 13:36:42.000000000","message":"You\u0027ve said that I can remove \u0027else\u0027 block.\nSure, dict can be filled as you mentioned.","commit_id":"edfd9b19735d37b28f0bcb39dbc2251e9e2115a5"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"1f7a5a8228ac7210787a669aaab4e7321221bb3e","unresolved":false,"context_lines":[{"line_number":116,"context_line":"            volume._from_db_object(context, volume, db_snapshot[\u0027volume\u0027])"},{"line_number":117,"context_line":"            snapshot.volume \u003d volume"},{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if metadata:"},{"line_number":121,"context_line":"                snapshot.metadata \u003d dict()"},{"line_number":122,"context_line":"                for item in metadata:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa32b979_186799eb","line":119,"in_reply_to":"fa32b979_8d1049ad","updated":"2015-06-26 13:31:40.000000000","message":"I have just optimized the code what you have used in if block, so I am not sure how it will call additional DB queries.","commit_id":"edfd9b19735d37b28f0bcb39dbc2251e9e2115a5"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"f7d68d78e0d1c5cef0d8ed7d1138dd5c1e21a269","unresolved":false,"context_lines":[{"line_number":116,"context_line":"            volume._from_db_object(context, volume, db_snapshot[\u0027volume\u0027])"},{"line_number":117,"context_line":"            snapshot.volume \u003d volume"},{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if metadata:"},{"line_number":121,"context_line":"                snapshot.metadata \u003d dict()"},{"line_number":122,"context_line":"                for item in metadata:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa32b979_8d1049ad","line":119,"in_reply_to":"fa32b979_edb47536","updated":"2015-06-26 13:19:58.000000000","message":"I\u0027m not agree with this. Because additional querying DB leads to bug that I\u0027m trying to fix.","commit_id":"edfd9b19735d37b28f0bcb39dbc2251e9e2115a5"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"cf9390116f93e9c63f43cc67711a22b98efaab74","unresolved":false,"context_lines":[{"line_number":121,"context_line":"                snapshot.metadata \u003d dict()"},{"line_number":122,"context_line":"                for item in metadata:"},{"line_number":123,"context_line":"                    snapshot.metadata[item[\u0027key\u0027]] \u003d item[\u0027value\u0027]"},{"line_number":124,"context_line":"            else:"},{"line_number":125,"context_line":"                snapshot.metadata \u003d db.snapshot_metadata_get(context,"},{"line_number":126,"context_line":"                                                             db_snapshot[\u0027id\u0027])"},{"line_number":127,"context_line":"        snapshot._context \u003d context"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa32b979_0d951987","line":124,"updated":"2015-06-26 13:09:16.000000000","message":"IMO, there is no need of this else block if metadata is not found from above query result, because in that case this else block will also return empty dictionary.","commit_id":"edfd9b19735d37b28f0bcb39dbc2251e9e2115a5"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"50777cc770189ba7ac316a15c4ba62d6bb7a6841","unresolved":false,"context_lines":[{"line_number":121,"context_line":"                snapshot.metadata \u003d {item[\u0027key\u0027]: item[\u0027value\u0027]"},{"line_number":122,"context_line":"                                     for item in metadata}"},{"line_number":123,"context_line":"            else:"},{"line_number":124,"context_line":"                snapshot.metadata \u003d db.snapshot_metadata_get(context,"},{"line_number":125,"context_line":"                                                             db_snapshot[\u0027id\u0027])"},{"line_number":126,"context_line":"        snapshot._context \u003d context"},{"line_number":127,"context_line":"        snapshot.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa32b979_8fb33d4d","line":124,"updated":"2015-06-27 13:22:32.000000000","message":"I think Ankit made a good point in patchset 7. If there is no metadata from the joined query, is there a reason we would get metadata querying it directly here? Can you describe how this works?","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"764150860fa01de9f889f135286ac89f3a1f80f4","unresolved":false,"context_lines":[{"line_number":121,"context_line":"                snapshot.metadata \u003d {item[\u0027key\u0027]: item[\u0027value\u0027]"},{"line_number":122,"context_line":"                                     for item in metadata}"},{"line_number":123,"context_line":"            else:"},{"line_number":124,"context_line":"                snapshot.metadata \u003d db.snapshot_metadata_get(context,"},{"line_number":125,"context_line":"                                                             db_snapshot[\u0027id\u0027])"},{"line_number":126,"context_line":"        snapshot._context \u003d context"},{"line_number":127,"context_line":"        snapshot.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_0c720de6","line":124,"in_reply_to":"ba3cc151_95c70e45","updated":"2015-06-29 13:38:59.000000000","message":"I can confirm it now but I can\u0027t guarantee that future work will not break it.","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":10485,"name":"Ankit Agrawal","email":"ankitagrawal8188@gmail.com","username":"ankitagrawal"},"change_message_id":"e9476400060c90953ebd5429326a6c80efc668e3","unresolved":false,"context_lines":[{"line_number":121,"context_line":"                snapshot.metadata \u003d {item[\u0027key\u0027]: item[\u0027value\u0027]"},{"line_number":122,"context_line":"                                     for item in metadata}"},{"line_number":123,"context_line":"            else:"},{"line_number":124,"context_line":"                snapshot.metadata \u003d db.snapshot_metadata_get(context,"},{"line_number":125,"context_line":"                                                             db_snapshot[\u0027id\u0027])"},{"line_number":126,"context_line":"        snapshot._context \u003d context"},{"line_number":127,"context_line":"        snapshot.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_95c70e45","line":124,"in_reply_to":"fa32b979_0111415d","updated":"2015-06-29 05:45:09.000000000","message":"Makes sense if you have not confirmed the join query results and want to rework on it separably, then I think you should add a TODO note here for clear understanding.","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"9c012677f33cddb9c133c67741491d8165c4942d","unresolved":false,"context_lines":[{"line_number":121,"context_line":"                snapshot.metadata \u003d {item[\u0027key\u0027]: item[\u0027value\u0027]"},{"line_number":122,"context_line":"                                     for item in metadata}"},{"line_number":123,"context_line":"            else:"},{"line_number":124,"context_line":"                snapshot.metadata \u003d db.snapshot_metadata_get(context,"},{"line_number":125,"context_line":"                                                             db_snapshot[\u0027id\u0027])"},{"line_number":126,"context_line":"        snapshot._context \u003d context"},{"line_number":127,"context_line":"        snapshot.obj_reset_changes()"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa32b979_0111415d","line":124,"in_reply_to":"fa32b979_8fb33d4d","updated":"2015-06-27 15:30:04.000000000","message":"IMO, this code doesn\u0027t know which query executed before - with join or not.\nthis code just fills snapshot object from db item. \n\nIn any way - I can rework DB layer(to be sure that all snapshot query has join with metadata), also I can rework all unit tests that have fake snapshot object without metadata. But it will be big review instead of one. And I thought to make small fix to let our gating work.","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"34936ae2108816842bcb403126c48432cbad134d","unresolved":false,"context_lines":[{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if metadata is None:"},{"line_number":121,"context_line":"                raise exception.MetadataAbsent()"},{"line_number":122,"context_line":"            snapshot.metadata \u003d {item[\u0027key\u0027]: item[\u0027value\u0027]"},{"line_number":123,"context_line":"                                 for item in metadata}"},{"line_number":124,"context_line":"        snapshot._context \u003d context"}],"source_content_type":"text/x-python","patch_set":11,"id":"ba3cc151_8a4176ab","line":121,"updated":"2015-07-02 12:44:23.000000000","message":"I fail to see how we are going to fix the bug we want to fix if we are raising MetadataAbsent on listings when the metadata is not there...","commit_id":"11ab8f39d675cc146a6a989055734e8388370b30"},{"author":{"_account_id":1207,"name":"Duncan Thomas","email":"duncan.thomas@gmail.com","username":"duncan-thomas"},"change_message_id":"2ddc51feb5dc238f6fcbb7956b6b6785c584c039","unresolved":false,"context_lines":[{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if metadata is None:"},{"line_number":121,"context_line":"                raise exception.MetadataAbsent()"},{"line_number":122,"context_line":"            snapshot.metadata \u003d {item[\u0027key\u0027]: item[\u0027value\u0027]"},{"line_number":123,"context_line":"                                 for item in metadata}"},{"line_number":124,"context_line":"        snapshot._context \u003d context"}],"source_content_type":"text/x-python","patch_set":11,"id":"ba3cc151_2b886dc4","line":121,"in_reply_to":"ba3cc151_084faf37","updated":"2015-07-02 13:17:14.000000000","message":"Gorka: Every existing codepath that needs metadata does a joined load. This just catches future codepaths so that they can add it. Doing queries on the DB for every entry in a list is an absolute no-no, it works fine in testing but it absolutely destroys the DB in production.","commit_id":"11ab8f39d675cc146a6a989055734e8388370b30"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"521905f9e55b84068127e8ba92e6517b9eca0711","unresolved":false,"context_lines":[{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if metadata is None:"},{"line_number":121,"context_line":"                raise exception.MetadataAbsent()"},{"line_number":122,"context_line":"            snapshot.metadata \u003d {item[\u0027key\u0027]: item[\u0027value\u0027]"},{"line_number":123,"context_line":"                                 for item in metadata}"},{"line_number":124,"context_line":"        snapshot._context \u003d context"}],"source_content_type":"text/x-python","patch_set":11,"id":"ba3cc151_0ea7b7aa","line":121,"in_reply_to":"ba3cc151_2b886dc4","updated":"2015-07-02 14:35:49.000000000","message":"What I didn\u0027t see was how this fixed the bug, but it must have been that I was too hungry, because it\u0027s obvious.  When doing the list, SnapshotList class calls snapshot_get_all and then uses the joined result (so metadata will be at least be {}) to call obj_make_list which calls _from_db_object and since we no longer try to get the metadata from DB we will never get the exception from the bug.","commit_id":"11ab8f39d675cc146a6a989055734e8388370b30"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"f8c559eb7d6e0236ac94ba2df37d2c0ece0f678c","unresolved":false,"context_lines":[{"line_number":118,"context_line":"        if \u0027metadata\u0027 in expected_attrs:"},{"line_number":119,"context_line":"            metadata \u003d db_snapshot.get(\u0027snapshot_metadata\u0027)"},{"line_number":120,"context_line":"            if metadata is None:"},{"line_number":121,"context_line":"                raise exception.MetadataAbsent()"},{"line_number":122,"context_line":"            snapshot.metadata \u003d {item[\u0027key\u0027]: item[\u0027value\u0027]"},{"line_number":123,"context_line":"                                 for item in metadata}"},{"line_number":124,"context_line":"        snapshot._context \u003d context"}],"source_content_type":"text/x-python","patch_set":11,"id":"ba3cc151_084faf37","line":121,"in_reply_to":"ba3cc151_8a4176ab","updated":"2015-07-02 12:50:27.000000000","message":"Now it will fail if {someone forget to join metadata while querying for snapshots} and {want metadata in expected_attributes}.\nCurrently all queries have such join.","commit_id":"11ab8f39d675cc146a6a989055734e8388370b30"}],"cinder/tests/unit/objects/test_snapshot.py":[{"author":{"_account_id":2759,"name":"Huang Zhiteng","email":"winston.d@gmail.com","username":"zhiteng-huang"},"change_message_id":"8400ec4b38b68612751116b744cff372c9030918","unresolved":false,"context_lines":[{"line_number":39,"context_line":"    \u0027display_name\u0027: \u0027fake_name\u0027,"},{"line_number":40,"context_line":"    \u0027display_description\u0027: \u0027fake_description\u0027,"},{"line_number":41,"context_line":"    \u0027snapshot_metadata\u0027: [{\u0027key\u0027: \u0027fake_key\u0027, \u0027value\u0027: \u0027fake_value\u0027}],"},{"line_number":42,"context_line":"}"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"class TestSnapshot(test_objects.BaseObjectsTestCase):"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_34e5e8a7","line":42,"updated":"2015-06-29 03:15:38.000000000","message":"How about:\n\n    fake_snapshot_with_metadata \u003d fake_snapshot.update(\u0027snapshot_metadata\u0027: [...]}\n\nThis can reduce overhead to manually sync fake_snapshot and fake_snapshot_with_metadata.","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"764150860fa01de9f889f135286ac89f3a1f80f4","unresolved":false,"context_lines":[{"line_number":39,"context_line":"    \u0027display_name\u0027: \u0027fake_name\u0027,"},{"line_number":40,"context_line":"    \u0027display_description\u0027: \u0027fake_description\u0027,"},{"line_number":41,"context_line":"    \u0027snapshot_metadata\u0027: [{\u0027key\u0027: \u0027fake_key\u0027, \u0027value\u0027: \u0027fake_value\u0027}],"},{"line_number":42,"context_line":"}"},{"line_number":43,"context_line":""},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"class TestSnapshot(test_objects.BaseObjectsTestCase):"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_a7969a92","line":42,"in_reply_to":"ba3cc151_34e5e8a7","updated":"2015-06-29 13:38:59.000000000","message":"it\u0027s good change. But I prefer to save current +2. If this review will be reworked then I\u0027ll change this palce.","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"da9e7ff6b9ec4b7b9adaf5fbbd75d2182d17d877","unresolved":false,"context_lines":[{"line_number":268,"context_line":"        snapshots \u003d objects.SnapshotList.get_all("},{"line_number":269,"context_line":"            self.context, search_opts)"},{"line_number":270,"context_line":"        self.assertEqual(1, len(snapshots))"},{"line_number":271,"context_line":"        TestSnapshot._compare(self, fake_snapshot, snapshots[0])"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_dd2a9c2c","line":271,"updated":"2015-06-29 07:41:30.000000000","message":"Isn\u0027t this missing an assert_called_once_with on snapshot_get_all?","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"84b301ce257b31e65485cae79486b3ab8872419d","unresolved":false,"context_lines":[{"line_number":268,"context_line":"        snapshots \u003d objects.SnapshotList.get_all("},{"line_number":269,"context_line":"            self.context, search_opts)"},{"line_number":270,"context_line":"        self.assertEqual(1, len(snapshots))"},{"line_number":271,"context_line":"        TestSnapshot._compare(self, fake_snapshot, snapshots[0])"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_7ef2549c","line":271,"in_reply_to":"ba3cc151_03ddf341","updated":"2015-06-29 16:16:19.000000000","message":"I will do it if I will rework this review :)","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"4189881d904bbc9af1c2e5aa23822a60bbd3b40d","unresolved":false,"context_lines":[{"line_number":268,"context_line":"        snapshots \u003d objects.SnapshotList.get_all("},{"line_number":269,"context_line":"            self.context, search_opts)"},{"line_number":270,"context_line":"        self.assertEqual(1, len(snapshots))"},{"line_number":271,"context_line":"        TestSnapshot._compare(self, fake_snapshot, snapshots[0])"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_03ddf341","line":271,"in_reply_to":"ba3cc151_4731cea1","updated":"2015-06-29 15:54:59.000000000","message":"I think it\u0027s the right thing to do, if this test path expects it to be called, it should check the arguments (even if other tests check it as well).  But that\u0027s a matter of opinion.","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"764150860fa01de9f889f135286ac89f3a1f80f4","unresolved":false,"context_lines":[{"line_number":268,"context_line":"        snapshots \u003d objects.SnapshotList.get_all("},{"line_number":269,"context_line":"            self.context, search_opts)"},{"line_number":270,"context_line":"        self.assertEqual(1, len(snapshots))"},{"line_number":271,"context_line":"        TestSnapshot._compare(self, fake_snapshot, snapshots[0])"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_4731cea1","line":271,"in_reply_to":"ba3cc151_dd2a9c2c","updated":"2015-06-29 13:38:59.000000000","message":"I think that such assert is not necessary. One of previous test has it - so it\u0027s checked...","commit_id":"561197e9fd99f4fcea34364cde499348fc79a80b"}]}