)]}'
{"nova/objects/aggregate.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"892466788e0e7c53ec72d0d21b6decb31702a8fa","unresolved":false,"context_lines":[{"line_number":84,"context_line":"def _metadata_add_to_db(context, aggregate_id, metadata, max_retries\u003d10,"},{"line_number":85,"context_line":"                        set_delete\u003dFalse):"},{"line_number":86,"context_line":"    all_keys \u003d metadata.keys()"},{"line_number":87,"context_line":"    for attempt in range(max_retries):"},{"line_number":88,"context_line":"        try:"},{"line_number":89,"context_line":"            query \u003d context.session.query(api_models.AggregateMetadata).\\"},{"line_number":90,"context_line":"                        filter_by(aggregate_id\u003daggregate_id)"}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_f6093a96","line":87,"updated":"2016-06-01 17:34:11.000000000","message":"I don\u0027t think retrying this within a transaction works, does it? We\u0027ll continue to see the same conflict until we grab a new transaction context yes? I assume you\u0027re copying these from db_api, which may actually have been broken in this regard by the engine_facade work...","commit_id":"6720587fc4e2b54e2b3ea817140db72bed009b5c"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"55ad80bb5476af44699d556cc484221d15cb5657","unresolved":false,"context_lines":[{"line_number":84,"context_line":"def _metadata_add_to_db(context, aggregate_id, metadata, max_retries\u003d10,"},{"line_number":85,"context_line":"                        set_delete\u003dFalse):"},{"line_number":86,"context_line":"    all_keys \u003d metadata.keys()"},{"line_number":87,"context_line":"    for attempt in range(max_retries):"},{"line_number":88,"context_line":"        try:"},{"line_number":89,"context_line":"            query \u003d context.session.query(api_models.AggregateMetadata).\\"},{"line_number":90,"context_line":"                        filter_by(aggregate_id\u003daggregate_id)"}],"source_content_type":"text/x-python","patch_set":13,"id":"9abb7d3a_a947d575","line":87,"in_reply_to":"9abb7d3a_f6093a96","updated":"2016-06-07 15:26:38.000000000","message":"Can\u0027t disagree. As we are in a transaction we can\u0027t see any changes outside of the transaction until commit. Will refactor to put the transaction block inside the retry.\n\nThere is a unit test for this but it isn\u0027t using actual db access. It would be difficult to unit test this as it relies on competing concurrent transactions.","commit_id":"6720587fc4e2b54e2b3ea817140db72bed009b5c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"639cad26bdcd91092260b3c186c2389e889f04b8","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                        api_models.AggregateMetadata.__table__.insert(),"},{"line_number":118,"context_line":"                        new_entries)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"                return metadata"},{"line_number":121,"context_line":"        except db_exc.DBDuplicateEntry:"},{"line_number":122,"context_line":"            # a concurrent transaction has been committed,"},{"line_number":123,"context_line":"            # try again unless this was the last attempt"}],"source_content_type":"text/x-python","patch_set":15,"id":"7aa08908_b53fa9b2","line":120,"updated":"2016-06-08 18:17:35.000000000","message":"Could we put everything inside the try (or maybe the with) in a helper function? This is nested function-\u003efor-\u003etry-\u003ewith, and in some cases two more levels deep.","commit_id":"10d8963195840ed8dd101508260d1266ea894256"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"6229be7f78f80aca4f23d6de02010a74c458539c","unresolved":false,"context_lines":[{"line_number":118,"context_line":"                        new_entries)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"                return metadata"},{"line_number":121,"context_line":"        except db_exc.DBDuplicateEntry:"},{"line_number":122,"context_line":"            # a concurrent transaction has been committed,"},{"line_number":123,"context_line":"            # try again unless this was the last attempt"},{"line_number":124,"context_line":"            with excutils.save_and_reraise_exception() as ctxt:"}],"source_content_type":"text/x-python","patch_set":15,"id":"7aa08908_441859d3","line":121,"updated":"2016-06-08 17:22:08.000000000","message":"Hmm, unless I am mistaken, we actually *don\u0027t* want to redo the transaction here. If another thread has already saved metadata with the same key for this aggregate, we are going to just blindly overwrite what we just saved with whatever value is in the metadata parameter. Retrying the entire transaction here doesn\u0027t look right to me. Instead, the try/except DBDuplicateEntry should be *only* around lines 116-118 and even then, I\u0027m not sure I would just overwrite what had been written to the DB here... I think it\u0027s more appropriate frankly to just raise the DuplicateDBEntry back up the layers and return a 409 Conflict to the user. But then again, this is code that will be executed from within an online data migration routine, no? In that case, we can\u0027t return a 409 Conflict to the user and we are left with a choice to either pass/ignore the error or retry reading the record that was added by another thread and seeing if there is any change to the value field, and if so issuing an UPDATE statement for that key....","commit_id":"10d8963195840ed8dd101508260d1266ea894256"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"639cad26bdcd91092260b3c186c2389e889f04b8","unresolved":false,"context_lines":[{"line_number":118,"context_line":"                        new_entries)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"                return metadata"},{"line_number":121,"context_line":"        except db_exc.DBDuplicateEntry:"},{"line_number":122,"context_line":"            # a concurrent transaction has been committed,"},{"line_number":123,"context_line":"            # try again unless this was the last attempt"},{"line_number":124,"context_line":"            with excutils.save_and_reraise_exception() as ctxt:"}],"source_content_type":"text/x-python","patch_set":15,"id":"7aa08908_f5b81113","line":121,"in_reply_to":"7aa08908_441859d3","updated":"2016-06-08 18:17:35.000000000","message":"No, this is done because of an actual API call, AFAIK.\n\nI agree, however, that if we fail once we can just bail here with a 409 and make them try again from the client level. If they do that naively, then we\u0027ll end up wiping out whatever we raced with anyway, but at least they made that decision.","commit_id":"10d8963195840ed8dd101508260d1266ea894256"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"f7d6f9b4f01ff654207348f9a904b08611adfa0d","unresolved":false,"context_lines":[{"line_number":118,"context_line":"                        new_entries)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"                return metadata"},{"line_number":121,"context_line":"        except db_exc.DBDuplicateEntry:"},{"line_number":122,"context_line":"            # a concurrent transaction has been committed,"},{"line_number":123,"context_line":"            # try again unless this was the last attempt"},{"line_number":124,"context_line":"            with excutils.save_and_reraise_exception() as ctxt:"}],"source_content_type":"text/x-python","patch_set":15,"id":"7aa08908_fd63d7ae","line":121,"in_reply_to":"7aa08908_441859d3","updated":"2016-06-08 19:08:06.000000000","message":"This whole thing is super ugly. You are correct that this code will blindly overwrite the key. I think however that this was the INTENTION of the code. The original commit that added the retry loop is here:https://review.openstack.org/#/c/36880/\n\nYou can see that the transaction block is the same as in this patch. (nova/db/sqalchemy/api.py:L4961).\n\nThis logic was modified (probably unintentionally) by the enginefacade changes:\n\nhttps://review.openstack.org/#/c/262248/3/nova/db/sqlalchemy/api.py\n\nThis worries me a little. (Hopefully this was the only case where things were unintentionally changed). You are right, the correct thing to do is probably return a 409. Unfortunately a 409 isn\u0027t currently in the API, so this would be a microversion change.\n\nNot sure of the best way to proceed.","commit_id":"10d8963195840ed8dd101508260d1266ea894256"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"1c1deb326d80ace7043ec594131fa951a18c2dcc","unresolved":false,"context_lines":[{"line_number":142,"context_line":"                    msg \u003d _(\"Add metadata failed for aggregate %(id)s \""},{"line_number":143,"context_line":"                            \"after %(retries)s retries\") % \\"},{"line_number":144,"context_line":"                              {\"id\": aggregate_id, \"retries\": max_retries}"},{"line_number":145,"context_line":"                    LOG.warning(msg)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"@db_api.api_context_manager.writer"}],"source_content_type":"text/x-python","patch_set":17,"id":"3aaa91ec_dfcdbdb8","line":145,"updated":"2016-06-22 20:25:31.000000000","message":"As mentioned on a previous PS, the above isn\u0027t particularly safe or advisable, but it\u0027s the existing way things work, so... ok :)","commit_id":"9179dc16100747905edbfaa68a7466d5213c0ab0"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"1c1deb326d80ace7043ec594131fa951a18c2dcc","unresolved":false,"context_lines":[{"line_number":320,"context_line":"        for key, value in updates.items():"},{"line_number":321,"context_line":"            if value is None:"},{"line_number":322,"context_line":"                try:"},{"line_number":323,"context_line":"                    metadata_delete(self._context, self.id, key)"},{"line_number":324,"context_line":"                except exception.AggregateMetadataNotFound:"},{"line_number":325,"context_line":"                    pass"},{"line_number":326,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":17,"id":"3aaa91ec_3fc5e18d","line":323,"updated":"2016-06-22 20:25:31.000000000","message":"Same question I have about ensuring IDs match between aggregates table in cell DB and API DB?","commit_id":"9179dc16100747905edbfaa68a7466d5213c0ab0"}],"nova/tests/functional/db/test_aggregate.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4ec5e51046210b926dcd9932a4ee06858d7f30ed","unresolved":false,"context_lines":[{"line_number":10,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":11,"context_line":"#    under the License."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"from copy import deepcopy"},{"line_number":14,"context_line":"import mock"},{"line_number":15,"context_line":"from oslo_db import exception as db_exc"},{"line_number":16,"context_line":"from oslo_utils import timeutils"}],"source_content_type":"text/x-python","patch_set":25,"id":"bacf61ea_c62b3393","line":13,"updated":"2016-07-28 17:54:29.000000000","message":"nit: only import modules, not symbols from modules. exception is sqlalchemy stuffs. also, separate stdlib imports from 3rd party imports with a blank line.","commit_id":"93b44ff5a022314a92a67b145feb33a7ef9a9df9"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4ec5e51046210b926dcd9932a4ee06858d7f30ed","unresolved":false,"context_lines":[{"line_number":288,"context_line":"                        \u0027fake_new_key\u0027: \u0027fake_new_value\u0027}"},{"line_number":289,"context_line":"        metadata.update(new_metadata)"},{"line_number":290,"context_line":"        aggregate_obj._metadata_add_to_db(self.context,"},{"line_number":291,"context_line":"                                          result[\u0027id\u0027], new_metadata)"},{"line_number":292,"context_line":"        expected \u003d _aggregate_metadata_get_all(self.context, result[\u0027id\u0027])"},{"line_number":293,"context_line":"        self.assertThat(metadata, matchers.DictMatches(expected))"},{"line_number":294,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"bacf61ea_26196f89","line":291,"updated":"2016-07-28 17:54:29.000000000","message":"erm, what exactly is this test testing that isn\u0027t already tested in test_aggregate_metadata_add()? The call to aggregate_obj._metadata_add_to_db() is exactly the same -- with different parameters, but there\u0027s not \"update\" going on that this test method name implies.","commit_id":"93b44ff5a022314a92a67b145feb33a7ef9a9df9"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"dc264a9c6a51497a7c22757058a92455555a142a","unresolved":false,"context_lines":[{"line_number":288,"context_line":"                        \u0027fake_new_key\u0027: \u0027fake_new_value\u0027}"},{"line_number":289,"context_line":"        metadata.update(new_metadata)"},{"line_number":290,"context_line":"        aggregate_obj._metadata_add_to_db(self.context,"},{"line_number":291,"context_line":"                                          result[\u0027id\u0027], new_metadata)"},{"line_number":292,"context_line":"        expected \u003d _aggregate_metadata_get_all(self.context, result[\u0027id\u0027])"},{"line_number":293,"context_line":"        self.assertThat(metadata, matchers.DictMatches(expected))"},{"line_number":294,"context_line":""}],"source_content_type":"text/x-python","patch_set":25,"id":"bacf61ea_7f30fee8","line":291,"in_reply_to":"bacf61ea_26196f89","updated":"2016-07-28 22:36:30.000000000","message":"The other test creates with no metadata specifically, whereas this creates with the default set and then updates it later to make sure that works. Because of how metadata is stored, this ends up being an update instead of insert operation.","commit_id":"93b44ff5a022314a92a67b145feb33a7ef9a9df9"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4ec5e51046210b926dcd9932a4ee06858d7f30ed","unresolved":false,"context_lines":[{"line_number":293,"context_line":"        self.assertThat(metadata, matchers.DictMatches(expected))"},{"line_number":294,"context_line":""},{"line_number":295,"context_line":"    def test_aggregate_metadata_add_retry(self):"},{"line_number":296,"context_line":"        result \u003d _create_aggregate(self.context, metadata\u003dNone)"},{"line_number":297,"context_line":"        with mock.patch(\u0027nova.db.sqlalchemy.api_models.\u0027"},{"line_number":298,"context_line":"                        \u0027AggregateMetadata.__table__.insert\u0027) as insert_mock:"},{"line_number":299,"context_line":"            insert_mock.side_effect \u003d db_exc.DBDuplicateEntry"}],"source_content_type":"text/x-python","patch_set":25,"id":"bacf61ea_061c2b76","line":296,"updated":"2016-07-28 17:54:29.000000000","message":"this should be placed in the unit tests really... mocking doesn\u0027t belong in functional tests. But, meh, you can move it later.","commit_id":"93b44ff5a022314a92a67b145feb33a7ef9a9df9"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4ec5e51046210b926dcd9932a4ee06858d7f30ed","unresolved":false,"context_lines":[{"line_number":324,"context_line":"                                           list(metadata.keys())[0])"},{"line_number":325,"context_line":"        expected \u003d _aggregate_metadata_get_all(self.context, result[\u0027id\u0027])"},{"line_number":326,"context_line":"        del metadata[list(metadata.keys())[0]]"},{"line_number":327,"context_line":"        self.assertThat(metadata, matchers.DictMatches(expected))"},{"line_number":328,"context_line":""},{"line_number":329,"context_line":"    def test_aggregate_remove_availability_zone(self):"},{"line_number":330,"context_line":"        result \u003d _create_aggregate(self.context, metadata\u003d{\u0027availability_zone\u0027:"}],"source_content_type":"text/x-python","patch_set":25,"id":"bacf61ea_66136767","line":327,"updated":"2016-07-28 17:54:29.000000000","message":"Instead of lines 326-327, an easier to read and more test-logical assertion would have just been:\n\n self.assertNotIn(list(metadata.keys())[0], expected)","commit_id":"93b44ff5a022314a92a67b145feb33a7ef9a9df9"}]}