)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f8583278244d6d7c038a9c26782419d57cc6cb14","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"32bde131_7e4a78a0","updated":"2022-09-22 19:31:02.000000000","message":"I forgot to translate the (success, warnings) from the _do_audit method to conform with the signature expectation for the _audit method to return success (instead of None!)","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cd050baf62bcdb34f8b5b20c6be5cc869bf2b36d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"4741c65e_bb84a7a0","updated":"2022-09-22 17:49:19.000000000","message":"recheck","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b88231b5905bf7c43e69f9dc230dcb0768e8e271","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"12aee58f_18e138ad","updated":"2022-09-22 19:27:51.000000000","message":"this had intended to be a pure refactor - no functional difference - but it seems to have broken a probe test where the parent does not (!?)\n\npytest swift/test/probe/test_sharder.py::TestContainerSharding::test_sharded_can_get_objects_different_policy\n\n... is failing to delete the container after draining it.  When i start up swift the object list is empty, but object count is still 50.","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"dbf0858725febd9744b3a4599f21d483f8b6e0c2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"d32fad6f_08c71354","updated":"2022-09-22 22:47:51.000000000","message":"I am in favor of breaking up large functions, this one as well!","commit_id":"3cd6c6a27d0303935b936ed5d88f7b2205666137"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"84e6dcd7f654e48a6fd2b2306040fdd0a592864a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"e98cad59_d59c8a28","updated":"2022-09-23 10:09:03.000000000","message":"Love it...with the one caveat of the method ordering in the file.\n\nnit: I am a little concerned that the original *behavioural* change (in my parent patch) is probably hard to see amidst all the refactoring we have done. How would people feel about doing all the refactoring in a separate patch and then applying the targeted behavioural change.\n\nI guess that future-me might be glad of a \"no behavioural change was intended by *this* patch\" commit message vs a \"behaviour deliberately changed\" commit message.","commit_id":"3cd6c6a27d0303935b936ed5d88f7b2205666137"}],"swift/container/sharder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cd050baf62bcdb34f8b5b20c6be5cc869bf2b36d","unresolved":true,"context_lines":[{"line_number":1249,"context_line":"                \u0027Audit warnings for shard %s (%s): %s\u0027,"},{"line_number":1250,"context_line":"                broker.db_file, quote(broker.path), \u0027, \u0027.join(warnings))"},{"line_number":1251,"context_line":"        self._increment_stat("},{"line_number":1252,"context_line":"            \u0027audit_shard\u0027, \u0027success\u0027 if success else \u0027failure\u0027, statsd\u003dTrue)"},{"line_number":1253,"context_line":""},{"line_number":1254,"context_line":"    def _reclaim_shard_ranges(self, broker, own_shard_range):"},{"line_number":1255,"context_line":"        delete_age \u003d time.time() - self.reclaim_age"}],"source_content_type":"text/x-python","patch_set":1,"id":"4cc7ac8d_92d36d4c","line":1252,"updated":"2022-09-22 17:49:19.000000000","message":"I think multiple value return (success, warnings) is a good pattern for a task that can pass/fail but also wants to describe the specifics in more detail.\n\nThis adapter pattern does a good job showing we:\n\nincrement attempt\n*make the attempt*\nlog warnings\nincreent success/failure\n\n... and that book keeping looks good pulled out-side of and around the method that has to *do* the audit","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cd050baf62bcdb34f8b5b20c6be5cc869bf2b36d","unresolved":true,"context_lines":[{"line_number":1274,"context_line":"            self.logger.warning(\u0027Audit failed for shard %s (%s) - skipping: \u0027"},{"line_number":1275,"context_line":"                                \u0027missing own shard range\u0027,"},{"line_number":1276,"context_line":"                                broker.db_file, quote(broker.path))"},{"line_number":1277,"context_line":"            return False, warnings"},{"line_number":1278,"context_line":""},{"line_number":1279,"context_line":"        # Get the root view of the world, at least that part of the world"},{"line_number":1280,"context_line":"        # that overlaps with this shard\u0027s namespace. The"}],"source_content_type":"text/x-python","patch_set":1,"id":"e9091b2e_f43dbefa","line":1277,"updated":"2022-09-22 17:49:19.000000000","message":"I think this *behavior* is *so* interesting the \"return early\" pattern actually makes more sense.  We just *don\u0027t* do an audit if we don\u0027t have an OSR - and that\u0027s really the ONLY time we can \"fail\" to audit.","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"84e6dcd7f654e48a6fd2b2306040fdd0a592864a","unresolved":true,"context_lines":[{"line_number":1274,"context_line":"            self.logger.warning(\u0027Audit failed for shard %s (%s) - skipping: \u0027"},{"line_number":1275,"context_line":"                                \u0027missing own shard range\u0027,"},{"line_number":1276,"context_line":"                                broker.db_file, quote(broker.path))"},{"line_number":1277,"context_line":"            return False, warnings"},{"line_number":1278,"context_line":""},{"line_number":1279,"context_line":"        # Get the root view of the world, at least that part of the world"},{"line_number":1280,"context_line":"        # that overlaps with this shard\u0027s namespace. The"}],"source_content_type":"text/x-python","patch_set":1,"id":"eccf11f3_c1f3a35f","line":1277,"in_reply_to":"e9091b2e_f43dbefa","updated":"2022-09-23 10:09:03.000000000","message":"ok so returning warnings is the game-changer: when the \"error\" condition occurs we can return early from this method, which allows a cleaner exit from the method later.\n\n+1 !","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cd050baf62bcdb34f8b5b20c6be5cc869bf2b36d","unresolved":true,"context_lines":[{"line_number":1284,"context_line":"        # container; when not shrinking to root, but to another acceptor,"},{"line_number":1285,"context_line":"        # the root range should be in sharded state and will not interfere"},{"line_number":1286,"context_line":"        # with cleaving, listing or updating behaviour."},{"line_number":1287,"context_line":"        shard_ranges \u003d self._fetch_shard_ranges("},{"line_number":1288,"context_line":"            broker, newest\u003dTrue,"},{"line_number":1289,"context_line":"            params\u003d{\u0027marker\u0027: str_to_wsgi(own_shard_range.lower_str),"},{"line_number":1290,"context_line":"                    \u0027end_marker\u0027: str_to_wsgi(own_shard_range.upper_str),"}],"source_content_type":"text/x-python","patch_set":1,"id":"98ac4290_a73b89bd","line":1287,"updated":"2022-09-22 17:49:19.000000000","message":"fetch","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cd050baf62bcdb34f8b5b20c6be5cc869bf2b36d","unresolved":true,"context_lines":[{"line_number":1293,"context_line":"        if shard_ranges:"},{"line_number":1294,"context_line":"            own_shard_range, own_shard_range_from_root \u003d \\"},{"line_number":1295,"context_line":"                self._merge_shard_ranges_from_root("},{"line_number":1296,"context_line":"                    broker, shard_ranges, own_shard_range)"},{"line_number":1297,"context_line":"            if not own_shard_range_from_root:"},{"line_number":1298,"context_line":"                # this is not necessarily an error - some replicas of the"},{"line_number":1299,"context_line":"                # root may not yet know about this shard container, or the"}],"source_content_type":"text/x-python","patch_set":1,"id":"220748e3_51ce1221","line":1296,"updated":"2022-09-22 17:49:19.000000000","message":"merge \u0027em if we got \u0027em","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"cd050baf62bcdb34f8b5b20c6be5cc869bf2b36d","unresolved":true,"context_lines":[{"line_number":1304,"context_line":"            warnings.append(\u0027unable to get shard ranges from root\u0027)"},{"line_number":1305,"context_line":"        # else, our shard range is deleted, so root may have reclaimed it"},{"line_number":1306,"context_line":""},{"line_number":1307,"context_line":"        self._reclaim_shard_ranges(broker, own_shard_range)"},{"line_number":1308,"context_line":""},{"line_number":1309,"context_line":"        return True, warnings"},{"line_number":1310,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"f2a3c3c1_7de269b3","line":1307,"updated":"2022-09-22 17:49:19.000000000","message":"always reclaim","commit_id":"7d6b2f2ed08a611cca760b744f29348d7a29aa48"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"84e6dcd7f654e48a6fd2b2306040fdd0a592864a","unresolved":true,"context_lines":[{"line_number":1243,"context_line":""},{"line_number":1244,"context_line":"    def _audit_shard_container(self, broker):"},{"line_number":1245,"context_line":"        self._increment_stat(\u0027audit_shard\u0027, \u0027attempted\u0027)"},{"line_number":1246,"context_line":"        success, warnings \u003d self._do_audit_shard_container(broker)"},{"line_number":1247,"context_line":"        if warnings:"},{"line_number":1248,"context_line":"            self.logger.warning("},{"line_number":1249,"context_line":"                \u0027Audit warnings for shard %s (%s): %s\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"2e185e40_bf4f76da","line":1246,"range":{"start_line":1246,"start_character":28,"end_line":1246,"end_character":58},"updated":"2022-09-23 10:09:03.000000000","message":"I\u0027m not sure to what degree it is a \"style rule\", but I always expect to look up in the file for helper methods called from a method (but I know I often need to look down), particularly when there is only one call chain, I expect it to go up the file.\n\nHere we now have some sub-methods above and some below _audit_shard_container - could we shift _do_audit_shard_container and _reclaim_shard_ranges above?","commit_id":"3cd6c6a27d0303935b936ed5d88f7b2205666137"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"84e6dcd7f654e48a6fd2b2306040fdd0a592864a","unresolved":true,"context_lines":[{"line_number":1261,"context_line":"                broker.empty()):"},{"line_number":1262,"context_line":"            broker.delete_db(Timestamp.now().internal)"},{"line_number":1263,"context_line":"            self.logger.debug(\u0027Deleted shard container %s (%s)\u0027,"},{"line_number":1264,"context_line":"                              broker.db_file, quote(broker.path))"},{"line_number":1265,"context_line":""},{"line_number":1266,"context_line":"    def _do_audit_shard_container(self, broker):"},{"line_number":1267,"context_line":"        warnings \u003d []"}],"source_content_type":"text/x-python","patch_set":2,"id":"a22a2a8d_7ca3f427","line":1264,"updated":"2022-09-23 10:09:03.000000000","message":"+1","commit_id":"3cd6c6a27d0303935b936ed5d88f7b2205666137"}]}