)]}'
{"nova/objects/aggregate.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b021eca32b68924f7737d99904c5185a9d70b5b0","unresolved":false,"context_lines":[{"line_number":80,"context_line":"        except db_exc.DBDuplicateEntry:"},{"line_number":81,"context_line":"            raise exception.AggregateHostExists(host\u003dhost,"},{"line_number":82,"context_line":"                                                aggregate_id\u003daggregate_id)"},{"line_number":83,"context_line":"        return host_ref"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @staticmethod"},{"line_number":86,"context_line":"    @db_api.api_context_manager.writer"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa0719c6_811ef5b8","line":83,"updated":"2016-03-23 17:52:16.000000000","message":"So, I think this is going to be a bit challenging. Given that an aggregate can have a lot of hosts, and that hosts and aggregates are in multiple pieces, it seems like we\u0027ll have a bit of a race here. I can imagine that we will have to create the aggregate in the API DB before we add hosts because of the FK. So, we do that and this suddenly thinks that the aggregate is there, and we start returning it (with no hosts). So something comes along and tries to re-insert the host in the aggregate, but races with the sync and fails.\n\nThe sync will also need to tolerate the duplication.\n\nYou could sync a whole aggregate in a single transaction I guess, but I don\u0027t know how large that will be for really big things. Maybe just some discussion required.","commit_id":"137314fed50251c7d96cf20bb79645fe374036c1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b021eca32b68924f7737d99904c5185a9d70b5b0","unresolved":false,"context_lines":[{"line_number":149,"context_line":"            return True"},{"line_number":150,"context_line":"        else:"},{"line_number":151,"context_line":"            try:"},{"line_number":152,"context_line":"                self._aggregate_get_from_db(self._context, self.id)"},{"line_number":153,"context_line":"            except exception.AggregateNotFound:"},{"line_number":154,"context_line":"                pass"},{"line_number":155,"context_line":"            return self._in_api"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa0719c6_a1d97959","line":152,"updated":"2016-03-23 17:52:16.000000000","message":"Did you mean to update self._in_api if you succeed here?","commit_id":"137314fed50251c7d96cf20bb79645fe374036c1"},{"author":{"_account_id":14511,"name":"Maciej Szankin","email":"maciej+review@szankin.pl","username":"mszankin"},"change_message_id":"8f1f59b61e31995a7246497b1b5996b4c097e968","unresolved":false,"context_lines":[{"line_number":51,"context_line":"@db_api.api_context_manager.writer"},{"line_number":52,"context_line":"def _host_add_to_db(context, aggregate_id, host):"},{"line_number":53,"context_line":"    # Check to see if the aggregate exists"},{"line_number":54,"context_line":"    _aggregate_get_from_db(context, aggregate_id)"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    host_ref \u003d api_models.AggregateHost()"},{"line_number":57,"context_line":"    host_ref.update({\"host\": host, \"aggregate_id\": aggregate_id})"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a18252c_fb1cf0c2","line":54,"range":{"start_line":54,"start_character":4,"end_line":54,"end_character":49},"updated":"2016-04-13 21:03:26.000000000","message":"Is this line necessary? If aggregates exists then nothing happens, if it does not then it raises exception that is not handled.","commit_id":"af45321cd82a744b71398b8b3cf311ef9c857442"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4b26dfecb6b36cac4c3076aaf1a4ecd2b3ccde20","unresolved":false,"context_lines":[{"line_number":51,"context_line":"@db_api.api_context_manager.writer"},{"line_number":52,"context_line":"def _host_add_to_db(context, aggregate_id, host):"},{"line_number":53,"context_line":"    # Check to see if the aggregate exists"},{"line_number":54,"context_line":"    _aggregate_get_from_db(context, aggregate_id)"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    host_ref \u003d api_models.AggregateHost()"},{"line_number":57,"context_line":"    host_ref.update({\"host\": host, \"aggregate_id\": aggregate_id})"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a18252c_0e36c8e5","line":54,"range":{"start_line":54,"start_character":4,"end_line":54,"end_character":49},"in_reply_to":"5a18252c_fb1cf0c2","updated":"2016-04-13 21:10:20.000000000","message":"Yes, it is necessary. The point is so that this call raises AggregateNotFound if there is no such aggregate_id in the database. The goal is for it to be unhandled here so that the caller gets it.","commit_id":"af45321cd82a744b71398b8b3cf311ef9c857442"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4b26dfecb6b36cac4c3076aaf1a4ecd2b3ccde20","unresolved":false,"context_lines":[{"line_number":147,"context_line":""},{"line_number":148,"context_line":"    @property"},{"line_number":149,"context_line":"    def in_api(self):"},{"line_number":150,"context_line":"        if self._in_api:"},{"line_number":151,"context_line":"            return True"},{"line_number":152,"context_line":"        else:"},{"line_number":153,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a18252c_6e5bfc2e","line":150,"updated":"2016-04-13 21:10:20.000000000","message":"Note that this will only be cached on one side of a call...","commit_id":"af45321cd82a744b71398b8b3cf311ef9c857442"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4b26dfecb6b36cac4c3076aaf1a4ecd2b3ccde20","unresolved":false,"context_lines":[{"line_number":243,"context_line":""},{"line_number":244,"context_line":"    @base.remotable"},{"line_number":245,"context_line":"    def add_host(self, host):"},{"line_number":246,"context_line":"        if self.in_api:"},{"line_number":247,"context_line":"            _host_add_to_db(self._context, self.id, host)"},{"line_number":248,"context_line":"        else:"},{"line_number":249,"context_line":"            db.aggregate_host_add(self._context, self.id, host)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a18252c_aeab54d5","line":246,"updated":"2016-04-13 21:10:20.000000000","message":"This will always be false after an RPC call. Since this is remotable, that means we will basically never get the benefit from across the RPC wire.\n\nI\u0027m guessing this only happens from the API, which doesn\u0027t (currently) make remotable calls. However, it might be worth noting here. Alternately, I think you could just do the try-in-api-else-try-in-main approach here, no?","commit_id":"af45321cd82a744b71398b8b3cf311ef9c857442"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4b26dfecb6b36cac4c3076aaf1a4ecd2b3ccde20","unresolved":false,"context_lines":[{"line_number":246,"context_line":"        if self.in_api:"},{"line_number":247,"context_line":"            _host_add_to_db(self._context, self.id, host)"},{"line_number":248,"context_line":"        else:"},{"line_number":249,"context_line":"            db.aggregate_host_add(self._context, self.id, host)"},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        if self.hosts is None:"},{"line_number":252,"context_line":"            self.hosts \u003d []"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a18252c_4e4e0070","line":249,"updated":"2016-04-13 21:10:20.000000000","message":"This raises AggregateNotFound as well, so the above makes L247 behave the same way.","commit_id":"af45321cd82a744b71398b8b3cf311ef9c857442"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"39ff6d4ac4c9eb9ecdad4aa401927e5982d37d2f","unresolved":false,"context_lines":[{"line_number":247,"context_line":"            _host_add_to_db(self._context, self.id, host)"},{"line_number":248,"context_line":"        else:"},{"line_number":249,"context_line":"            db.aggregate_host_add(self._context, self.id, host)"},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        if self.hosts is None:"},{"line_number":252,"context_line":"            self.hosts \u003d []"},{"line_number":253,"context_line":"        self.hosts.append(host)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bab6814e_168c7534","line":250,"updated":"2016-05-24 13:19:44.000000000","message":"fortunately, that works without being racy because we\u0027re only creating or updating aggregates in the cell DB, right?","commit_id":"c7d95cdaf41413f1164bd08123cfe5409ef4da11"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"20716cb5fee644e9f53e701d7bc4719b006a6182","unresolved":false,"context_lines":[{"line_number":247,"context_line":"            _host_add_to_db(self._context, self.id, host)"},{"line_number":248,"context_line":"        else:"},{"line_number":249,"context_line":"            db.aggregate_host_add(self._context, self.id, host)"},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        if self.hosts is None:"},{"line_number":252,"context_line":"            self.hosts \u003d []"},{"line_number":253,"context_line":"        self.hosts.append(host)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bab6814e_4be9eb19","line":250,"in_reply_to":"bab6814e_168c7534","updated":"2016-05-25 16:23:33.000000000","message":"There isn\u0027t a race condition associated with this code itself. The aggregate either lives in the API db or the Cell DB.\n\nThe race condition occurs when we try and migrate aggregates between the two of them.\n\nSo any racyness is going to occur in the middle of migrating. We check if it is in the API, it isn\u0027t. So we go to add it to the data in the main db. In this time the migration function has moved it to the API db and deleted the aggregate from the main db. The host add should fail.\n\nActually I need to add to this series. As there is a race condition here, I should make sure that all update operations fail until the migration is complete in the same way as has been done for the _create function in the last patch of this series.","commit_id":"c7d95cdaf41413f1164bd08123cfe5409ef4da11"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"c02bc4f009dc4b709a53dccce67a326304f4bf9b","unresolved":false,"context_lines":[{"line_number":247,"context_line":"            _host_add_to_db(self._context, self.id, host)"},{"line_number":248,"context_line":"        else:"},{"line_number":249,"context_line":"            db.aggregate_host_add(self._context, self.id, host)"},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        if self.hosts is None:"},{"line_number":252,"context_line":"            self.hosts \u003d []"},{"line_number":253,"context_line":"        self.hosts.append(host)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bab6814e_26881271","line":250,"in_reply_to":"bab6814e_4be9eb19","updated":"2016-05-25 20:14:45.000000000","message":"So, as we discussed over IRC, both the migration and this API call are for admin operators, not single users, which means they should know that there is a running migration while they modify the aggregate. That said, we could maybe think of having a conservative negative call when the admin modifies the aggregates API while a migration is running, instead of leaving a way to get that race condition.","commit_id":"c7d95cdaf41413f1164bd08123cfe5409ef4da11"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"20716cb5fee644e9f53e701d7bc4719b006a6182","unresolved":false,"context_lines":[{"line_number":258,"context_line":"        if self.in_api:"},{"line_number":259,"context_line":"            _host_delete_from_db(self._context, self.id, host)"},{"line_number":260,"context_line":"        else:"},{"line_number":261,"context_line":"            db.aggregate_host_delete(self._context, self.id, host)"},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"        self.hosts.remove(host)"},{"line_number":264,"context_line":"        self.obj_reset_changes(fields\u003d[\u0027hosts\u0027])"}],"source_content_type":"text/x-python","patch_set":9,"id":"bab6814e_eb441fb8","line":261,"updated":"2016-05-25 16:23:33.000000000","message":"Again, race condition here. But again the delete will fail and the operation can be repeated to get the correct results. Operator only.","commit_id":"c7d95cdaf41413f1164bd08123cfe5409ef4da11"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a1da55bad6b24275a0aeafb43c269d2a31904ab5","unresolved":false,"context_lines":[{"line_number":59,"context_line":"        host_ref.save(context.session)"},{"line_number":60,"context_line":"    except db_exc.DBDuplicateEntry:"},{"line_number":61,"context_line":"        raise exception.AggregateHostExists(host\u003dhost,"},{"line_number":62,"context_line":"                                            aggregate_id\u003daggregate_id)"},{"line_number":63,"context_line":"    return host_ref"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_76cc2a89","line":62,"updated":"2016-06-01 17:29:31.000000000","message":"This can\u0027t happen right? We\u0027re inside a transaction here, which means if we passed line 54, we won\u0027t get a conflict here correct? I think you need to put this check around this whole function so that we catch the actual race and raise the right exception.","commit_id":"b557a17ebee1b233498b32ea3582c3d4cadba314"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ddea549fb75e0a200241e61cc07c2134e4ed5ab8","unresolved":false,"context_lines":[{"line_number":59,"context_line":"        host_ref.save(context.session)"},{"line_number":60,"context_line":"    except db_exc.DBDuplicateEntry:"},{"line_number":61,"context_line":"        raise exception.AggregateHostExists(host\u003dhost,"},{"line_number":62,"context_line":"                                            aggregate_id\u003daggregate_id)"},{"line_number":63,"context_line":"    return host_ref"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_e6347bfc","line":62,"in_reply_to":"7aa08908_26396342","updated":"2016-06-06 20:09:51.000000000","message":"Oh duh, right, thanks ;)\n\nI guess I\u0027m still curious about where the exception is raised, because I\u0027m not sure whether the transaction start on L51 grabs a consistent view of the DB there, or only once we query a thing. If grabs a snapshot there, then we could race with an updater between L52 and L59.","commit_id":"b557a17ebee1b233498b32ea3582c3d4cadba314"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"4f4820f091b906a38f87798adf90f149dfaafc63","unresolved":false,"context_lines":[{"line_number":59,"context_line":"        host_ref.save(context.session)"},{"line_number":60,"context_line":"    except db_exc.DBDuplicateEntry:"},{"line_number":61,"context_line":"        raise exception.AggregateHostExists(host\u003dhost,"},{"line_number":62,"context_line":"                                            aggregate_id\u003daggregate_id)"},{"line_number":63,"context_line":"    return host_ref"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_e6590c5d","line":62,"in_reply_to":"9abb7d3a_76cc2a89","updated":"2016-06-03 15:03:44.000000000","message":"Not sure what you mean here. The \u0027aggregate_get_from_db\u0027 call is to check that the aggregate exists at all. We could still get a conflict even if the aggregate exists as we are modifying the aggregate_host table in such a way as to fail to pass the unique \u0027aggregate_id, host\u0027 constraint on that table.","commit_id":"b557a17ebee1b233498b32ea3582c3d4cadba314"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"adf656a394974aec10e08fbe9f52775ac8738f94","unresolved":false,"context_lines":[{"line_number":59,"context_line":"        host_ref.save(context.session)"},{"line_number":60,"context_line":"    except db_exc.DBDuplicateEntry:"},{"line_number":61,"context_line":"        raise exception.AggregateHostExists(host\u003dhost,"},{"line_number":62,"context_line":"                                            aggregate_id\u003daggregate_id)"},{"line_number":63,"context_line":"    return host_ref"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"7aa08908_26396342","line":62,"in_reply_to":"9abb7d3a_e6590c5d","updated":"2016-06-06 20:01:46.000000000","message":"Ok so the aggregate_get_from_db call has nothing to do with the aggregate host. This exception won\u0027t be missed. The DuplicateEntry exception will happen before the the transaction commit.","commit_id":"b557a17ebee1b233498b32ea3582c3d4cadba314"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a1da55bad6b24275a0aeafb43c269d2a31904ab5","unresolved":false,"context_lines":[{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    if count \u003d\u003d 0:"},{"line_number":77,"context_line":"        raise exception.AggregateHostNotFound(aggregate_id\u003daggregate_id,"},{"line_number":78,"context_line":"                                              host\u003dhost)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"@base.NovaObjectRegistry.register"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_56aac685","line":78,"updated":"2016-06-01 17:29:31.000000000","message":"Same here I think.","commit_id":"b557a17ebee1b233498b32ea3582c3d4cadba314"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"4f4820f091b906a38f87798adf90f149dfaafc63","unresolved":false,"context_lines":[{"line_number":75,"context_line":""},{"line_number":76,"context_line":"    if count \u003d\u003d 0:"},{"line_number":77,"context_line":"        raise exception.AggregateHostNotFound(aggregate_id\u003daggregate_id,"},{"line_number":78,"context_line":"                                              host\u003dhost)"},{"line_number":79,"context_line":""},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"@base.NovaObjectRegistry.register"}],"source_content_type":"text/x-python","patch_set":12,"id":"9abb7d3a_264af4ac","line":78,"in_reply_to":"9abb7d3a_56aac685","updated":"2016-06-03 15:03:44.000000000","message":"I think you are right here. The _aggregate_get_from_db call should have made sure that the aggregate exists. Theoretically that should mean that the \u0027count\u0027 value is always greater than 0 as a delete should have occured. This if statement is either extraneous or defensive programming. :)","commit_id":"b557a17ebee1b233498b32ea3582c3d4cadba314"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"96de6908068685730e474c3d0047fbf219cd7519","unresolved":false,"context_lines":[{"line_number":63,"context_line":"                                            aggregate_id\u003daggregate_id)"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"@db_api.api_context_manager.writer"},{"line_number":67,"context_line":"def _host_delete_from_db(context, aggregate_id, host):"},{"line_number":68,"context_line":"    count \u003d 0"},{"line_number":69,"context_line":"    with db_api.api_context_manager.writer.using(context):"}],"source_content_type":"text/x-python","patch_set":14,"id":"7aa08908_da7a64bc","line":66,"range":{"start_line":66,"start_character":0,"end_line":66,"end_character":34},"updated":"2016-06-08 18:08:45.000000000","message":"Shouldn\u0027t this be removed now?","commit_id":"2d343fceb8ba6250cae0d68335b92a0a912a95e8"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"4ddb28bba69a2f24ab69f53d31c2cb94ccf02d69","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"def _host_add_to_db(context, aggregate_id, host):"},{"line_number":67,"context_line":"    try:"},{"line_number":68,"context_line":"        with db_api.api_context_manager.writer.using(context):"},{"line_number":69,"context_line":"            # Check to see if the aggregate exists"},{"line_number":70,"context_line":"            _aggregate_get_from_db(context, aggregate_id)"},{"line_number":71,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"7aa08908_9517a644","line":68,"updated":"2016-06-16 10:35:37.000000000","message":"maybe a stupid question, why aren\u0027t you not using the decorator instead of the contextmanager, given the current with statement encompasses all the method?\n\nEDIT: Doh, nevermind, just saw good Dan\u0027s point in PS12.","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"73fe8900c9b1dd29fb108d5e2ff8f8a0d60a8bd6","unresolved":false,"context_lines":[{"line_number":168,"context_line":"            return True"},{"line_number":169,"context_line":"        else:"},{"line_number":170,"context_line":"            try:"},{"line_number":171,"context_line":"                _aggregate_get_from_db(self._context, self.id)"},{"line_number":172,"context_line":"                self._in_api \u003d True"},{"line_number":173,"context_line":"            except exception.AggregateNotFound:"},{"line_number":174,"context_line":"                pass"}],"source_content_type":"text/x-python","patch_set":16,"id":"3aaa91ec_df23fda5","line":171,"updated":"2016-06-22 20:18:27.000000000","message":"But won\u0027t self.id be a different autoincrement key from the cell DB? How are we ensuring that the autoincrement\u0027ing IDs match from the cell DB? How about looking up via UUID instead and get rid of that problem?","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"4ddb28bba69a2f24ab69f53d31c2cb94ccf02d69","unresolved":false,"context_lines":[{"line_number":172,"context_line":"                self._in_api \u003d True"},{"line_number":173,"context_line":"            except exception.AggregateNotFound:"},{"line_number":174,"context_line":"                pass"},{"line_number":175,"context_line":"            return self._in_api"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"    @base.remotable_classmethod"},{"line_number":178,"context_line":"    def get_by_id(cls, context, aggregate_id):"}],"source_content_type":"text/x-python","patch_set":16,"id":"7aa08908_9a7b55be","line":175,"updated":"2016-06-16 10:35:37.000000000","message":"thanks for reproducing the Flavor pattern.","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"4a09a32db4bf7f61b7fecc7cb9549cb882f2bdf9","unresolved":false,"context_lines":[{"line_number":63,"context_line":"    return aggregate"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"def _host_add_to_db(context, aggregate_id, host):"},{"line_number":67,"context_line":"    try:"},{"line_number":68,"context_line":"        with db_api.api_context_manager.writer.using(context):"},{"line_number":69,"context_line":"            # Check to see if the aggregate exists"}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_4de5c2a6","line":66,"updated":"2016-07-08 19:31:02.000000000","message":"These two methods should be on the object class. There are some examples of doing it at the module level like this but that was due to a bug in the fixtures library with python 3. We put them on the object because subclassing the object and overriding them is the potential mechanism for changing the persistence layer. That\u0027s not in place yet but it\u0027s something that has been discussed for a bit.\n\nThis is a nit for the moment, it can wait for a full cleanup at some point.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"40f02ae90a88763ecec45ab383dd8de964cd470d","unresolved":false,"context_lines":[{"line_number":63,"context_line":"    return aggregate"},{"line_number":64,"context_line":""},{"line_number":65,"context_line":""},{"line_number":66,"context_line":"def _host_add_to_db(context, aggregate_id, host):"},{"line_number":67,"context_line":"    try:"},{"line_number":68,"context_line":"        with db_api.api_context_manager.writer.using(context):"},{"line_number":69,"context_line":"            # Check to see if the aggregate exists"}],"source_content_type":"text/x-python","patch_set":17,"id":"dada55a8_fc7351e5","line":66,"in_reply_to":"1aa78d24_4de5c2a6","updated":"2016-07-19 16:04:51.000000000","message":"I originally had these on the object. They were moved out because of the reason mentioned above. If that isn\u0027t fixed I\u0027ll wait to move these back to the object.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"4a09a32db4bf7f61b7fecc7cb9549cb882f2bdf9","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"def _host_add_to_db(context, aggregate_id, host):"},{"line_number":67,"context_line":"    try:"},{"line_number":68,"context_line":"        with db_api.api_context_manager.writer.using(context):"},{"line_number":69,"context_line":"            # Check to see if the aggregate exists"},{"line_number":70,"context_line":"            _aggregate_get_from_db(context, aggregate_id)"},{"line_number":71,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_8dedea90","line":68,"updated":"2016-07-08 19:31:02.000000000","message":"Why not use the method decorator?","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"40f02ae90a88763ecec45ab383dd8de964cd470d","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"def _host_add_to_db(context, aggregate_id, host):"},{"line_number":67,"context_line":"    try:"},{"line_number":68,"context_line":"        with db_api.api_context_manager.writer.using(context):"},{"line_number":69,"context_line":"            # Check to see if the aggregate exists"},{"line_number":70,"context_line":"            _aggregate_get_from_db(context, aggregate_id)"},{"line_number":71,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"dada55a8_9c64d522","line":68,"in_reply_to":"1aa78d24_8dedea90","updated":"2016-07-19 16:04:51.000000000","message":"There is a very good reason that this has been moved out from a decorator.\nThis was because of the misgivings I and Dan Smith had about whether the DBDuplicateEntry exception would be properly triggered on contending transactions with the wrapper. See the previous versions and comments.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"0715740700a73cf0611119575fb7eeea97611483","unresolved":false,"context_lines":[{"line_number":65,"context_line":""},{"line_number":66,"context_line":"def _host_add_to_db(context, aggregate_id, host):"},{"line_number":67,"context_line":"    try:"},{"line_number":68,"context_line":"        with db_api.api_context_manager.writer.using(context):"},{"line_number":69,"context_line":"            # Check to see if the aggregate exists"},{"line_number":70,"context_line":"            _aggregate_get_from_db(context, aggregate_id)"},{"line_number":71,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"dada55a8_e768c789","line":68,"in_reply_to":"dada55a8_9c64d522","updated":"2016-07-19 23:29:24.000000000","message":"It would be helpful to have a comment to this effect.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"d880c7027ad90f0fe808ef9a137a97c5a726b024","unresolved":false,"context_lines":[{"line_number":70,"context_line":"            _aggregate_get_from_db(context, aggregate_id)"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"            host_ref \u003d api_models.AggregateHost()"},{"line_number":73,"context_line":"            host_ref.update({\"host\": host, \"aggregate_id\": aggregate_id})"},{"line_number":74,"context_line":"            host_ref.save(context.session)"},{"line_number":75,"context_line":"            return host_ref"},{"line_number":76,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_a380b747","line":73,"updated":"2016-07-08 19:53:52.000000000","message":"@alaski: Sorry, I don\u0027t understand you on this one. host_ref is a brand new object model of type api_models.AggregateHost. It wasn\u0027t looked up at all.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"4a09a32db4bf7f61b7fecc7cb9549cb882f2bdf9","unresolved":false,"context_lines":[{"line_number":70,"context_line":"            _aggregate_get_from_db(context, aggregate_id)"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"            host_ref \u003d api_models.AggregateHost()"},{"line_number":73,"context_line":"            host_ref.update({\"host\": host, \"aggregate_id\": aggregate_id})"},{"line_number":74,"context_line":"            host_ref.save(context.session)"},{"line_number":75,"context_line":"            return host_ref"},{"line_number":76,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_0d773ad5","line":73,"range":{"start_line":73,"start_character":43,"end_line":73,"end_character":71},"updated":"2016-07-08 19:31:02.000000000","message":"This is unecessary right? The aggregate was looked up with this key so there should be no need to reset it.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"2872aed35e6bb5e1ad0d35548ec847e4dd24526f","unresolved":false,"context_lines":[{"line_number":70,"context_line":"            _aggregate_get_from_db(context, aggregate_id)"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"            host_ref \u003d api_models.AggregateHost()"},{"line_number":73,"context_line":"            host_ref.update({\"host\": host, \"aggregate_id\": aggregate_id})"},{"line_number":74,"context_line":"            host_ref.save(context.session)"},{"line_number":75,"context_line":"            return host_ref"},{"line_number":76,"context_line":"    except db_exc.DBDuplicateEntry:"}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_e3211fb6","line":73,"in_reply_to":"1aa78d24_a380b747","updated":"2016-07-08 20:21:41.000000000","message":"Yeah, I has a dumb. I somehow thought that this object was returned from the aggregate lookup and was being updated.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"d880c7027ad90f0fe808ef9a137a97c5a726b024","unresolved":false,"context_lines":[{"line_number":82,"context_line":"    count \u003d 0"},{"line_number":83,"context_line":"    with db_api.api_context_manager.writer.using(context):"},{"line_number":84,"context_line":"        # Check to see if the aggregate exists"},{"line_number":85,"context_line":"        _aggregate_get_from_db(context, aggregate_id)"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"        query \u003d context.session.query(api_models.AggregateHost)"},{"line_number":88,"context_line":"        query \u003d query.filter(api_models.AggregateHost.aggregate_id \u003d\u003d"}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_0396eb93","line":85,"updated":"2016-07-08 19:53:52.000000000","message":"@alaski: The above checks for the aggregate record, not the aggregate_host mapping record, which the count check below checks for.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"4a09a32db4bf7f61b7fecc7cb9549cb882f2bdf9","unresolved":false,"context_lines":[{"line_number":82,"context_line":"    count \u003d 0"},{"line_number":83,"context_line":"    with db_api.api_context_manager.writer.using(context):"},{"line_number":84,"context_line":"        # Check to see if the aggregate exists"},{"line_number":85,"context_line":"        _aggregate_get_from_db(context, aggregate_id)"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"        query \u003d context.session.query(api_models.AggregateHost)"},{"line_number":88,"context_line":"        query \u003d query.filter(api_models.AggregateHost.aggregate_id \u003d\u003d"}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_0d5e1a45","line":85,"updated":"2016-07-08 19:31:02.000000000","message":"The count check below should be equivalent to this check so this is unnecessary.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"2872aed35e6bb5e1ad0d35548ec847e4dd24526f","unresolved":false,"context_lines":[{"line_number":82,"context_line":"    count \u003d 0"},{"line_number":83,"context_line":"    with db_api.api_context_manager.writer.using(context):"},{"line_number":84,"context_line":"        # Check to see if the aggregate exists"},{"line_number":85,"context_line":"        _aggregate_get_from_db(context, aggregate_id)"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"        query \u003d context.session.query(api_models.AggregateHost)"},{"line_number":88,"context_line":"        query \u003d query.filter(api_models.AggregateHost.aggregate_id \u003d\u003d"}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_1e1abe6e","line":85,"in_reply_to":"1aa78d24_0396eb93","updated":"2016-07-08 20:21:41.000000000","message":"Yep. I somehow kept missing the different between the aggregate and aggregatehost. I still think this is unecessary though. Does it matter if this exists before we delete the aggregatehost?","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"}],"nova/tests/functional/db/test_aggregate.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"d880c7027ad90f0fe808ef9a137a97c5a726b024","unresolved":false,"context_lines":[{"line_number":91,"context_line":""},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"@db_api.api_context_manager.reader"},{"line_number":94,"context_line":"def _aggregate_host_get_all(context, aggregate_id):"},{"line_number":95,"context_line":"    return context.session.query(api_models.AggregateHost).\\"},{"line_number":96,"context_line":"                       filter_by(aggregate_id\u003daggregate_id).all()"},{"line_number":97,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_c37b437a","line":94,"updated":"2016-07-08 19:53:52.000000000","message":"@alaski: I think the idea here is to have some checks-and-balances where we check an object call with a result from directly using the database or vice versa...","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"4a09a32db4bf7f61b7fecc7cb9549cb882f2bdf9","unresolved":false,"context_lines":[{"line_number":91,"context_line":""},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"@db_api.api_context_manager.reader"},{"line_number":94,"context_line":"def _aggregate_host_get_all(context, aggregate_id):"},{"line_number":95,"context_line":"    return context.session.query(api_models.AggregateHost).\\"},{"line_number":96,"context_line":"                       filter_by(aggregate_id\u003daggregate_id).all()"},{"line_number":97,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_40be2900","line":94,"updated":"2016-07-08 19:31:02.000000000","message":"Why have these three methods interact with the db directly rather than using the aggregate module methods? That would exercise those methods further.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"40f02ae90a88763ecec45ab383dd8de964cd470d","unresolved":false,"context_lines":[{"line_number":91,"context_line":""},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"@db_api.api_context_manager.reader"},{"line_number":94,"context_line":"def _aggregate_host_get_all(context, aggregate_id):"},{"line_number":95,"context_line":"    return context.session.query(api_models.AggregateHost).\\"},{"line_number":96,"context_line":"                       filter_by(aggregate_id\u003daggregate_id).all()"},{"line_number":97,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"dada55a8_478d667a","line":94,"in_reply_to":"1aa78d24_c37b437a","updated":"2016-07-19 16:04:51.000000000","message":"These tests originally were in unit tests. These functions were somewhat copied from the db/api.py unit tests where there were direct db creation functions. As they have been moved to functional it may make sense to get rid of these and use functions directly.\n\nHowever, because there is a series of patches that make up the migrations we will need to have these functions temporarily at least because the object functions to create aggregates, metadata, hosts in the API db haven\u0027t been implemented early in the series.\n\nI wouldn\u0027t mind keeping these. The tests came from db/api.py and are written for direct database structures rather than objects. :/","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"2872aed35e6bb5e1ad0d35548ec847e4dd24526f","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        self.assertEqual(2, len(rl1))"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def test_aggregate_host_add_to_db(self):"},{"line_number":195,"context_line":"        result \u003d _create_aggregate_with_hosts(self.context,"},{"line_number":196,"context_line":"                                              metadata\u003dNone)"},{"line_number":197,"context_line":"        expected \u003d aggregate_obj._aggregate_get_from_db(self.context,"},{"line_number":198,"context_line":"                                                        result[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":17,"id":"1aa78d24_9e450e19","line":195,"updated":"2016-07-08 20:21:41.000000000","message":"Jay, this is my concern with having the db methods here. I would expect this test to check the aggregate module _host_add_to_db method, but it does not do that. Instead this test mostly checks that the helper method here works.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"40f02ae90a88763ecec45ab383dd8de964cd470d","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        self.assertEqual(2, len(rl1))"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def test_aggregate_host_add_to_db(self):"},{"line_number":195,"context_line":"        result \u003d _create_aggregate_with_hosts(self.context,"},{"line_number":196,"context_line":"                                              metadata\u003dNone)"},{"line_number":197,"context_line":"        expected \u003d aggregate_obj._aggregate_get_from_db(self.context,"},{"line_number":198,"context_line":"                                                        result[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":17,"id":"dada55a8_e710da03","line":195,"in_reply_to":"1aa78d24_9e450e19","updated":"2016-07-19 16:04:51.000000000","message":"The intention was to test the helper methods. There is a patch later in the series that adds to this file with new test objects that adds more functional, whole system tests for the objects. This happens in the \u0027create\u0027 patch in this series as its the place where the object could be properly functionally tested.\n\nAs mentioned earlier these tests originally lived in unit tests. Jay and others asked me to move them in to functional as they use real database access.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"0715740700a73cf0611119575fb7eeea97611483","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        self.assertEqual(2, len(rl1))"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"    def test_aggregate_host_add_to_db(self):"},{"line_number":195,"context_line":"        result \u003d _create_aggregate_with_hosts(self.context,"},{"line_number":196,"context_line":"                                              metadata\u003dNone)"},{"line_number":197,"context_line":"        expected \u003d aggregate_obj._aggregate_get_from_db(self.context,"},{"line_number":198,"context_line":"                                                        result[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":17,"id":"dada55a8_c7cf837d","line":195,"in_reply_to":"dada55a8_e710da03","updated":"2016-07-19 23:29:24.000000000","message":"I completely agree with moving these to functional tests because of the database accesses. \n\nIf these are going to be testing the helper methods then moving them to a separate test class would be good so it\u0027s clear when a test should be checking an object versus when it\u0027s checking a helper method. The existence of a test method named test_aggregate_host_add_to_db here makes me expect that it\u0027s checking that a host can be added to an aggregate via non test code, but that\u0027s not what this does.","commit_id":"1caabb9c0b49876c13af936556aba52fea4a4ddd"}],"nova/tests/unit/objects/test_aggregate.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"93caaea05977e70b9c18d85b120a9d74b3731404","unresolved":false,"context_lines":[{"line_number":231,"context_line":"        cell_db_agg \u003d aggregate.Aggregate.get_by_id(self.context, ca2[\u0027id\u0027])"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"        self.assertEqual(api_db_agg.in_api, True)"},{"line_number":234,"context_line":"        self.assertEqual(cell_db_agg.in_api, False)"},{"line_number":235,"context_line":""},{"line_number":236,"context_line":"    @mock.patch(\u0027nova.objects.aggregate._aggregate_get_from_db\u0027)"},{"line_number":237,"context_line":"    @mock.patch(\u0027nova.db.aggregate_get\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bab6814e_1aaafa85","line":234,"updated":"2016-05-23 22:59:09.000000000","message":"Yup, good tests. ++","commit_id":"c7d95cdaf41413f1164bd08123cfe5409ef4da11"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a9027f2649ad08a3d5e1fed8347feb4c9f19f4c3","unresolved":false,"context_lines":[{"line_number":373,"context_line":"        agg.add_host(\u0027bar\u0027)"},{"line_number":374,"context_line":"        self.assertEqual(agg.hosts, [\u0027foo\u0027, \u0027bar\u0027])"},{"line_number":375,"context_line":"        mock_host_add_api.assert_called_once_with(self.context, 123, \u0027bar\u0027)"},{"line_number":376,"context_line":"        mock_host_add.assert_not_called()"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"    @mock.patch(\u0027nova.db.aggregate_host_delete\u0027)"},{"line_number":379,"context_line":"    def test_delete_host(self, mock_host_delete):"}],"source_content_type":"text/x-python","patch_set":13,"id":"7aa08908_4c33adf6","line":376,"updated":"2016-06-07 18:50:04.000000000","message":"Note that only the mock library in Py3K has an assert_not_called() method on the Mock class. In Python 2.7, the above is actually a magically-mocked-out method which actually does *not* check that the mock was not called. Instead, a compatible (with Py2) way of doing the above is:\n\n self.assertFalse(most_host_add.called)\n\nHere and throughout.","commit_id":"6870b1eec69ca826afeca2643f3471fbce2d864b"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"01d31cd81f45e4f2bff9fc5747f0625437393e9e","unresolved":false,"context_lines":[{"line_number":373,"context_line":"        agg.add_host(\u0027bar\u0027)"},{"line_number":374,"context_line":"        self.assertEqual(agg.hosts, [\u0027foo\u0027, \u0027bar\u0027])"},{"line_number":375,"context_line":"        mock_host_add_api.assert_called_once_with(self.context, 123, \u0027bar\u0027)"},{"line_number":376,"context_line":"        mock_host_add.assert_not_called()"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"    @mock.patch(\u0027nova.db.aggregate_host_delete\u0027)"},{"line_number":379,"context_line":"    def test_delete_host(self, mock_host_delete):"}],"source_content_type":"text/x-python","patch_set":13,"id":"7aa08908_6cdbcc1b","line":376,"in_reply_to":"7aa08908_4c33adf6","updated":"2016-06-08 14:31:03.000000000","message":"That..... Is extremely annoying. I think I have used this in a good number of places. Thanks for pointing this out. Will modify this series to use assertFalse.","commit_id":"6870b1eec69ca826afeca2643f3471fbce2d864b"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"73fe8900c9b1dd29fb108d5e2ff8f8a0d60a8bd6","unresolved":false,"context_lines":[{"line_number":92,"context_line":""},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"@db_api.api_context_manager.reader"},{"line_number":95,"context_line":"def _aggregate_host_get_all(context, aggregate_id):"},{"line_number":96,"context_line":"    return context.session.query(api_models.AggregateHost).\\"},{"line_number":97,"context_line":"                       filter_by(aggregate_id\u003daggregate_id).all()"},{"line_number":98,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"3aaa91ec_5f19cd78","line":95,"updated":"2016-06-22 20:18:27.000000000","message":"should be _aggregate_host_get_all_by_aggregate_id() since you aren\u0027t actually getting all agg-host mapping records but only the ones for a specific aggregate.","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"8d5658b3fd7689e9b35dc350f9257ec966a86c61","unresolved":false,"context_lines":[{"line_number":92,"context_line":""},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"@db_api.api_context_manager.reader"},{"line_number":95,"context_line":"def _aggregate_host_get_all(context, aggregate_id):"},{"line_number":96,"context_line":"    return context.session.query(api_models.AggregateHost).\\"},{"line_number":97,"context_line":"                       filter_by(aggregate_id\u003daggregate_id).all()"},{"line_number":98,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"3aaa91ec_b0d0b0f7","line":95,"in_reply_to":"3aaa91ec_5f19cd78","updated":"2016-06-23 15:29:52.000000000","message":"This is true. A factual statement. :) Will modify the name to match what it actually does.","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"73fe8900c9b1dd29fb108d5e2ff8f8a0d60a8bd6","unresolved":false,"context_lines":[{"line_number":94,"context_line":"@db_api.api_context_manager.reader"},{"line_number":95,"context_line":"def _aggregate_host_get_all(context, aggregate_id):"},{"line_number":96,"context_line":"    return context.session.query(api_models.AggregateHost).\\"},{"line_number":97,"context_line":"                       filter_by(aggregate_id\u003daggregate_id).all()"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"class _TestAggregateObject(object):"}],"source_content_type":"text/x-python","patch_set":16,"id":"3aaa91ec_bf26f1b7","line":97,"updated":"2016-06-22 20:18:27.000000000","message":"@doffm: Note that test_db_api.py is the *exception* though, not the rule :)","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"4ddb28bba69a2f24ab69f53d31c2cb94ccf02d69","unresolved":false,"context_lines":[{"line_number":94,"context_line":"@db_api.api_context_manager.reader"},{"line_number":95,"context_line":"def _aggregate_host_get_all(context, aggregate_id):"},{"line_number":96,"context_line":"    return context.session.query(api_models.AggregateHost).\\"},{"line_number":97,"context_line":"                       filter_by(aggregate_id\u003daggregate_id).all()"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"class _TestAggregateObject(object):"}],"source_content_type":"text/x-python","patch_set":16,"id":"7aa08908_35407207","line":97,"updated":"2016-06-16 10:35:37.000000000","message":"sure, I understand why you\u0027re defining that method here for testing but given it\u0027s an UT only (and not a functional test), we should maybe just verify the asserted calls; and really if you want to do those DB-persisted tests, then do them either in a separate testcase or even better in the functional tests tree (but I can\u0027t find a right existing functional TestCase for that object layer)","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":18247,"name":"Mark Doffman","email":"mdoffman@linux.vnet.ibm.com","username":"doffm"},"change_message_id":"ef31890622977d4a78eb501dc292585a924abdfc","unresolved":false,"context_lines":[{"line_number":94,"context_line":"@db_api.api_context_manager.reader"},{"line_number":95,"context_line":"def _aggregate_host_get_all(context, aggregate_id):"},{"line_number":96,"context_line":"    return context.session.query(api_models.AggregateHost).\\"},{"line_number":97,"context_line":"                       filter_by(aggregate_id\u003daggregate_id).all()"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"class _TestAggregateObject(object):"}],"source_content_type":"text/x-python","patch_set":16,"id":"7aa08908_fa6b6bef","line":97,"in_reply_to":"7aa08908_35407207","updated":"2016-06-16 14:07:44.000000000","message":"Later in the series a functional test is added for the object and its database access. https://review.openstack.org/#/c/314781/  Looking at it there is no reason I can\u0027t split that out and put it at the front of the series if you would like. There is certainly some duplication between these unit tests and those functional tests. I don\u0027t mind test duplication though. :)\n\nMy personal opinion is that having unit tests that don\u0027t mock out database access here is OK. It still a unit rather than system test. It also matches what happens in the tests/unit/db/test_db_api.py tests, which these are based off.","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"73fe8900c9b1dd29fb108d5e2ff8f8a0d60a8bd6","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        cell_db_agg \u003d aggregate.Aggregate.get_by_id(self.context, ca2[\u0027id\u0027])"},{"line_number":240,"context_line":""},{"line_number":241,"context_line":"        self.assertEqual(api_db_agg.in_api, True)"},{"line_number":242,"context_line":"        self.assertEqual(cell_db_agg.in_api, False)"},{"line_number":243,"context_line":""},{"line_number":244,"context_line":"    @mock.patch(\u0027nova.objects.aggregate._aggregate_get_from_db\u0027)"},{"line_number":245,"context_line":"    @mock.patch(\u0027nova.db.aggregate_get\u0027)"}],"source_content_type":"text/x-python","patch_set":16,"id":"3aaa91ec_9f29758c","line":242,"updated":"2016-06-22 20:18:27.000000000","message":"As we\u0027ve gone over before, the above should be moved to functional tests, but I\u0027m ok having that done in a followup patch.","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"73fe8900c9b1dd29fb108d5e2ff8f8a0d60a8bd6","unresolved":false,"context_lines":[{"line_number":250,"context_line":"        self.compare_obj(agg, fake_aggregate, subs\u003dSUBS)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        mock_get_api.assert_called_once_with(self.context, 123)"},{"line_number":253,"context_line":"        self.assertFalse(mock_get.called)"},{"line_number":254,"context_line":""},{"line_number":255,"context_line":"    @mock.patch(\u0027nova.objects.aggregate._aggregate_get_from_db\u0027)"},{"line_number":256,"context_line":"    @mock.patch(\u0027nova.db.aggregate_get\u0027)"}],"source_content_type":"text/x-python","patch_set":16,"id":"3aaa91ec_ff20799d","line":253,"updated":"2016-06-22 20:18:27.000000000","message":"++ :)","commit_id":"edc4df4feb87dc9ecd10d80051fd831cbe3f85a9"}]}