)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"9ec9146c23565d1be4a64fa75fff1f3d3c4cf5ab","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"5e2bf420_ab88dd07","updated":"2022-07-06 03:16:46.000000000","message":"Oh see what your doing here. Moving it out of get alternate states (so that can continue to just be db_state driven) but add an exception back in _is_deleted so it\u0027s _ONLY_ related to the deleted check (and takes into account any objects in shards)\n\nThat is brillant Clay.. here I am going down what\u0027s best for stats when 2 state machines collide, and here you are just worrying about the case at hand in a place that wont effect stats. Wow! Here I am obviously caught up over thinking it. ","commit_id":"ba305e0f10677af9a1956d5e424561bdaf2897e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"bae38122112968f091dc0e6aa3668c08e3814d01","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"4cca5825_83d640fc","updated":"2022-07-05 22:16:53.000000000","message":"if we think this is worth working on more; we should probably pull all the memory-db connection management stuff into a pre-factor, and then add a unit test specifically for the new logic in is_deleted","commit_id":"ba305e0f10677af9a1956d5e424561bdaf2897e5"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1bfc9f8c789b0db59d3bcb3f1a67e3ec6d439777","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"5188c3a5_2026cd64","updated":"2022-07-07 14:08:41.000000000","message":"I tried a different approach here https://review.opendev.org/c/openstack/swift/+/848962","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"728ef85cd8fe1ec29875ae1c45646a437b2c46b4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"bff09b66_ed5f0891","updated":"2022-07-08 06:30:03.000000000","message":"I\u0027m having a play rebasing this off my remove memory broker patch and reworking my suggested way. Hopefully get something up today or my Monday. ","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d8b08cadfb1c60c09132a11528b9ae791a8f4003","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"116d6d75_b73f8075","updated":"2022-07-06 22:51:38.000000000","message":"Matt, I don\u0027t think you\u0027re over-thinking.  I think there\u0027s two ways of thinking about the problem.\n\n1) what\u0027s the best thing we can do about the stuck databases in prod\n2) what\u0027s the best thing we can do to reduce complexity in the sharding/replicator state machine\n\nI think a targeted approach might be better for #1 - but the issue with connection re-use for in-memory databases is starting to get messy https://review.opendev.org/c/openstack/swift/+/848909/","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e0069a4aecc2fc39d67236a4d529d9c781fde5e4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"3446f59b_3220f22b","updated":"2022-07-08 16:16:48.000000000","message":"rebased","commit_id":"616323f22dd40c5225327ad687f4ea83c61a931b"}],"swift/container/backend.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1bfc9f8c789b0db59d3bcb3f1a67e3ec6d439777","unresolved":true,"context_lines":[{"line_number":807,"context_line":"        \"\"\""},{"line_number":808,"context_line":"        Check if the DB is considered to be deleted."},{"line_number":809,"context_line":""},{"line_number":810,"context_line":"        This object count used in this check is the same as the container"},{"line_number":811,"context_line":"        object count that would be returned in the result of :meth:`get_info`"},{"line_number":812,"context_line":"        and exposed to a client i.e. it is based on the container_stat view for"},{"line_number":813,"context_line":"        the current storage policy index or relevant shard range usage."},{"line_number":814,"context_line":""},{"line_number":815,"context_line":"        :param conn: database conn"}],"source_content_type":"text/x-python","patch_set":3,"id":"b9c37d68_d70e7981","line":812,"range":{"start_line":810,"start_character":8,"end_line":812,"end_character":32},"updated":"2022-07-07 14:08:41.000000000","message":"I think this statement cease to be true with the proposed change i.e. it is now possible for HEAD to return 404 while both _is_deleted and is_deleted return False\n\n   HEAD -\u003e get_info_is_deleted() -\u003e get_info()\n \n vs\n \n   is_deleted() -\u003e _is_deleted()","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"728ef85cd8fe1ec29875ae1c45646a437b2c46b4","unresolved":true,"context_lines":[{"line_number":810,"context_line":"        This object count used in this check is the same as the container"},{"line_number":811,"context_line":"        object count that would be returned in the result of :meth:`get_info`"},{"line_number":812,"context_line":"        and exposed to a client i.e. it is based on the container_stat view for"},{"line_number":813,"context_line":"        the current storage policy index or relevant shard range usage."},{"line_number":814,"context_line":""},{"line_number":815,"context_line":"        :param conn: database conn"},{"line_number":816,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"1ee63415_40c9ec7d","line":813,"range":{"start_line":813,"start_character":12,"end_line":813,"end_character":40},"updated":"2022-07-08 06:30:03.000000000","message":"Hmm, this part has me a little worried. If we go my commented approach, ie: It isn\u0027t deleted if there are objects, be them in the object table or shardranges. \n\nBasically because, even if we are sharded and we have objects in the object table, they\u0027re misplaced, but still objects. Then what about any objects that are in the wrong policy?\n\nSurely, on the same vein we actually want to let the reconciler fix these objects first, if they\u0027re not deleted?\n\nMaybe we need a:\n\n  SELECT sum(object_count) as object_count FROM policy_stat\n\ninstead of the SQL below.","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"30aeadb61b45b2af65e8116693f70d990d920b6f","unresolved":true,"context_lines":[{"line_number":820,"context_line":"            SELECT put_timestamp, delete_timestamp, object_count"},{"line_number":821,"context_line":"            FROM container_stat\u0027\u0027\u0027).fetchone()"},{"line_number":822,"context_line":"        info \u003d dict(info)"},{"line_number":823,"context_line":"        info.update(self._get_alternate_object_stats()[1])"},{"line_number":824,"context_line":"        # special case for unsharded with sharding initiated"},{"line_number":825,"context_line":"        osr \u003d self._own_shard_range(no_default\u003dTrue)"},{"line_number":826,"context_line":"        if self.db_epoch is None and osr:"}],"source_content_type":"text/x-python","patch_set":3,"id":"748631ca_0f60b6e6","line":823,"range":{"start_line":823,"start_character":8,"end_line":823,"end_character":58},"updated":"2022-07-06 23:27:13.000000000","message":"I like this approach as it\u0027s more targeted. But got me thinking about it last night. _get_alternate_object_stats here, when the db_state is SHARDED will return only the shardrange based stats. Which means there is a chance here in this code we never actually check the object table of the DB. \nSure if it\u0027s sharded then we should look at the shards for object count, however, the object table in that case is used as a misplaced object location.. and so should also be considered objects (although misplaced) in this container.\n\nSo I think we can simplify this even more by simply removing this line. That way, info will contain the object_count from the internal object table. We then just need to include any shard object counts.\n\nIn fact, as this is just gathering info for _is_deleted_info, that is does it have any objects, can we get away with something even simpler and just sum them up?\n\n  info \u003d conn.execute(\u0027\u0027\u0027\n      SELECT put_timestamp, delete_timestamp, object_count\n      FROM container_stat\u0027\u0027\u0027).fetchone()\n  info \u003d dict(info)\n  for key, value in self.get_shard_usage().items():\n      if value:\n          info[key] +\u003d value\n  return self._is_deleted_info(**info)\n  \nBecause even if there are no shards, we\u0027ll still get `sum([])` or 0\u0027s back. And whatever state we\u0027re in, if there are shards with objects, we probably don\u0027t want to be deleted so the sharder can do it\u0027s work to remidy the situation.","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1bfc9f8c789b0db59d3bcb3f1a67e3ec6d439777","unresolved":true,"context_lines":[{"line_number":821,"context_line":"            FROM container_stat\u0027\u0027\u0027).fetchone()"},{"line_number":822,"context_line":"        info \u003d dict(info)"},{"line_number":823,"context_line":"        info.update(self._get_alternate_object_stats()[1])"},{"line_number":824,"context_line":"        # special case for unsharded with sharding initiated"},{"line_number":825,"context_line":"        osr \u003d self._own_shard_range(no_default\u003dTrue)"},{"line_number":826,"context_line":"        if self.db_epoch is None and osr:"},{"line_number":827,"context_line":"            for key, value in self.get_shard_usage().items():"}],"source_content_type":"text/x-python","patch_set":3,"id":"d3d32d07_5e0cd0fa","line":824,"range":{"start_line":824,"start_character":10,"end_line":824,"end_character":60},"updated":"2022-07-07 14:08:41.000000000","message":"is the following actually restricted to that special case? curious that the conditions does not include get_db_state() \u003d\u003d UNSHARDED","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d629bea008c1b1560d75a8265c4283725de43ab3","unresolved":true,"context_lines":[{"line_number":821,"context_line":"            FROM container_stat\u0027\u0027\u0027).fetchone()"},{"line_number":822,"context_line":"        info \u003d dict(info)"},{"line_number":823,"context_line":"        info.update(self._get_alternate_object_stats()[1])"},{"line_number":824,"context_line":"        # special case for unsharded with sharding initiated"},{"line_number":825,"context_line":"        osr \u003d self._own_shard_range(no_default\u003dTrue)"},{"line_number":826,"context_line":"        if self.db_epoch is None and osr:"},{"line_number":827,"context_line":"            for key, value in self.get_shard_usage().items():"}],"source_content_type":"text/x-python","patch_set":3,"id":"55d756e1_74947587","line":824,"range":{"start_line":824,"start_character":10,"end_line":824,"end_character":60},"in_reply_to":"d3d32d07_5e0cd0fa","updated":"2022-07-08 14:14:00.000000000","message":"On reflection, I think we actually need to also consider get_shard_usage() while the db state is SHARDING, because once we fix the immediate bug stopping empty db\u0027s from sharding, those DBs will go to SHARDING but still be empty (i.e. no object rows in the DB) - but we do not want the DB to appear deleted during that time either!","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d629bea008c1b1560d75a8265c4283725de43ab3","unresolved":true,"context_lines":[{"line_number":826,"context_line":"        if self.db_epoch is None and osr:"},{"line_number":827,"context_line":"            for key, value in self.get_shard_usage().items():"},{"line_number":828,"context_line":"                if info.get(key, 0) \u003c value:"},{"line_number":829,"context_line":"                    info[key] \u003d value"},{"line_number":830,"context_line":"        return self._is_deleted_info(**info)"},{"line_number":831,"context_line":""},{"line_number":832,"context_line":"    def is_old_enough_to_reclaim(self, now, reclaim_age):"}],"source_content_type":"text/x-python","patch_set":3,"id":"ffa4cc48_cfaa3f62","line":829,"updated":"2022-07-08 14:14:00.000000000","message":"I think that this test test.unit.container.test_backend.TestContainerBroker.test_is_deleted ought to need updating given this change. I suspect it still passes because the test broker has not been given an own_shard_range, but that isn\u0027t realistic: if a broker has shard ranges that have useful object_count then it would also have an own_sr.\n\nOr, is there ever a case where we think get_shard_usage would return non-zero object count but that should be ignored? (NB get_shard_usage only sums shards with state in (ShardRange.ACTIVE, ShardRange.SHARDING, ShardRange.SHRINKING)","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"}],"test/probe/test_sharder.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1bfc9f8c789b0db59d3bcb3f1a67e3ec6d439777","unresolved":true,"context_lines":[{"line_number":431,"context_line":"class BaseAutoContainerSharding(BaseTestContainerSharding):"},{"line_number":432,"context_line":""},{"line_number":433,"context_line":"    def _maybe_skip_test(self):"},{"line_number":434,"context_line":"        super(BaseTestContainerSharding, self)._maybe_skip_test()"},{"line_number":435,"context_line":"        auto_shard \u003d all(config_true_value(c.get(\u0027auto_shard\u0027, False))"},{"line_number":436,"context_line":"                         for c in self.cont_configs)"},{"line_number":437,"context_line":"        if not auto_shard:"}],"source_content_type":"text/x-python","patch_set":3,"id":"d5a34c3b_8ce86feb","line":434,"range":{"start_line":434,"start_character":14,"end_line":434,"end_character":39},"updated":"2022-07-07 14:08:41.000000000","message":"should be BaseAutoTestContainerSharding","commit_id":"05b7f440a31c748a723e439303f973e0a11ea4b4"}]}