)]}'
{"nova/api/openstack/placement/objects/resource_provider.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"761b929656e6935dddac10dd40ab094131ba0fb8","unresolved":false,"context_lines":[{"line_number":3689,"context_line":""},{"line_number":3690,"context_line":""},{"line_number":3691,"context_line":"@base.VersionedObjectRegistry.register_if(False)"},{"line_number":3692,"context_line":"class Consumer(base.VersionedObject):"},{"line_number":3693,"context_line":"    fields \u003d {"},{"line_number":3694,"context_line":"        \"id\": fields.IntegerField(read_only\u003dTrue),"},{"line_number":3695,"context_line":"        \"uuid\": fields.UUIDField(nullable\u003dFalse),"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_9d3b08d0","line":3692,"range":{"start_line":3692,"start_character":6,"end_line":3692,"end_character":14},"updated":"2018-04-17 19:20:48.000000000","message":"Do we need an update method? Or do we get that for free? Or is it coming in a subsequent patch?","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"31399bde04cd53ea4ebb1b70a35d3c38021b3cd7","unresolved":false,"context_lines":[{"line_number":3689,"context_line":""},{"line_number":3690,"context_line":""},{"line_number":3691,"context_line":"@base.VersionedObjectRegistry.register_if(False)"},{"line_number":3692,"context_line":"class Consumer(base.VersionedObject):"},{"line_number":3693,"context_line":"    fields \u003d {"},{"line_number":3694,"context_line":"        \"id\": fields.IntegerField(read_only\u003dTrue),"},{"line_number":3695,"context_line":"        \"uuid\": fields.UUIDField(nullable\u003dFalse),"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_b3663ded","line":3692,"range":{"start_line":3692,"start_character":6,"end_line":3692,"end_character":14},"in_reply_to":"9f6a8fd7_38e20ad7","updated":"2018-04-17 20:59:23.000000000","message":"Riiight, but there\u0027s nothing that uses any of the code in this class until later in the series.  The impression one gets from looking at the series is that this patch adds the Consumer class, and subsequent patches use it.  So IMO that should be all or nothing.","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"bc8143ab9f7435492d2417084dcba562169bfba6","unresolved":false,"context_lines":[{"line_number":3689,"context_line":""},{"line_number":3690,"context_line":""},{"line_number":3691,"context_line":"@base.VersionedObjectRegistry.register_if(False)"},{"line_number":3692,"context_line":"class Consumer(base.VersionedObject):"},{"line_number":3693,"context_line":"    fields \u003d {"},{"line_number":3694,"context_line":"        \"id\": fields.IntegerField(read_only\u003dTrue),"},{"line_number":3695,"context_line":"        \"uuid\": fields.UUIDField(nullable\u003dFalse),"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_38e20ad7","line":3692,"range":{"start_line":3692,"start_character":6,"end_line":3692,"end_character":14},"in_reply_to":"9f6a8fd7_9d3b08d0","updated":"2018-04-17 20:39:22.000000000","message":"There is nothing that modifies a consumer record until the last patch. I added a save() method there.","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"3cdc07e623506fe9150c982672aa732b997ea71a","unresolved":false,"context_lines":[{"line_number":3689,"context_line":""},{"line_number":3690,"context_line":""},{"line_number":3691,"context_line":"@base.VersionedObjectRegistry.register_if(False)"},{"line_number":3692,"context_line":"class Consumer(base.VersionedObject):"},{"line_number":3693,"context_line":"    fields \u003d {"},{"line_number":3694,"context_line":"        \"id\": fields.IntegerField(read_only\u003dTrue),"},{"line_number":3695,"context_line":"        \"uuid\": fields.UUIDField(nullable\u003dFalse),"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_0d85575a","line":3692,"range":{"start_line":3692,"start_character":6,"end_line":3692,"end_character":14},"in_reply_to":"9f6a8fd7_b3663ded","updated":"2018-04-19 14:05:42.000000000","message":"Didn\u0027t get it into PS3, but it will be in PS4.","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"761b929656e6935dddac10dd40ab094131ba0fb8","unresolved":false,"context_lines":[{"line_number":3750,"context_line":"        except exception.NotFound:"},{"line_number":3751,"context_line":"            # First, make sure that we have the internal project and user ids,"},{"line_number":3752,"context_line":"            # and not the external UUIDs. None is also acceptable."},{"line_number":3753,"context_line":"            if project_id is None or isinstance(project_id, int):"},{"line_number":3754,"context_line":"                pid \u003d project_id"},{"line_number":3755,"context_line":"            else:"},{"line_number":3756,"context_line":"                pid \u003d _ensure_project(ctx, project_id)"},{"line_number":3757,"context_line":"            if user_id is None or isinstance(user_id, int):"},{"line_number":3758,"context_line":"                uid \u003d user_id"},{"line_number":3759,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_426831f4","line":3756,"range":{"start_line":3753,"start_character":12,"end_line":3756,"end_character":54},"updated":"2018-04-17 19:20:48.000000000","message":"So the project_id param can be the integer internal ID or a string UUID?  Then we should definitely have docstring words for it.\n\nLikewise user_id","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"bc8143ab9f7435492d2417084dcba562169bfba6","unresolved":false,"context_lines":[{"line_number":3750,"context_line":"        except exception.NotFound:"},{"line_number":3751,"context_line":"            # First, make sure that we have the internal project and user ids,"},{"line_number":3752,"context_line":"            # and not the external UUIDs. None is also acceptable."},{"line_number":3753,"context_line":"            if project_id is None or isinstance(project_id, int):"},{"line_number":3754,"context_line":"                pid \u003d project_id"},{"line_number":3755,"context_line":"            else:"},{"line_number":3756,"context_line":"                pid \u003d _ensure_project(ctx, project_id)"},{"line_number":3757,"context_line":"            if user_id is None or isinstance(user_id, int):"},{"line_number":3758,"context_line":"                uid \u003d user_id"},{"line_number":3759,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_d8e66ee9","line":3756,"range":{"start_line":3753,"start_character":12,"end_line":3756,"end_character":54},"in_reply_to":"9f6a8fd7_426831f4","updated":"2018-04-17 20:39:22.000000000","message":"That\u0027s a holdover from when this could be called from ensure_consumer(), which converted the external UUID to internal ID, or the main code, where only external UUIDs were used. I\u0027ll remove the check for the integer form.","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"761b929656e6935dddac10dd40ab094131ba0fb8","unresolved":false,"context_lines":[{"line_number":3780,"context_line":""},{"line_number":3781,"context_line":"    @classmethod"},{"line_number":3782,"context_line":"    @db_api.api_context_manager.writer"},{"line_number":3783,"context_line":"    def ensure_consumer(cls, ctx, consumer_uuid, project_id, user_id,"},{"line_number":3784,"context_line":"            request_generation):"},{"line_number":3785,"context_line":"        \"\"\"Examines the project_id, user_id of the allocation along with the"},{"line_number":3786,"context_line":"        supplied consumer_uuid and ensures that if project_id and user_id"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_dd08a029","line":3783,"range":{"start_line":3783,"start_character":8,"end_line":3783,"end_character":23},"updated":"2018-04-17 19:20:48.000000000","message":"Sorry, I\u0027m not seeing what this method gets us that get_or_create doesn\u0027t.  I.e. why do we need both?","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"bc8143ab9f7435492d2417084dcba562169bfba6","unresolved":false,"context_lines":[{"line_number":3780,"context_line":""},{"line_number":3781,"context_line":"    @classmethod"},{"line_number":3782,"context_line":"    @db_api.api_context_manager.writer"},{"line_number":3783,"context_line":"    def ensure_consumer(cls, ctx, consumer_uuid, project_id, user_id,"},{"line_number":3784,"context_line":"            request_generation):"},{"line_number":3785,"context_line":"        \"\"\"Examines the project_id, user_id of the allocation along with the"},{"line_number":3786,"context_line":"        supplied consumer_uuid and ensures that if project_id and user_id"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_983eb627","line":3783,"range":{"start_line":3783,"start_character":8,"end_line":3783,"end_character":23},"in_reply_to":"9f6a8fd7_dd08a029","updated":"2018-04-17 20:39:22.000000000","message":"We don\u0027t. Originally the ensure* was a copy of the ensure_consumer_project_user() from the Allocation object, but as this developed, the same logic was needed in get_or_create(). I\u0027ve removed this method, as nothing called it.","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"761b929656e6935dddac10dd40ab094131ba0fb8","unresolved":false,"context_lines":[{"line_number":3787,"context_line":"        are set that there are records in the consumers, projects, and"},{"line_number":3788,"context_line":"        users table for these entities."},{"line_number":3789,"context_line":""},{"line_number":3790,"context_line":"        :param ctx: `nova.context.RequestContext` object that may or may not"},{"line_number":3791,"context_line":"                    have the oslo.db Session object in it"},{"line_number":3792,"context_line":"        :param consumer_uuid: The UUID of the consumer"},{"line_number":3793,"context_line":"        :param project_id: the corresponding project\u0027s external UUID."}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_421fd16b","line":3790,"range":{"start_line":3790,"start_character":66,"end_line":3790,"end_character":76},"updated":"2018-04-17 19:20:48.000000000","message":"Oh?  L3804 calls into get_or_create; if that hits L3764 we definitely expect there to be a session, nah?","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"bc8143ab9f7435492d2417084dcba562169bfba6","unresolved":false,"context_lines":[{"line_number":3787,"context_line":"        are set that there are records in the consumers, projects, and"},{"line_number":3788,"context_line":"        users table for these entities."},{"line_number":3789,"context_line":""},{"line_number":3790,"context_line":"        :param ctx: `nova.context.RequestContext` object that may or may not"},{"line_number":3791,"context_line":"                    have the oslo.db Session object in it"},{"line_number":3792,"context_line":"        :param consumer_uuid: The UUID of the consumer"},{"line_number":3793,"context_line":"        :param project_id: the corresponding project\u0027s external UUID."}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_f81252b1","line":3790,"range":{"start_line":3790,"start_character":66,"end_line":3790,"end_character":76},"in_reply_to":"9f6a8fd7_421fd16b","updated":"2018-04-17 20:39:22.000000000","message":"The problem was that some tests create allocations that don\u0027t have a session. But this code is going bye-bye, so...","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"761b929656e6935dddac10dd40ab094131ba0fb8","unresolved":false,"context_lines":[{"line_number":3804,"context_line":"        return cls.get_or_create(ctx, consumer_uuid, pid, uid,"},{"line_number":3805,"context_line":"                request_generation\u003drequest_generation)"},{"line_number":3806,"context_line":""},{"line_number":3807,"context_line":"    def increment(self):"},{"line_number":3808,"context_line":"        \"\"\"Increments the value of the generation field.\"\"\""},{"line_number":3809,"context_line":"        # TODO(edleafe): Add the logic for generation conflicts"},{"line_number":3810,"context_line":"        self.generation +\u003d 1"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_c2520168","line":3807,"range":{"start_line":3807,"start_character":8,"end_line":3807,"end_character":17},"updated":"2018-04-17 19:20:48.000000000","message":"This should probably be called increment_generation.  Otherwise \"increment Consumer\" is implied, which is... kinda?  But not really?","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"bc8143ab9f7435492d2417084dcba562169bfba6","unresolved":false,"context_lines":[{"line_number":3804,"context_line":"        return cls.get_or_create(ctx, consumer_uuid, pid, uid,"},{"line_number":3805,"context_line":"                request_generation\u003drequest_generation)"},{"line_number":3806,"context_line":""},{"line_number":3807,"context_line":"    def increment(self):"},{"line_number":3808,"context_line":"        \"\"\"Increments the value of the generation field.\"\"\""},{"line_number":3809,"context_line":"        # TODO(edleafe): Add the logic for generation conflicts"},{"line_number":3810,"context_line":"        self.generation +\u003d 1"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f6a8fd7_f887b2d6","line":3807,"range":{"start_line":3807,"start_character":8,"end_line":3807,"end_character":17},"in_reply_to":"9f6a8fd7_c2520168","updated":"2018-04-17 20:39:22.000000000","message":"Seems clear enough to me, but explicit is better than implicit, right? Changed.","commit_id":"2b6f27ca4feeea20b4a45e02571cb0ef8650448c"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a19664d9049583994f9675d76c5d21f977a7ff9d","unresolved":false,"context_lines":[{"line_number":2017,"context_line":"    return pid, uid"},{"line_number":2018,"context_line":""},{"line_number":2019,"context_line":""},{"line_number":2020,"context_line":"@db_api.api_context_manager.writer"},{"line_number":2021,"context_line":"def _get_allocations_by_provider_id(ctx, rp_id):"},{"line_number":2022,"context_line":"    allocs \u003d sa.alias(_ALLOC_TBL, name\u003d\"a\")"},{"line_number":2023,"context_line":"    cols \u003d ["}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_d748bc2f","line":2020,"updated":"2018-04-26 10:27:41.000000000","message":"this is changing the context manager from reader to writer on this method and it only does a select, and is also unrelated this change, so I assume this is not desired?","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"62c5432538ccf0a289a7168c630d7b136fef1380","unresolved":false,"context_lines":[{"line_number":2017,"context_line":"    return pid, uid"},{"line_number":2018,"context_line":""},{"line_number":2019,"context_line":""},{"line_number":2020,"context_line":"@db_api.api_context_manager.writer"},{"line_number":2021,"context_line":"def _get_allocations_by_provider_id(ctx, rp_id):"},{"line_number":2022,"context_line":"    allocs \u003d sa.alias(_ALLOC_TBL, name\u003d\"a\")"},{"line_number":2023,"context_line":"    cols \u003d ["}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_3d12370b","line":2020,"in_reply_to":"9f6a8fd7_d748bc2f","updated":"2018-04-26 14:05:42.000000000","message":"Yeah, it\u0027s the result of merging different versions of this code. Fixed.","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a19664d9049583994f9675d76c5d21f977a7ff9d","unresolved":false,"context_lines":[{"line_number":3751,"context_line":"        :returns Consumer object"},{"line_number":3752,"context_line":"        :raises NotFound if no such consumer could be found"},{"line_number":3753,"context_line":"        \"\"\""},{"line_number":3754,"context_line":"        consumer \u003d sa.alias(_CONSUMER_TBL, name\u003d\"consumer\")"},{"line_number":3755,"context_line":"        project \u003d sa.alias(_PROJECT_TBL, name\u003d\"p\")"},{"line_number":3756,"context_line":"        user \u003d sa.alias(_USER_TBL, name\u003d\"u\")"},{"line_number":3757,"context_line":"        project_join \u003d sa.outerjoin(consumer, project,"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_7731b0a2","line":3754,"updated":"2018-04-26 10:27:41.000000000","message":"If the \u0027names\u0027 here are not going to be used anywhere, you can leave them off. sa will make reasonable (and deterministic) names for you.","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"62c5432538ccf0a289a7168c630d7b136fef1380","unresolved":false,"context_lines":[{"line_number":3751,"context_line":"        :returns Consumer object"},{"line_number":3752,"context_line":"        :raises NotFound if no such consumer could be found"},{"line_number":3753,"context_line":"        \"\"\""},{"line_number":3754,"context_line":"        consumer \u003d sa.alias(_CONSUMER_TBL, name\u003d\"consumer\")"},{"line_number":3755,"context_line":"        project \u003d sa.alias(_PROJECT_TBL, name\u003d\"p\")"},{"line_number":3756,"context_line":"        user \u003d sa.alias(_USER_TBL, name\u003d\"u\")"},{"line_number":3757,"context_line":"        project_join \u003d sa.outerjoin(consumer, project,"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_bdeba7da","line":3754,"in_reply_to":"9f6a8fd7_7731b0a2","updated":"2018-04-26 14:05:42.000000000","message":"Yeah, but this is mostly copy/pasted code that I then changed for this method. I\u0027ll remove them.","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a19664d9049583994f9675d76c5d21f977a7ff9d","unresolved":false,"context_lines":[{"line_number":3808,"context_line":"            # The value of -1 for generation is used to show that there was no"},{"line_number":3809,"context_line":"            # record in the DB."},{"line_number":3810,"context_line":"            cls._insert(ctx, uuid, pid, uid, -1)"},{"line_number":3811,"context_line":"            obj \u003d cls.get_by_uuid(ctx, uuid)"},{"line_number":3812,"context_line":"        # The \u0027generation\u0027 field will contain the value in the DB; we want"},{"line_number":3813,"context_line":"        # the value of \u0027request_generation\u0027 to hold the value from the"},{"line_number":3814,"context_line":"        # request."}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_77bf70d0","line":3811,"updated":"2018-04-26 10:27:41.000000000","message":"we\u0027ve just done the insert, could we not avoid another read by using the data we already have?\n\n(not that I really care about the additional read all that much, it just seems a bit redundant)","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"62c5432538ccf0a289a7168c630d7b136fef1380","unresolved":false,"context_lines":[{"line_number":3808,"context_line":"            # The value of -1 for generation is used to show that there was no"},{"line_number":3809,"context_line":"            # record in the DB."},{"line_number":3810,"context_line":"            cls._insert(ctx, uuid, pid, uid, -1)"},{"line_number":3811,"context_line":"            obj \u003d cls.get_by_uuid(ctx, uuid)"},{"line_number":3812,"context_line":"        # The \u0027generation\u0027 field will contain the value in the DB; we want"},{"line_number":3813,"context_line":"        # the value of \u0027request_generation\u0027 to hold the value from the"},{"line_number":3814,"context_line":"        # request."}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_fdf4bf89","line":3811,"in_reply_to":"9f6a8fd7_77bf70d0","updated":"2018-04-26 14:05:42.000000000","message":"I could create the object directly here; I would just have to assume that the \u0027id\u0027 field is zero. I would think that that would be frowned upon, but if that\u0027s better than reading it from the DB, I can change it.","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"72d19f2ab8de6cf9753ca7235b6e078d49031b60","unresolved":false,"context_lines":[{"line_number":3816,"context_line":"        obj.enforce_generation \u003d enforce_generation"},{"line_number":3817,"context_line":"        return obj"},{"line_number":3818,"context_line":""},{"line_number":3819,"context_line":"    def increment_generation(self):"},{"line_number":3820,"context_line":"        \"\"\"Increments the value of the generation field after validating that"},{"line_number":3821,"context_line":"        the requested generation matches the database value.i"},{"line_number":3822,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_1af17902","line":3819,"updated":"2018-04-26 11:02:00.000000000","message":"Incrementing the generation needs to be a single sql call that does that \u0027update where\u0027 (see the way resource provider generation is updated). Otherwise it is not properly a compare and swap: http://www.joinfu.com/2015/01/understanding-reservations-concurrency-locking-in-nova/","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"62c5432538ccf0a289a7168c630d7b136fef1380","unresolved":false,"context_lines":[{"line_number":3816,"context_line":"        obj.enforce_generation \u003d enforce_generation"},{"line_number":3817,"context_line":"        return obj"},{"line_number":3818,"context_line":""},{"line_number":3819,"context_line":"    def increment_generation(self):"},{"line_number":3820,"context_line":"        \"\"\"Increments the value of the generation field after validating that"},{"line_number":3821,"context_line":"        the requested generation matches the database value.i"},{"line_number":3822,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_9d9023f0","line":3819,"in_reply_to":"9f6a8fd7_1af17902","updated":"2018-04-26 14:05:42.000000000","message":"I had done it this way originally, but the problem of handling requested generations of None as well as conditional enforcement made the code very branchy. I can re-work it back like that and see if it looks better to you.","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a19664d9049583994f9675d76c5d21f977a7ff9d","unresolved":false,"context_lines":[{"line_number":3833,"context_line":"                 (self.generation \u003d\u003d self.request_generation))"},{"line_number":3834,"context_line":"        if not valid:"},{"line_number":3835,"context_line":"            raise exception.ConcurrentUpdateDetected()"},{"line_number":3836,"context_line":"            return False"},{"line_number":3837,"context_line":"        return True"},{"line_number":3838,"context_line":""},{"line_number":3839,"context_line":"    @db_api.api_context_manager.writer"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_d792fc58","line":3836,"updated":"2018-04-26 10:27:41.000000000","message":"you\u0027ve returned after a raise?","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"62c5432538ccf0a289a7168c630d7b136fef1380","unresolved":false,"context_lines":[{"line_number":3833,"context_line":"                 (self.generation \u003d\u003d self.request_generation))"},{"line_number":3834,"context_line":"        if not valid:"},{"line_number":3835,"context_line":"            raise exception.ConcurrentUpdateDetected()"},{"line_number":3836,"context_line":"            return False"},{"line_number":3837,"context_line":"        return True"},{"line_number":3838,"context_line":""},{"line_number":3839,"context_line":"    @db_api.api_context_manager.writer"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_7dcaef1a","line":3836,"in_reply_to":"9f6a8fd7_d792fc58","updated":"2018-04-26 14:05:42.000000000","message":"Doh!","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a19664d9049583994f9675d76c5d21f977a7ff9d","unresolved":false,"context_lines":[{"line_number":3837,"context_line":"        return True"},{"line_number":3838,"context_line":""},{"line_number":3839,"context_line":"    @db_api.api_context_manager.writer"},{"line_number":3840,"context_line":"    def save(self, ctx):"},{"line_number":3841,"context_line":"        \"\"\"Since Consumer objects should be created by either \u0027get_by_uuid()\u0027"},{"line_number":3842,"context_line":"        or \u0027get_or_create()\u0027, which will ensure that a record exists in the"},{"line_number":3843,"context_line":"        database, we only need to update that record here."}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_d7a0bc94","line":3840,"updated":"2018-04-26 10:27:41.000000000","message":"When is this ever likely to happen? One a consumer is created and written the first time isn\u0027t the only time we will change the generation be when we do the increment when writing allocations, which should happen outside the context of this object handling with an explicit sql call, as with the the rp generations are incremented.","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"62c5432538ccf0a289a7168c630d7b136fef1380","unresolved":false,"context_lines":[{"line_number":3837,"context_line":"        return True"},{"line_number":3838,"context_line":""},{"line_number":3839,"context_line":"    @db_api.api_context_manager.writer"},{"line_number":3840,"context_line":"    def save(self, ctx):"},{"line_number":3841,"context_line":"        \"\"\"Since Consumer objects should be created by either \u0027get_by_uuid()\u0027"},{"line_number":3842,"context_line":"        or \u0027get_or_create()\u0027, which will ensure that a record exists in the"},{"line_number":3843,"context_line":"        database, we only need to update that record here."}],"source_content_type":"text/x-python","patch_set":5,"id":"9f6a8fd7_3d70f7dc","line":3840,"in_reply_to":"9f6a8fd7_d7a0bc94","updated":"2018-04-26 14:05:42.000000000","message":"See L3819.","commit_id":"1088b51f485d2e62b875159cb30d4e7fac7dc910"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"568697bec20485db658766131dccc1e6615c9b98","unresolved":false,"context_lines":[{"line_number":2002,"context_line":"    return _ensure_lookup_table_entry(ctx, _USER_TBL, external_id)"},{"line_number":2003,"context_line":""},{"line_number":2004,"context_line":""},{"line_number":2005,"context_line":"def _to_internal(ctx, project_id, user_id):"},{"line_number":2006,"context_line":"    \"\"\"Make sure that we have the internal project and user ids and not the"},{"line_number":2007,"context_line":"    external UUIDs. None is also acceptable."},{"line_number":2008,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":6,"id":"9f6a8fd7_341d690f","line":2005,"range":{"start_line":2005,"start_character":5,"end_line":2005,"end_character":16},"updated":"2018-04-26 19:26:09.000000000","message":"this function name is hiding the fact that data could be written during the function. That\u0027s what the _ensure_XXX() functions are all about -- indicating that the function ensures there is an internal record for a particular external identifier.\n\nI would prefer you either get rid of this function (just unpack it into the single place it is used) or alternately do the ensure of the project and user information from the caller (i.e. the handler).\n\nIt always bothered me that the code to check if a project/consumer existed (and create one if not) was being doing inside the AllocationList.create_all() call and not done beforehand by the handler.","commit_id":"67933443fe6b8e740670de9ddc6b2d4bd273abf8"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"568697bec20485db658766131dccc1e6615c9b98","unresolved":false,"context_lines":[{"line_number":3720,"context_line":"        \"generation\": fields.IntegerField(nullable\u003dFalse),"},{"line_number":3721,"context_line":"        \"project_id\": fields.StringField(nullable\u003dTrue),"},{"line_number":3722,"context_line":"        \"user_id\": fields.StringField(nullable\u003dTrue),"},{"line_number":3723,"context_line":"        # When a request supplies their version of what they believe the"},{"line_number":3724,"context_line":"        # current consumer_generation value to be, that will be stored in this"},{"line_number":3725,"context_line":"        # field for checking race conditions. For now, it is simply None."},{"line_number":3726,"context_line":"        \"request_generation\": fields.IntegerField(nullable\u003dTrue),"},{"line_number":3727,"context_line":"        # By default, this is False to maintain the behavior for microversions"},{"line_number":3728,"context_line":"        # before 1.99. Newer microversions will set this to True, which will"},{"line_number":3729,"context_line":"        # check that the request supplied a consumer_generation that matches"},{"line_number":3730,"context_line":"        # the value in the database. A failed match indicates a race condition,"},{"line_number":3731,"context_line":"        # and a ConcurrentUpdateDetected will be raised."},{"line_number":3732,"context_line":"        \"enforce_generation\": fields.BooleanField(default\u003dFalse),"},{"line_number":3733,"context_line":"    }"},{"line_number":3734,"context_line":""},{"line_number":3735,"context_line":"    @staticmethod"},{"line_number":3736,"context_line":"    def _from_db_object(context, target, source):"}],"source_content_type":"text/x-python","patch_set":6,"id":"9f6a8fd7_14db857d","line":3733,"range":{"start_line":3723,"start_character":0,"end_line":3733,"end_character":5},"updated":"2018-04-26 19:26:09.000000000","message":"I was expecting all the logic of whether to increment the generation (and enforce concurrent writes) would go entirely in the handler, similar to how is done for the resource provider generation when setting inventories:\n\nhttps://github.com/openstack/nova/blob/master/nova/api/openstack/placement/handlers/inventory.py#L292-L298\n\nI don\u0027t believe we need these fields to be on the Consumer object.","commit_id":"67933443fe6b8e740670de9ddc6b2d4bd273abf8"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"568697bec20485db658766131dccc1e6615c9b98","unresolved":false,"context_lines":[{"line_number":3827,"context_line":"                # This is a new record that was created at the beginning of"},{"line_number":3828,"context_line":"                # this transaction."},{"line_number":3829,"context_line":"                if self.request_generation is None:"},{"line_number":3830,"context_line":"                    stmt \u003d _CONSUMER_TBL.update().where(sa.and_("},{"line_number":3831,"context_line":"                           _CONSUMER_TBL.c.id \u003d\u003d self.id,"},{"line_number":3832,"context_line":"                           _CONSUMER_TBL.c.generation \u003d\u003d -1)).values("},{"line_number":3833,"context_line":"                                generation\u003d(new_gen))"},{"line_number":3834,"context_line":"                    res \u003d ctx.session.execute(stmt)"},{"line_number":3835,"context_line":"                    if res.rowcount !\u003d 1:"},{"line_number":3836,"context_line":"                        # Another process has created the record while this one"},{"line_number":3837,"context_line":"                        # was attempting to."},{"line_number":3838,"context_line":"                        raise exception.ConcurrentUpdateDetected"},{"line_number":3839,"context_line":"                else:"},{"line_number":3840,"context_line":"                    # The caller thought the consumer existed, but some other"},{"line_number":3841,"context_line":"                    # process must have deleted it first."}],"source_content_type":"text/x-python","patch_set":6,"id":"9f6a8fd7_3446c923","line":3838,"range":{"start_line":3830,"start_character":0,"end_line":3838,"end_character":64},"updated":"2018-04-26 19:26:09.000000000","message":"This is really the only thing I think this method should be doing. All the logic and conditionals belong in the handler. The object code should be as close to Plain Old Data objects as possible.","commit_id":"67933443fe6b8e740670de9ddc6b2d4bd273abf8"}]}