)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"3ef207dcc9cd5167cd599c6e750aae76636e86dc","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: I3e99dc702d55a7424c6482969e03cb4afac854a4"},{"line_number":16,"context_line":"Closes-Bug: 1301728"},{"line_number":17,"context_line":"Co-Authored-By: Pete Zaitcev \u003czaitcev@redhat.com\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"dada55a8_93a168ac","line":17,"updated":"2016-07-25 14:49:00.000000000","message":"Modified Pete\u0027s test form Old patch to address modified change.","commit_id":"025b69e8836dd47c845c8a580c694f91085c1234"}],"doc/source/deployment_guide.rst":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":227,"context_line":"exist in a single file ``/etc/swift/object-server.conf``::"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"    [DEFAULT]"},{"line_number":230,"context_line":"    reclaim_age \u003d 259200"},{"line_number":231,"context_line":""},{"line_number":232,"context_line":"    [pipeline:main]"},{"line_number":233,"context_line":"    pipeline \u003d object-server"}],"source_content_type":"text/x-rst","patch_set":2,"id":"dada55a8_fc6351db","line":230,"updated":"2016-07-25 16:58:09.000000000","message":"there are other places where the reclaim_age option needs to be moved in this file e.g. into the DEFAULT section at line 413","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"}],"etc/object-server.conf-sample":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":257,"context_line":"# node_timeout \u003d 10"},{"line_number":258,"context_line":"# http_timeout \u003d 60"},{"line_number":259,"context_line":"# lockup_timeout \u003d 1800"},{"line_number":260,"context_line":"# reclaim_age \u003d 604800"},{"line_number":261,"context_line":"# ring_check_interval \u003d 15"},{"line_number":262,"context_line":"# recon_cache_path \u003d /var/cache/swift"},{"line_number":263,"context_line":"# handoffs_first \u003d False"}],"source_content_type":"application/octet-stream","patch_set":2,"id":"dada55a8_fc0eb130","line":260,"updated":"2016-07-25 16:58:09.000000000","message":"reclaim_age is here too","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"}],"swift/obj/auditor.py":[{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"e2e2d183bc91540464788dab99907361e7c9f8a3","unresolved":false,"context_lines":[{"line_number":233,"context_line":"        # this method doesn\u0027t normally raise errors, even if the audit"},{"line_number":234,"context_line":"        # location does not exist; if this raises an unexpected error it"},{"line_number":235,"context_line":"        # will get logged in failsafe"},{"line_number":236,"context_line":"        df \u003d diskfile_mgr.get_diskfile_from_audit_location(location)"},{"line_number":237,"context_line":"        reader \u003d None"},{"line_number":238,"context_line":"        try:"},{"line_number":239,"context_line":"            with df.open():"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_46a72a4f","line":236,"updated":"2016-07-29 06:28:28.000000000","message":"I\u0027m wondering if we can modify get_diskfile_from_audit_location to use get_diskfile_from_hash for this (probably it needs more cleanup though). That is because get_diskfile_from_hash obviously calls cleanup_ondisk_files that can remove older tombstones than recaim_age. That could solve the issue of undeleted tombstone...","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"ede59d9c8438950ee70a394d6d272e02ee3eeb6e","unresolved":false,"context_lines":[{"line_number":233,"context_line":"        # this method doesn\u0027t normally raise errors, even if the audit"},{"line_number":234,"context_line":"        # location does not exist; if this raises an unexpected error it"},{"line_number":235,"context_line":"        # will get logged in failsafe"},{"line_number":236,"context_line":"        df \u003d diskfile_mgr.get_diskfile_from_audit_location(location)"},{"line_number":237,"context_line":"        reader \u003d None"},{"line_number":238,"context_line":"        try:"},{"line_number":239,"context_line":"            with df.open():"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_41038a0d","line":236,"in_reply_to":"bacf61ea_46a72a4f","updated":"2016-07-29 10:39:03.000000000","message":"Yeah maybe some refactoring could result in what you suggest. But I\u0027ll think a little more on this, because need to figure why this would be better than removing file and/or calling invalidate_hash.","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"296bac2f0634cd60cd25e4cef367ef572f26f192","unresolved":false,"context_lines":[{"line_number":262,"context_line":"            # If the tombstone is old and beyond reclaim age, delete it and"},{"line_number":263,"context_line":"            # invalidate hash"},{"line_number":264,"context_line":"            ondisk_info_dict \u003d df._ondisk_info or {}"},{"line_number":265,"context_line":"            ts \u003d (ondisk_info_dict[\u0027ts_info\u0027][\u0027timestamp\u0027]).internal"},{"line_number":266,"context_line":"            ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":267,"context_line":"            reclaim_age \u003d diskfile_mgr.reclaim_age"},{"line_number":268,"context_line":"            if (time.time() - float(ts)) \u003e reclaim_age:"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_15d39a05","line":265,"updated":"2016-07-29 04:21:29.000000000","message":"stylistinc bikeshedding: these two statements look contradictory. If you do hit an empty _ondisk_info, you set a dictionary... but that will immediately crash with KeyError. Right? So the \"or {}\" part does nothing.","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"ede59d9c8438950ee70a394d6d272e02ee3eeb6e","unresolved":false,"context_lines":[{"line_number":262,"context_line":"            # If the tombstone is old and beyond reclaim age, delete it and"},{"line_number":263,"context_line":"            # invalidate hash"},{"line_number":264,"context_line":"            ondisk_info_dict \u003d df._ondisk_info or {}"},{"line_number":265,"context_line":"            ts \u003d (ondisk_info_dict[\u0027ts_info\u0027][\u0027timestamp\u0027]).internal"},{"line_number":266,"context_line":"            ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":267,"context_line":"            reclaim_age \u003d diskfile_mgr.reclaim_age"},{"line_number":268,"context_line":"            if (time.time() - float(ts)) \u003e reclaim_age:"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_c95a0a50","line":265,"in_reply_to":"bacf61ea_15d39a05","updated":"2016-07-29 10:39:03.000000000","message":"Correct, my bad. Will fix it, thanks.","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"296bac2f0634cd60cd25e4cef367ef572f26f192","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":267,"context_line":"            reclaim_age \u003d diskfile_mgr.reclaim_age"},{"line_number":268,"context_line":"            if (time.time() - float(ts)) \u003e reclaim_age:"},{"line_number":269,"context_line":"                remove_file(ts_file)"},{"line_number":270,"context_line":"                diskfile.invalidate_hash(dirname(dirname(ts_file)))"},{"line_number":271,"context_line":"        except DiskFileNotExist:"},{"line_number":272,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_b5ebaeaf","line":269,"updated":"2016-07-29 04:21:29.000000000","message":"Just one? I suppose there\u0027s no harm in this if you have 2 tombstones, auditor will find it on next pass...","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"e2e2d183bc91540464788dab99907361e7c9f8a3","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":267,"context_line":"            reclaim_age \u003d diskfile_mgr.reclaim_age"},{"line_number":268,"context_line":"            if (time.time() - float(ts)) \u003e reclaim_age:"},{"line_number":269,"context_line":"                remove_file(ts_file)"},{"line_number":270,"context_line":"                diskfile.invalidate_hash(dirname(dirname(ts_file)))"},{"line_number":271,"context_line":"        except DiskFileNotExist:"},{"line_number":272,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_c6af9a92","line":269,"in_reply_to":"bacf61ea_b5ebaeaf","updated":"2016-07-29 06:28:28.000000000","message":"I don\u0027t think we need to remove the file directly here. Because the trigger with invalidate_hash will issue the cleanup_dir against to object-replicator and object-replicator knows which files should be deleted and also will delete the hash dir if it\u0027s empty.\n\nRight?","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"ede59d9c8438950ee70a394d6d272e02ee3eeb6e","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":267,"context_line":"            reclaim_age \u003d diskfile_mgr.reclaim_age"},{"line_number":268,"context_line":"            if (time.time() - float(ts)) \u003e reclaim_age:"},{"line_number":269,"context_line":"                remove_file(ts_file)"},{"line_number":270,"context_line":"                diskfile.invalidate_hash(dirname(dirname(ts_file)))"},{"line_number":271,"context_line":"        except DiskFileNotExist:"},{"line_number":272,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_c10f7a2c","line":269,"in_reply_to":"bacf61ea_b5ebaeaf","updated":"2016-07-29 10:39:03.000000000","message":"Yeah, it\u0027d find on next pass.","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"ede59d9c8438950ee70a394d6d272e02ee3eeb6e","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":267,"context_line":"            reclaim_age \u003d diskfile_mgr.reclaim_age"},{"line_number":268,"context_line":"            if (time.time() - float(ts)) \u003e reclaim_age:"},{"line_number":269,"context_line":"                remove_file(ts_file)"},{"line_number":270,"context_line":"                diskfile.invalidate_hash(dirname(dirname(ts_file)))"},{"line_number":271,"context_line":"        except DiskFileNotExist:"},{"line_number":272,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_de6e372e","line":269,"in_reply_to":"bacf61ea_c6af9a92","updated":"2016-07-29 10:39:03.000000000","message":"invalidate_hash in itself will not trigger cleanup_dir - https://github.com/openstack/swift/blob/master/swift/obj/diskfile.py#L293\n\nDo you mean object-replicator takes care of empty hash dir?","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"f2c220e17c1baefcca2c573b4ec8672640d12d07","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":267,"context_line":"            reclaim_age \u003d diskfile_mgr.reclaim_age"},{"line_number":268,"context_line":"            if (time.time() - float(ts)) \u003e reclaim_age:"},{"line_number":269,"context_line":"                remove_file(ts_file)"},{"line_number":270,"context_line":"                diskfile.invalidate_hash(dirname(dirname(ts_file)))"},{"line_number":271,"context_line":"        except DiskFileNotExist:"},{"line_number":272,"context_line":"            pass"}],"source_content_type":"text/x-python","patch_set":3,"id":"bacf61ea_00814fc3","line":269,"in_reply_to":"bacf61ea_de6e372e","updated":"2016-08-01 02:23:02.000000000","message":"Ok, good question. As you said, invalidate_hash doesn\u0027t call cleanup directly but it issues the cleanup to the next round get_hashes.\n\nIIRC, the tombstone reclaiming is a role of object-replicator. In the replication process, replicator will call get_hashes and then compare the hash with remote object-server. In the get_hashes call, replicator will remove the tombstone older than reclaim age if it\u0027s found. However, as the bug report described, if we have already the hash.pkl for the partition dir and it is never invalidated for some reasons (e.g. users calls only GET after reclaim_age passed), the hash.pkl will return empty value for the suffix dir as no date exists in the dir and unfortunately .ts file exists forever there.\n\nHowever, if we call invalidate_hash for the suffix_dir, object-replicator will attempt to get hashes *WITH RESOLVING* the directly consistency (i.e. it can reclaim the older .ts file).\n\nThis [1] is an additional unit test for tracking the get_hashes behavior.\n\nAnd, this code looks like to have diskfile.invalidate_has() already at L270. Hence, object-replicator should reclaim the object in the suffix dir on the next round replication process due to the hash_invalidate.\n\nThat\u0027s what I meant in the previous comment.\n\nI think it could be better a few process can have the role to delete files for code simplification and AFAIK auditor has not had it so far. That\u0027s why I thought we don\u0027t have to call remove_files directly here.\n\n\n1: https://gist.github.com/bloodeagle40234/b97ccf55f7e6526a646603ba63185f44","commit_id":"e26ec27de4d5c4da8173bdeb6b68e64a80a7792f"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"392843131eb4942e8fcf03aa721e36611f0e2711","unresolved":false,"context_lines":[{"line_number":45,"context_line":"        self.conf \u003d conf"},{"line_number":46,"context_line":"        self.logger \u003d logger"},{"line_number":47,"context_line":"        self.devices \u003d devices"},{"line_number":48,"context_line":"        self.diskfile_router \u003d diskfile.DiskFileRouter(conf, self.logger)"},{"line_number":49,"context_line":"        self.max_files_per_second \u003d float(conf.get(\u0027files_per_second\u0027, 20))"},{"line_number":50,"context_line":"        self.max_bytes_per_second \u003d float(conf.get(\u0027bytes_per_second\u0027,"},{"line_number":51,"context_line":"                                                   10000000))"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_42020c0b","line":48,"updated":"2016-08-18 07:04:28.000000000","message":"Probably the reason you thought \"we need reclaim_age value inside of auditor module\" is the miss setting of DiskFileRouter here. Basically DiskFileRouter take a config dict for DiskFileManagers. However, this \"conf\" is for auditor section, i.e. we are missing any config values for object-replicator/object-server here.\n\nSo I think we have a couple of solutions:\n\n- Update the conf file with what we need likely\n\n conf.update({\u0027reclaim_age\u0027: self.reclaim_age})\n self.diskfile_router \u003d diskfile.DiskFileRouter(conf, self.logger)\n\n(To do this, we need move the diskfile_router init below the recaim_age definition)\n\n\n\nOr\n\n- Change reclaim_age (and probably rsync_timeout) as a config in DEFAULT section.\n\nBecause that is used by module-wide (not only replicator but also auditor)\n\nThe latter one seems to have impact a bit so that I\u0027d prefer the first one, if it could be.","commit_id":"7100ca7610d9b40ea5900b71c2867e1f465e1b1b"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"bb104f50ea961cb04da3745561648468ada8295b","unresolved":false,"context_lines":[{"line_number":45,"context_line":"        self.conf \u003d conf"},{"line_number":46,"context_line":"        self.logger \u003d logger"},{"line_number":47,"context_line":"        self.devices \u003d devices"},{"line_number":48,"context_line":"        self.diskfile_router \u003d diskfile.DiskFileRouter(conf, self.logger)"},{"line_number":49,"context_line":"        self.max_files_per_second \u003d float(conf.get(\u0027files_per_second\u0027, 20))"},{"line_number":50,"context_line":"        self.max_bytes_per_second \u003d float(conf.get(\u0027bytes_per_second\u0027,"},{"line_number":51,"context_line":"                                                   10000000))"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_42668c2a","line":48,"in_reply_to":"3ac371cc_42020c0b","updated":"2016-08-18 07:19:16.000000000","message":"Not sure I understand. I do intend to pull the value from auditor conf. In lines 75-76, I try to get the value from auditor conf and if it doesn\u0027t exist, will fall back to replicator conf value (which if doesn\u0027t exist falls back to a default value).\n\nMaybe what I am missing is adding a note in some docs to tell ops to give reclaim_age conf for auditor?","commit_id":"7100ca7610d9b40ea5900b71c2867e1f465e1b1b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"392843131eb4942e8fcf03aa721e36611f0e2711","unresolved":false,"context_lines":[{"line_number":59,"context_line":"            # if we can\u0027t parse the real config (generally a KeyError on"},{"line_number":60,"context_line":"            # __file__, or SystemExit on no object-replicator section) we use"},{"line_number":61,"context_line":"            # a very conservative default"},{"line_number":62,"context_line":"            default \u003d 86400"},{"line_number":63,"context_line":"            default_reclaim_age \u003d 604800"},{"line_number":64,"context_line":"        else:"},{"line_number":65,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("},{"line_number":66,"context_line":"                \u0027rsync_timeout\u0027, replicator.DEFAULT_RSYNC_TIMEOUT))"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_d1a6ccac","line":63,"range":{"start_line":62,"start_character":0,"end_line":63,"end_character":40},"updated":"2016-08-18 07:04:28.000000000","message":"IMO, it is better to change also \"default\" variable named as \"default_rsync_timeout\" to clarify the meaning comparing with the new variable \"default_reclaim_age\".","commit_id":"7100ca7610d9b40ea5900b71c2867e1f465e1b1b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"392843131eb4942e8fcf03aa721e36611f0e2711","unresolved":false,"context_lines":[{"line_number":65,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("},{"line_number":66,"context_line":"                \u0027rsync_timeout\u0027, replicator.DEFAULT_RSYNC_TIMEOUT))"},{"line_number":67,"context_line":"            default_reclaim_age \u003d int(replicator_config.get("},{"line_number":68,"context_line":"                \u0027reclaim_age\u0027, 604800))"},{"line_number":69,"context_line":"            # Here we can do some light math for ops and use the *replicator\u0027s*"},{"line_number":70,"context_line":"            # rsync_timeout (plus 15 mins to avoid deleting local tempfiles"},{"line_number":71,"context_line":"            # before the remote replicator kills it\u0027s rsync)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_f129489c","line":68,"updated":"2016-08-18 07:04:28.000000000","message":"Could we define DEFAULT_RECLAIM_AGE in replicator as well as  DEFAULT_RSYNC_TIMEOUT?","commit_id":"7100ca7610d9b40ea5900b71c2867e1f465e1b1b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"392843131eb4942e8fcf03aa721e36611f0e2711","unresolved":false,"context_lines":[{"line_number":268,"context_line":"            ondisk_info_dict \u003d df._ondisk_info"},{"line_number":269,"context_line":"            ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":270,"context_line":""},{"line_number":271,"context_line":"            suffix_path \u003d dirname(dirname(ts_file))"},{"line_number":272,"context_line":"            partition_path \u003d dirname(suffix_path)"},{"line_number":273,"context_line":"            device_path \u003d dirname(dirname(partition_path))"},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"            partition_dir \u003d basename(partition_path)"},{"line_number":276,"context_line":"            device_dir \u003d basename(device_path)"},{"line_number":277,"context_line":"            suffix_dir \u003d [basename(suffix_path)]"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"            diskfile_mgr.get_hashes(device_dir, partition_dir, suffix_dir,"},{"line_number":280,"context_line":"                                    location.policy, self.reclaim_age)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_94fd6270","line":277,"range":{"start_line":271,"start_character":12,"end_line":277,"end_character":48},"updated":"2016-08-18 07:04:28.000000000","message":"We know already those info in AuditLocation.\n\nSo\n\n diskfile_mgr.get_hashes(location.device, location.partition,\n                         [location.path[-3:]],\n                         location.policy, self.reclaim_age)\n\nwould be enough instead of \"dirname\" and \"basename\", though we could consider to add \"suffix\" as a variable in the AuditLocation instead of using location.path[-3:].","commit_id":"7100ca7610d9b40ea5900b71c2867e1f465e1b1b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f3e06903df70c6af9c329bfaca53cdc55c9cc16c","unresolved":false,"context_lines":[{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e self.reclaim_age:"},{"line_number":273,"context_line":"                ts_file \u003d ondisk_info_dict[\u0027ts_file\u0027]"},{"line_number":274,"context_line":"                suffix_path \u003d dirname(dirname(ts_file))"},{"line_number":275,"context_line":"                suffix_dir \u003d [basename(suffix_path)]"},{"line_number":276,"context_line":"                diskfile_mgr.get_hashes(location.device, location.partition,"},{"line_number":277,"context_line":"                                        suffix_dir,"},{"line_number":278,"context_line":"                                        location.policy)"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa7ab95a_b16b71b8","line":275,"updated":"2016-09-02 10:32:01.000000000","message":"I find these names sort of confusing - look call site in the object server in the REPLICATE verb - this api is like this:\n\n            hashes \u003d self._diskfile_router[policy].get_hashes(\n                device, partition, suffixes, policy)\n\nsuffixes, a list of suffix.\n\n    # convert a file system path to the suffix of the hash of the name of the object\n    # .../objects/part/\u003csuffix\u003e/hash/timestamp.ts\n    suffix \u003d basename(dirname(dirname(ts_file)))\n    # use the invalidate suffixes api to force a suffix rebuild on this suffix immediately\n    suffixes \u003d [basename(suffix_path)]","commit_id":"c4cb178aae9cd1b890dd1a25269fb97b299b00a5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f3e06903df70c6af9c329bfaca53cdc55c9cc16c","unresolved":false,"context_lines":[{"line_number":275,"context_line":"                suffix_dir \u003d [basename(suffix_path)]"},{"line_number":276,"context_line":"                diskfile_mgr.get_hashes(location.device, location.partition,"},{"line_number":277,"context_line":"                                        suffix_dir,"},{"line_number":278,"context_line":"                                        location.policy)"},{"line_number":279,"context_line":"        except DiskFileNotExist:"},{"line_number":280,"context_line":"            pass"},{"line_number":281,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"fa7ab95a_95048f7e","line":278,"updated":"2016-09-02 10:32:01.000000000","message":"this is too much get_hashes rebuilds the hashes pkl for the entire partition - any invalid suffix PLUS the one we force recalc will list ever hashdir - and we don\u0027t need or want to do that - not for every reclaimable tombstone\n\nit\u0027s quite possible multiple reclaimable tombstones will exist in the same part - on initial rollout probably even multiples in the same suffix (!) - and we don\u0027t want to rehash the whole part and force recalc the same suffix multiple times as we go.","commit_id":"c4cb178aae9cd1b890dd1a25269fb97b299b00a5"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"5e488cf9631783608995fddffc8c644df2b82b10","unresolved":false,"context_lines":[{"line_number":59,"context_line":"            # __file__, or SystemExit on no object-replicator section) we use"},{"line_number":60,"context_line":"            # a very conservative default"},{"line_number":61,"context_line":"            default_rsync_timeout \u003d 86400"},{"line_number":62,"context_line":"            default_reclaim_age \u003d 604800"},{"line_number":63,"context_line":"        else:"},{"line_number":64,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("},{"line_number":65,"context_line":"                \u0027rsync_timeout\u0027, replicator.DEFAULT_RSYNC_TIMEOUT))"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a8ec9b2_258189fb","line":62,"updated":"2016-09-19 07:27:21.000000000","message":"I can understand the very conservative default for the rsync timeout here, I guess,s but the same default reclaim age is used here and in the else.. Seeing as we are using the same number, we should problem variable-ify it.\n\n  default_reclaim_age \u003d 604800\n  ..\n  try:\n      ...\n  except (KeyError, SystemExit):\n      ...\n  else:\n      default_reclaim_age \u003d int(replicator_config.get(\n          \u0027reclaim_age\u0027, default_reclaim_age))","commit_id":"5093229c20037ee1ded87a23f81cd59d036259b4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"68e0126f2c2f068890bed91f796cb2a72f3e9484","unresolved":false,"context_lines":[{"line_number":59,"context_line":"            # __file__, or SystemExit on no object-replicator section) we use"},{"line_number":60,"context_line":"            # a very conservative default"},{"line_number":61,"context_line":"            default_rsync_timeout \u003d 86400"},{"line_number":62,"context_line":"            default_reclaim_age \u003d 604800"},{"line_number":63,"context_line":"        else:"},{"line_number":64,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("},{"line_number":65,"context_line":"                \u0027rsync_timeout\u0027, replicator.DEFAULT_RSYNC_TIMEOUT))"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a8ec9b2_cbf7e18a","line":62,"in_reply_to":"7a8ec9b2_18007365","updated":"2016-09-19 23:59:58.000000000","message":"That\u0027s not the problem, the problem is the same value is being hard coded more then once... if in the future we want to change the default reclaim age, we\u0027ll need to go and find all occurrences and change them manually, but if we only defined it once, then that\u0027s a much easier job, and much less prone to potential unexpected behaviour when we find one we missed. Sure maybe 2 isn\u0027t _that_ big a deal, but it\u0027s good practice.","commit_id":"5093229c20037ee1ded87a23f81cd59d036259b4"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"ff1892009a81d0ba61818042d1b8e531c4ea8ad8","unresolved":false,"context_lines":[{"line_number":59,"context_line":"            # __file__, or SystemExit on no object-replicator section) we use"},{"line_number":60,"context_line":"            # a very conservative default"},{"line_number":61,"context_line":"            default_rsync_timeout \u003d 86400"},{"line_number":62,"context_line":"            default_reclaim_age \u003d 604800"},{"line_number":63,"context_line":"        else:"},{"line_number":64,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("},{"line_number":65,"context_line":"                \u0027rsync_timeout\u0027, replicator.DEFAULT_RSYNC_TIMEOUT))"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a8ec9b2_18007365","line":62,"in_reply_to":"7a8ec9b2_258189fb","updated":"2016-09-19 08:36:32.000000000","message":"Since only one of them is going to happen, I don\u0027t think there is going to be any difference variable-fying it or not.","commit_id":"5093229c20037ee1ded87a23f81cd59d036259b4"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"f2df7d340125550f524dc6ac7068f5da9eac63df","unresolved":false,"context_lines":[{"line_number":59,"context_line":"            # __file__, or SystemExit on no object-replicator section) we use"},{"line_number":60,"context_line":"            # a very conservative default"},{"line_number":61,"context_line":"            default_rsync_timeout \u003d 86400"},{"line_number":62,"context_line":"            default_reclaim_age \u003d 604800"},{"line_number":63,"context_line":"        else:"},{"line_number":64,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("},{"line_number":65,"context_line":"                \u0027rsync_timeout\u0027, replicator.DEFAULT_RSYNC_TIMEOUT))"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a8ec9b2_e4c900ae","line":62,"in_reply_to":"7a8ec9b2_cbf7e18a","updated":"2016-09-20 14:53:53.000000000","message":"yup I agree.","commit_id":"5093229c20037ee1ded87a23f81cd59d036259b4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"5e488cf9631783608995fddffc8c644df2b82b10","unresolved":false,"context_lines":[{"line_number":267,"context_line":"        except DiskFileDeleted:"},{"line_number":268,"context_line":"            # If there is a reclaimable tombstone, invalidate the hash value"},{"line_number":269,"context_line":"            ondisk_info_dict \u003d df._ondisk_info"},{"line_number":270,"context_line":"            ts \u003d (ondisk_info_dict[\u0027ts_info\u0027][\u0027timestamp\u0027])"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e self.reclaim_age:"},{"line_number":273,"context_line":"                df.manager.invalidate_hash(dirname(df._datadir))"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a8ec9b2_00e39bd9","line":270,"updated":"2016-09-19 07:27:21.000000000","message":"ondisk_info_dict name is just as long as df._ondisk_info, so I don\u0027t see the point in putting the dict into a variable and then calling the variable isn\u0027t:\n\n ts \u003d df._ondisk_info[\u0027ts_info\u0027][\u0027timestamp\u0027]\n\nThe same thing, without having to create a variable first. It would be ok if it saves space or if you were going to use the variable more often.. and ondisk_info_dict is redefined on line 279.. but it wont reuse what you just grabbed.\n\nSo the way I see it, either do call directly (like above) or reuse the variable you you put it into (below) if it is already defined.","commit_id":"5093229c20037ee1ded87a23f81cd59d036259b4"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"ff1892009a81d0ba61818042d1b8e531c4ea8ad8","unresolved":false,"context_lines":[{"line_number":267,"context_line":"        except DiskFileDeleted:"},{"line_number":268,"context_line":"            # If there is a reclaimable tombstone, invalidate the hash value"},{"line_number":269,"context_line":"            ondisk_info_dict \u003d df._ondisk_info"},{"line_number":270,"context_line":"            ts \u003d (ondisk_info_dict[\u0027ts_info\u0027][\u0027timestamp\u0027])"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e self.reclaim_age:"},{"line_number":273,"context_line":"                df.manager.invalidate_hash(dirname(df._datadir))"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a8ec9b2_981ac3c4","line":270,"in_reply_to":"7a8ec9b2_00e39bd9","updated":"2016-09-19 08:36:32.000000000","message":"I can\u0027t reuse this one below as I\u0027m defining it here in an except block. Will make a direct use w/o variable.","commit_id":"5093229c20037ee1ded87a23f81cd59d036259b4"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"68e0126f2c2f068890bed91f796cb2a72f3e9484","unresolved":false,"context_lines":[{"line_number":267,"context_line":"        except DiskFileDeleted:"},{"line_number":268,"context_line":"            # If there is a reclaimable tombstone, invalidate the hash value"},{"line_number":269,"context_line":"            ondisk_info_dict \u003d df._ondisk_info"},{"line_number":270,"context_line":"            ts \u003d (ondisk_info_dict[\u0027ts_info\u0027][\u0027timestamp\u0027])"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e self.reclaim_age:"},{"line_number":273,"context_line":"                df.manager.invalidate_hash(dirname(df._datadir))"}],"source_content_type":"text/x-python","patch_set":9,"id":"7a8ec9b2_cb6c4139","line":270,"in_reply_to":"7a8ec9b2_981ac3c4","updated":"2016-09-19 23:59:58.000000000","message":"Sure, what ever works for you :)\n\nYou could intialise it outside the except block, then it would be in scope in both the except and below, also the except isn\u0027t returning so it\u0027ll still run the code below (reuse the variable in the case of a DiskFileDeleted).\n\nBut calling the _ondisk_info is probably cleaner option, so that works better :)","commit_id":"5093229c20037ee1ded87a23f81cd59d036259b4"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b32d434a73a153c075f6a762f9356391f5535bfc","unresolved":false,"context_lines":[{"line_number":56,"context_line":"        except (KeyError, SystemExit):"},{"line_number":57,"context_line":"            # if we can\u0027t parse the real config (generally a KeyError on"},{"line_number":58,"context_line":"            # __file__, or SystemExit on no object-replicator section) we use"},{"line_number":59,"context_line":"            # a very conservative default"},{"line_number":60,"context_line":"            default \u003d 86400"},{"line_number":61,"context_line":"        else:"},{"line_number":62,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_3ccf2a57","side":"PARENT","line":59,"updated":"2016-09-21 21:09:12.000000000","message":"comment should probably be clarified that the default_rsync_timeout is conservative (1 day) - the reclaim age is quite normal (1 week)","commit_id":"561284e3d404d3744ce1c669efbbcec0c7050b0c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1198d0c06c24e3bb0edf782e6a196e3193faff92","unresolved":false,"context_lines":[{"line_number":59,"context_line":"            # __file__, or SystemExit on no object-replicator section) we use"},{"line_number":60,"context_line":"            # a very conservative default"},{"line_number":61,"context_line":"            default_rsync_timeout \u003d 86400"},{"line_number":62,"context_line":"            default_reclaim_age \u003d 604800"},{"line_number":63,"context_line":"        else:"},{"line_number":64,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("},{"line_number":65,"context_line":"                \u0027rsync_timeout\u0027, replicator.DEFAULT_RSYNC_TIMEOUT))"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_38557d4f","line":62,"updated":"2016-09-21 17:55:56.000000000","message":"Over here [1] you defined a constant for default reclaim age (which seems unrelated) but here you don\u0027t?\n\nAlso, in diskfile we define ONE_WEEK \u003d 604800\n\n[1] https://review.openstack.org/#/c/346865/11/swift/obj/replicator.py@45\n\nFeels like we could gather all the constants for days, weeks etc into a single place (utils.py?) and re-use them everywhere. Maybe as a follow up though.","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b32d434a73a153c075f6a762f9356391f5535bfc","unresolved":false,"context_lines":[{"line_number":64,"context_line":"            replicator_rsync_timeout \u003d int(replicator_config.get("},{"line_number":65,"context_line":"                \u0027rsync_timeout\u0027, replicator.DEFAULT_RSYNC_TIMEOUT))"},{"line_number":66,"context_line":"            default_reclaim_age \u003d int(replicator_config.get("},{"line_number":67,"context_line":"                \u0027reclaim_age\u0027, 604800))"},{"line_number":68,"context_line":"            # Here we can do some light math for ops and use the *replicator\u0027s*"},{"line_number":69,"context_line":"            # rsync_timeout (plus 15 mins to avoid deleting local tempfiles"},{"line_number":70,"context_line":"            # before the remote replicator kills it\u0027s rsync)"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_fc830201","line":67,"updated":"2016-09-21 21:09:12.000000000","message":"using the replicator.DEFAULT_RECLAIM_AGE here would look more consistent with the line above.\n\nYou should probably just all it replicator_reclaim_age","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"40536d1b59a318e428743869a2aff8d54d7a5cc3","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            self.conf.get(\u0027rsync_tempfile_timeout\u0027), default_rsync_timeout)"},{"line_number":74,"context_line":"        self.reclaim_age \u003d config_auto_int_value("},{"line_number":75,"context_line":"            self.conf.get(\u0027reclaim_age\u0027), default_reclaim_age)"},{"line_number":76,"context_line":"        conf.update({\u0027reclaim_age\u0027: self.reclaim_age})"},{"line_number":77,"context_line":"        self.diskfile_router \u003d diskfile.DiskFileRouter(conf, self.logger)"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"        self.auditor_type \u003d \u0027ALL\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_f5f8b37e","line":76,"range":{"start_line":76,"start_character":8,"end_line":76,"end_character":54},"updated":"2016-09-21 03:55:47.000000000","message":"why do we update the value in the conf dictionary? Well not seem to only use the self.reclaim_age variable for this value and don\u0027t look in the conf anymore.","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b32d434a73a153c075f6a762f9356391f5535bfc","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            self.conf.get(\u0027rsync_tempfile_timeout\u0027), default_rsync_timeout)"},{"line_number":74,"context_line":"        self.reclaim_age \u003d config_auto_int_value("},{"line_number":75,"context_line":"            self.conf.get(\u0027reclaim_age\u0027), default_reclaim_age)"},{"line_number":76,"context_line":"        conf.update({\u0027reclaim_age\u0027: self.reclaim_age})"},{"line_number":77,"context_line":"        self.diskfile_router \u003d diskfile.DiskFileRouter(conf, self.logger)"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"        self.auditor_type \u003d \u0027ALL\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_9c267eb8","line":76,"in_reply_to":"7a8ec9b2_58a16993","updated":"2016-09-21 21:09:12.000000000","message":"I believe having a DiskFileManager with a reclaim_age that\u0027s different than what\u0027s used by the process that created the router is wrong/insane/bug-waiting-to-happen.\n\nTest make sure we don\u0027t break the defensive coding would be a great addition.","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1198d0c06c24e3bb0edf782e6a196e3193faff92","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            self.conf.get(\u0027rsync_tempfile_timeout\u0027), default_rsync_timeout)"},{"line_number":74,"context_line":"        self.reclaim_age \u003d config_auto_int_value("},{"line_number":75,"context_line":"            self.conf.get(\u0027reclaim_age\u0027), default_reclaim_age)"},{"line_number":76,"context_line":"        conf.update({\u0027reclaim_age\u0027: self.reclaim_age})"},{"line_number":77,"context_line":"        self.diskfile_router \u003d diskfile.DiskFileRouter(conf, self.logger)"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"        self.auditor_type \u003d \u0027ALL\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_58a16993","line":76,"range":{"start_line":76,"start_character":8,"end_line":76,"end_character":54},"in_reply_to":"7a8ec9b2_713af099","updated":"2016-09-21 17:55:56.000000000","message":"I agree with Matt - there seems to be no reason to update the DiskfileManager conf with reclaim_age because AFAICT the auditor is not using any diskfile manager methods that use reclaim_age. So this line could be deleted I think?\n\nIf not there should be a test that fails when I do delete it :)","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"5b9fde94a1f016c0a98e2c204e50b465045c707e","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            self.conf.get(\u0027rsync_tempfile_timeout\u0027), default_rsync_timeout)"},{"line_number":74,"context_line":"        self.reclaim_age \u003d config_auto_int_value("},{"line_number":75,"context_line":"            self.conf.get(\u0027reclaim_age\u0027), default_reclaim_age)"},{"line_number":76,"context_line":"        conf.update({\u0027reclaim_age\u0027: self.reclaim_age})"},{"line_number":77,"context_line":"        self.diskfile_router \u003d diskfile.DiskFileRouter(conf, self.logger)"},{"line_number":78,"context_line":""},{"line_number":79,"context_line":"        self.auditor_type \u003d \u0027ALL\u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_713af099","line":76,"range":{"start_line":76,"start_character":8,"end_line":76,"end_character":54},"in_reply_to":"7a8ec9b2_f5f8b37e","updated":"2016-09-21 07:07:28.000000000","message":"Good question! This was required for previous code to work. The issue is, when there is no reclaim_age value in auditor config, we want it to fall back to replicator\u0027s. Now, if we don\u0027t update the conf dict, in the below line#77 diskfile_router will use the default value in diskfile.py, when it doesn\u0027t find a value in auditor conf - as opposed to falling back to replicator conf.\n\nAlthough for current implementation of invalidate_hash it has no \"direct\" impact, we still want \"conf\" to pick values what it\u0027s supposed to, i.e. either auditor\u0027s or replicator\u0027s config.","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d4de28c9897e1ce231d2a0e80ca1e26195798a5a","unresolved":false,"context_lines":[{"line_number":19,"context_line":"import time"},{"line_number":20,"context_line":"import signal"},{"line_number":21,"context_line":"import re"},{"line_number":22,"context_line":"from os.path import basename, dirname, join"},{"line_number":23,"context_line":"from random import shuffle"},{"line_number":24,"context_line":"from swift import gettext_ as _"},{"line_number":25,"context_line":"from contextlib import closing"}],"source_content_type":"text/x-python","patch_set":12,"id":"7a8ec9b2_fea05244","line":22,"updated":"2016-09-21 22:40:53.000000000","message":"in retrospect I probably should have dropped this unrelated code churn, I don\u0027t particularly even prefer using anonymous os.path functions.","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"7d39e4e6470314464df1e0497e17d2afccd8f501","unresolved":false,"context_lines":[{"line_number":19,"context_line":"import time"},{"line_number":20,"context_line":"import signal"},{"line_number":21,"context_line":"import re"},{"line_number":22,"context_line":"from os.path import basename, dirname, join"},{"line_number":23,"context_line":"from random import shuffle"},{"line_number":24,"context_line":"from swift import gettext_ as _"},{"line_number":25,"context_line":"from contextlib import closing"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_941411f6","line":22,"in_reply_to":"5a8bc5a2_e034dc4b","updated":"2016-09-22 18:29:14.000000000","message":"I just prefer \u0027os.path.join\u0027 to \u0027join\u0027 - dunno why - don\u0027t really care your call","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"610b190a2dc91effdafba8a4a76e0935d8cadca4","unresolved":false,"context_lines":[{"line_number":19,"context_line":"import time"},{"line_number":20,"context_line":"import signal"},{"line_number":21,"context_line":"import re"},{"line_number":22,"context_line":"from os.path import basename, dirname, join"},{"line_number":23,"context_line":"from random import shuffle"},{"line_number":24,"context_line":"from swift import gettext_ as _"},{"line_number":25,"context_line":"from contextlib import closing"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_e034dc4b","line":22,"in_reply_to":"7a8ec9b2_fea05244","updated":"2016-09-22 05:23:13.000000000","message":"I could remove basename, dirname in a next patchset. I would still want to keep join since we use it in more than one place.","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":265,"context_line":"            self.logger.error(_(\u0027ERROR Object %(obj)s failed audit and was\u0027"},{"line_number":266,"context_line":"                                \u0027 quarantined: %(err)s\u0027),"},{"line_number":267,"context_line":"                              {\u0027obj\u0027: location, \u0027err\u0027: err})"},{"line_number":268,"context_line":"        except DiskFileDeleted:"},{"line_number":269,"context_line":"            # If there is a reclaimable tombstone, we\u0027ll invalidate the hash"},{"line_number":270,"context_line":"            # to trigger the replciator to rehash/cleanup this suffix"},{"line_number":271,"context_line":"            ts \u003d df._ondisk_info[\u0027ts_info\u0027][\u0027timestamp\u0027]"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_c7f996ca","line":268,"updated":"2016-09-22 15:14:32.000000000","message":"for a tombstone we\u0027ll reach this clause twice for every auditor run - once on the zero bytes file process and once on the \u0027main\u0027 process - so every reclaimable tombstone will have its suffix added to the hashes.invalid file twice.\n\nThat\u0027s not great and is avoidable by making the invalidate_hash conditional on which process we\u0027re in:\n\n  if (self.zero_byte_only_at_fps and (time.time() - float(ts)) \u003e df.manager.reclaim_age):\n      df.manager.invalidate_hash(dirname(df._datadir))\n\nbut I\u0027m not sure *how* bad it is, given that the suffix will be appended to the hashes.invalid file on every auditor cycle anyway, until a replicator or reconstructor cleans up.","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"7d39e4e6470314464df1e0497e17d2afccd8f501","unresolved":false,"context_lines":[{"line_number":265,"context_line":"            self.logger.error(_(\u0027ERROR Object %(obj)s failed audit and was\u0027"},{"line_number":266,"context_line":"                                \u0027 quarantined: %(err)s\u0027),"},{"line_number":267,"context_line":"                              {\u0027obj\u0027: location, \u0027err\u0027: err})"},{"line_number":268,"context_line":"        except DiskFileDeleted:"},{"line_number":269,"context_line":"            # If there is a reclaimable tombstone, we\u0027ll invalidate the hash"},{"line_number":270,"context_line":"            # to trigger the replciator to rehash/cleanup this suffix"},{"line_number":271,"context_line":"            ts \u003d df._ondisk_info[\u0027ts_info\u0027][\u0027timestamp\u0027]"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_d4cbf970","line":268,"in_reply_to":"5a8bc5a2_c7f996ca","updated":"2016-09-22 18:29:14.000000000","message":"i don\u0027t think it\u0027s that bad, but I think it\u0027s a good improvement regardless, don\u0027t really care which side it ends up in - zbf is a couple few orders of magnitude shorter than md5 audit because of rate-limiting on loaded disks.  Hours vs. Months.","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":267,"context_line":"                              {\u0027obj\u0027: location, \u0027err\u0027: err})"},{"line_number":268,"context_line":"        except DiskFileDeleted:"},{"line_number":269,"context_line":"            # If there is a reclaimable tombstone, we\u0027ll invalidate the hash"},{"line_number":270,"context_line":"            # to trigger the replciator to rehash/cleanup this suffix"},{"line_number":271,"context_line":"            ts \u003d df._ondisk_info[\u0027ts_info\u0027][\u0027timestamp\u0027]"},{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e df.manager.reclaim_age:"},{"line_number":273,"context_line":"                df.manager.invalidate_hash(dirname(df._datadir))"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_49969495","line":270,"updated":"2016-09-22 15:14:32.000000000","message":"nit: s/replciator/replicator/","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":268,"context_line":"        except DiskFileDeleted:"},{"line_number":269,"context_line":"            # If there is a reclaimable tombstone, we\u0027ll invalidate the hash"},{"line_number":270,"context_line":"            # to trigger the replciator to rehash/cleanup this suffix"},{"line_number":271,"context_line":"            ts \u003d df._ondisk_info[\u0027ts_info\u0027][\u0027timestamp\u0027]"},{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e df.manager.reclaim_age:"},{"line_number":273,"context_line":"                df.manager.invalidate_hash(dirname(df._datadir))"},{"line_number":274,"context_line":"        except DiskFileNotExist:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_495e9446","line":271,"updated":"2016-09-22 15:14:32.000000000","message":"follow-up?: The delete timestamp is included as a parameter of DiskFileDeleted, so we can avoid more[1] bleeding of knowledge of df._ondisk_info into the auditor by using:\n\n  except DiskFileDeleted as err:\n      ts \u003d err.timestamp\n\n[1] \"more\" because we already do at line 279\n\nI also noticed that the tombstone metadata is read to get the timestamp, rather than inferring the value from the filename, which seems inefficient, but that is unrelated to this patch.","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"7d39e4e6470314464df1e0497e17d2afccd8f501","unresolved":false,"context_lines":[{"line_number":268,"context_line":"        except DiskFileDeleted:"},{"line_number":269,"context_line":"            # If there is a reclaimable tombstone, we\u0027ll invalidate the hash"},{"line_number":270,"context_line":"            # to trigger the replciator to rehash/cleanup this suffix"},{"line_number":271,"context_line":"            ts \u003d df._ondisk_info[\u0027ts_info\u0027][\u0027timestamp\u0027]"},{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e df.manager.reclaim_age:"},{"line_number":273,"context_line":"                df.manager.invalidate_hash(dirname(df._datadir))"},{"line_number":274,"context_line":"        except DiskFileNotExist:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_345edde9","line":271,"in_reply_to":"5a8bc5a2_495e9446","updated":"2016-09-22 18:29:14.000000000","message":"that\u0027d be a good improvement","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":270,"context_line":"            # to trigger the replciator to rehash/cleanup this suffix"},{"line_number":271,"context_line":"            ts \u003d df._ondisk_info[\u0027ts_info\u0027][\u0027timestamp\u0027]"},{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e df.manager.reclaim_age:"},{"line_number":273,"context_line":"                df.manager.invalidate_hash(dirname(df._datadir))"},{"line_number":274,"context_line":"        except DiskFileNotExist:"},{"line_number":275,"context_line":"            pass"},{"line_number":276,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_ff08f22a","line":273,"updated":"2016-09-22 15:14:32.000000000","message":"nit: df._datadir is same as location, df.manager is same as diskfile_mgr. Possibly makes code easier to follow if we stick to using the same vars throughout. Perhaps it could be argued that we should not assume those equivalences in case some future diskfile broke them, in which case a better interface would be (IMHO):\n\n  diskfile_mgr.invalidate_hash(df)","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"7d39e4e6470314464df1e0497e17d2afccd8f501","unresolved":false,"context_lines":[{"line_number":270,"context_line":"            # to trigger the replciator to rehash/cleanup this suffix"},{"line_number":271,"context_line":"            ts \u003d df._ondisk_info[\u0027ts_info\u0027][\u0027timestamp\u0027]"},{"line_number":272,"context_line":"            if (time.time() - float(ts)) \u003e df.manager.reclaim_age:"},{"line_number":273,"context_line":"                df.manager.invalidate_hash(dirname(df._datadir))"},{"line_number":274,"context_line":"        except DiskFileNotExist:"},{"line_number":275,"context_line":"            pass"},{"line_number":276,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_14aba175","line":273,"in_reply_to":"5a8bc5a2_ff08f22a","updated":"2016-09-22 18:29:14.000000000","message":"i prefer using the diskfiles own manager slightly despite the quivilence - it\u0027ll be less code churn when we get per policy audit locaiton generators working [1]\n\nI thought the only param that invalidate_hash would accept is the path to the suffix dir?\n\n1. https://bugs.launchpad.net/swift/+bug/1534212","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"}],"swift/obj/diskfile.py":[{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"3ef207dcc9cd5167cd599c6e750aae76636e86dc","unresolved":false,"context_lines":[{"line_number":2081,"context_line":"                if (time.time() - float(ts)) \u003e reclaim_age * 1.1:"},{"line_number":2082,"context_line":"                    purge_file \u003d self._manager.make_on_disk_filename("},{"line_number":2083,"context_line":"                        ts, ext\u003d\u0027.ts\u0027)"},{"line_number":2084,"context_line":"                    remove_file(os.path.join(self._datadir, purge_file))"},{"line_number":2085,"context_line":"        return exc"},{"line_number":2086,"context_line":""},{"line_number":2087,"context_line":"    def _verify_name_matches_hash(self, data_file):"}],"source_content_type":"text/x-python","patch_set":1,"id":"dada55a8_53c5303a","line":2084,"updated":"2016-07-25 14:49:00.000000000","message":"This is similar to \"purge\" method except there is no hash invalidation here.","commit_id":"025b69e8836dd47c845c8a580c694f91085c1234"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":2077,"context_line":"                exc \u003d DiskFileDeleted(metadata\u003dmetadata)"},{"line_number":2078,"context_line":"                # If the tombstone is old and beyond reclaim age, delete it"},{"line_number":2079,"context_line":"                ts \u003d Timestamp(metadata[\u0027X-Timestamp\u0027])"},{"line_number":2080,"context_line":"                reclaim_age \u003d self._manager.reclaim_age"},{"line_number":2081,"context_line":"                if (time.time() - float(ts)) \u003e reclaim_age * 1.1:"},{"line_number":2082,"context_line":"                    purge_file \u003d self._manager.make_on_disk_filename("},{"line_number":2083,"context_line":"                        ts, ext\u003d\u0027.ts\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_82662c27","line":2080,"updated":"2016-07-25 16:58:09.000000000","message":"need to be very careful here...currently self.reclaim_age is only used in code paths called from replicator or reconstructor processes. This method is called during object-server operations (i.e. when opening a deleted file while handling GETs/PUTs etc) and the object server uses a disk file manager instance with default self.reclaim_age of one week.\n\nSo with this change, if you intend the reclaim age to be longer than one week then it MUST be set for the object-server config. But many operators will only have it set for the replicator and reconstructor, and may have it \u003e\u003e one week. So when updating to this version they would start to have tombstones reclaimed sooner than they intended.","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"a3f006babe88adf54f6867485ba72f48c8dc4bbf","unresolved":false,"context_lines":[{"line_number":2077,"context_line":"                exc \u003d DiskFileDeleted(metadata\u003dmetadata)"},{"line_number":2078,"context_line":"                # If the tombstone is old and beyond reclaim age, delete it"},{"line_number":2079,"context_line":"                ts \u003d Timestamp(metadata[\u0027X-Timestamp\u0027])"},{"line_number":2080,"context_line":"                reclaim_age \u003d self._manager.reclaim_age"},{"line_number":2081,"context_line":"                if (time.time() - float(ts)) \u003e reclaim_age * 1.1:"},{"line_number":2082,"context_line":"                    purge_file \u003d self._manager.make_on_disk_filename("},{"line_number":2083,"context_line":"                        ts, ext\u003d\u0027.ts\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_88e197d3","line":2080,"in_reply_to":"dada55a8_82662c27","updated":"2016-07-27 14:12:44.000000000","message":"I now moved the code to auditor.py. This should not be a concern I believe?","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":2078,"context_line":"                # If the tombstone is old and beyond reclaim age, delete it"},{"line_number":2079,"context_line":"                ts \u003d Timestamp(metadata[\u0027X-Timestamp\u0027])"},{"line_number":2080,"context_line":"                reclaim_age \u003d self._manager.reclaim_age"},{"line_number":2081,"context_line":"                if (time.time() - float(ts)) \u003e reclaim_age * 1.1:"},{"line_number":2082,"context_line":"                    purge_file \u003d self._manager.make_on_disk_filename("},{"line_number":2083,"context_line":"                        ts, ext\u003d\u0027.ts\u0027)"},{"line_number":2084,"context_line":"                    remove_file(os.path.join(self._datadir, purge_file))"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_a774229c","line":2081,"updated":"2016-07-25 16:58:09.000000000","message":"why multiply reclaim_age by 1.1 ?","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"a3f006babe88adf54f6867485ba72f48c8dc4bbf","unresolved":false,"context_lines":[{"line_number":2078,"context_line":"                # If the tombstone is old and beyond reclaim age, delete it"},{"line_number":2079,"context_line":"                ts \u003d Timestamp(metadata[\u0027X-Timestamp\u0027])"},{"line_number":2080,"context_line":"                reclaim_age \u003d self._manager.reclaim_age"},{"line_number":2081,"context_line":"                if (time.time() - float(ts)) \u003e reclaim_age * 1.1:"},{"line_number":2082,"context_line":"                    purge_file \u003d self._manager.make_on_disk_filename("},{"line_number":2083,"context_line":"                        ts, ext\u003d\u0027.ts\u0027)"},{"line_number":2084,"context_line":"                    remove_file(os.path.join(self._datadir, purge_file))"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_a8b57bd1","line":2081,"in_reply_to":"dada55a8_a774229c","updated":"2016-07-27 14:12:44.000000000","message":"oops, I wanted to remove that.","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":2079,"context_line":"                ts \u003d Timestamp(metadata[\u0027X-Timestamp\u0027])"},{"line_number":2080,"context_line":"                reclaim_age \u003d self._manager.reclaim_age"},{"line_number":2081,"context_line":"                if (time.time() - float(ts)) \u003e reclaim_age * 1.1:"},{"line_number":2082,"context_line":"                    purge_file \u003d self._manager.make_on_disk_filename("},{"line_number":2083,"context_line":"                        ts, ext\u003d\u0027.ts\u0027)"},{"line_number":2084,"context_line":"                    remove_file(os.path.join(self._datadir, purge_file))"},{"line_number":2085,"context_line":"        return exc"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_c7e036fd","line":2082,"updated":"2016-07-25 16:58:09.000000000","message":"is any of this necessary? - ts_file is the full path to the tombstone file","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"a3f006babe88adf54f6867485ba72f48c8dc4bbf","unresolved":false,"context_lines":[{"line_number":2079,"context_line":"                ts \u003d Timestamp(metadata[\u0027X-Timestamp\u0027])"},{"line_number":2080,"context_line":"                reclaim_age \u003d self._manager.reclaim_age"},{"line_number":2081,"context_line":"                if (time.time() - float(ts)) \u003e reclaim_age * 1.1:"},{"line_number":2082,"context_line":"                    purge_file \u003d self._manager.make_on_disk_filename("},{"line_number":2083,"context_line":"                        ts, ext\u003d\u0027.ts\u0027)"},{"line_number":2084,"context_line":"                    remove_file(os.path.join(self._datadir, purge_file))"},{"line_number":2085,"context_line":"        return exc"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_c8b42fd3","line":2082,"in_reply_to":"dada55a8_c7e036fd","updated":"2016-07-27 14:12:44.000000000","message":"Done","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"392843131eb4942e8fcf03aa721e36611f0e2711","unresolved":false,"context_lines":[{"line_number":1029,"context_line":"            if not hash_:"},{"line_number":1030,"context_line":"                suffix_dir \u003d join(partition_path, suffix)"},{"line_number":1031,"context_line":"                try:"},{"line_number":1032,"context_line":"                    print(\u0027in rehash\u0027)"},{"line_number":1033,"context_line":"                    hashes[suffix] \u003d self._hash_suffix(suffix_dir, reclaim_age)"},{"line_number":1034,"context_line":"                    hashed +\u003d 1"},{"line_number":1035,"context_line":"                except PathNotDir:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_54b92acb","line":1032,"updated":"2016-08-18 07:04:28.000000000","message":"Don\u0027t leave \"print\" to stdout.","commit_id":"7100ca7610d9b40ea5900b71c2867e1f465e1b1b"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"392843131eb4942e8fcf03aa721e36611f0e2711","unresolved":false,"context_lines":[{"line_number":1223,"context_line":"                                 policy\u003dpolicy, **kwargs)"},{"line_number":1224,"context_line":""},{"line_number":1225,"context_line":"    def get_hashes(self, device, partition, suffixes, policy,"},{"line_number":1226,"context_line":"                   reclaim_age\u003dNone):"},{"line_number":1227,"context_line":"        \"\"\""},{"line_number":1228,"context_line":""},{"line_number":1229,"context_line":"        :param device: name of target device"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_174e94c0","line":1226,"updated":"2016-08-18 07:04:28.000000000","message":"I don\u0027t think we need to extract get_hashes to be able to take \"reclaim_age\". DiskFileManager has the value reclaim_age from conf so that what we need is adding the variable to the conf at initialization. See my comment in swift/obj/auditor.py","commit_id":"7100ca7610d9b40ea5900b71c2867e1f465e1b1b"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"bb104f50ea961cb04da3745561648468ada8295b","unresolved":false,"context_lines":[{"line_number":1223,"context_line":"                                 policy\u003dpolicy, **kwargs)"},{"line_number":1224,"context_line":""},{"line_number":1225,"context_line":"    def get_hashes(self, device, partition, suffixes, policy,"},{"line_number":1226,"context_line":"                   reclaim_age\u003dNone):"},{"line_number":1227,"context_line":"        \"\"\""},{"line_number":1228,"context_line":""},{"line_number":1229,"context_line":"        :param device: name of target device"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_dd17fd42","line":1226,"in_reply_to":"3ac371cc_174e94c0","updated":"2016-08-18 07:19:16.000000000","message":"what DiskFileManager has here is replicator value, what I\u0027d need to send is auditor\u0027s conf value (if one is defined). So I need this value here. I asserted this with a test, it will fail without this.","commit_id":"7100ca7610d9b40ea5900b71c2867e1f465e1b1b"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"597d964a855b90749499d90a34f2b99eff71db26","unresolved":false,"context_lines":[{"line_number":987,"context_line":"                            got"},{"line_number":988,"context_line":"        :param do_listdir: force existence check for all hashes in the"},{"line_number":989,"context_line":"                           partition"},{"line_number":990,"context_line":"        :param reclaim_age: age at which to remove tombstones"},{"line_number":991,"context_line":""},{"line_number":992,"context_line":"        :returns: tuple of (number of suffix dirs hashed, dictionary of hashes)"},{"line_number":993,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":6,"id":"1ac06dbe_981a776d","side":"PARENT","line":990,"updated":"2016-08-19 14:03:12.000000000","message":"this was a mistake, needs to be removed in another method. Will fix it with next patchset.","commit_id":"561284e3d404d3744ce1c669efbbcec0c7050b0c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1198d0c06c24e3bb0edf782e6a196e3193faff92","unresolved":false,"context_lines":[{"line_number":745,"context_line":"                    ts_info   -\u003e a file info dict for a .ts file"},{"line_number":746,"context_line":"                    data_info -\u003e a file info dict for a .data file"},{"line_number":747,"context_line":"                    meta_info -\u003e a file info dict for a .meta file"},{"line_number":748,"context_line":"                    ctype_info -\u003e a file info dict for a contenttype timestamp"},{"line_number":749,"context_line":"                    unexpected -\u003e a list of file info dicts for unexpected"},{"line_number":750,"context_line":"                                  files"},{"line_number":751,"context_line":"                    possible_reclaim -\u003e a list of file info dicts for possible"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_fb0b0fd3","line":748,"updated":"2016-09-21 17:55:56.000000000","message":"hmm, more accurately its a file info dict for a .meta file that contains the content type value\n\nAlso\n\ns/contenttype/content-type/","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1198d0c06c24e3bb0edf782e6a196e3193faff92","unresolved":false,"context_lines":[{"line_number":746,"context_line":"                    data_info -\u003e a file info dict for a .data file"},{"line_number":747,"context_line":"                    meta_info -\u003e a file info dict for a .meta file"},{"line_number":748,"context_line":"                    ctype_info -\u003e a file info dict for a contenttype timestamp"},{"line_number":749,"context_line":"                    unexpected -\u003e a list of file info dicts for unexpected"},{"line_number":750,"context_line":"                                  files"},{"line_number":751,"context_line":"                    possible_reclaim -\u003e a list of file info dicts for possible"},{"line_number":752,"context_line":"                                        reclaimable files"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_3bcf3780","line":749,"updated":"2016-09-21 17:55:56.000000000","message":"these are file paths not info dicts","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b32d434a73a153c075f6a762f9356391f5535bfc","unresolved":false,"context_lines":[{"line_number":746,"context_line":"                    data_info -\u003e a file info dict for a .data file"},{"line_number":747,"context_line":"                    meta_info -\u003e a file info dict for a .meta file"},{"line_number":748,"context_line":"                    ctype_info -\u003e a file info dict for a contenttype timestamp"},{"line_number":749,"context_line":"                    unexpected -\u003e a list of file info dicts for unexpected"},{"line_number":750,"context_line":"                                  files"},{"line_number":751,"context_line":"                    possible_reclaim -\u003e a list of file info dicts for possible"},{"line_number":752,"context_line":"                                        reclaimable files"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_bc317aa4","line":749,"in_reply_to":"7a8ec9b2_3bcf3780","updated":"2016-09-21 21:09:12.000000000","message":"FWIW all of this is better than not having docstrings for these args - lesson learned is doing extra drive-by cleanup work in the same change is not worth it.\n\nI suggest you learn to use git add -p and reduce your diffs to only needed code - then let me know if you find a *good* workflow that allows you submit drive-bys that happen while working on a patch as unrelated change in gerrit - i normally try to cobble it together with a series of branches and cherry-picking.","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"}],"test/unit/obj/test_auditor.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":749,"context_line":"    def test_with_tombstone_delete(self):"},{"line_number":750,"context_line":""},{"line_number":751,"context_line":"        def do_audit(self, timestamp):"},{"line_number":752,"context_line":"            time_ \u003d Timestamp(timestamp)"},{"line_number":753,"context_line":"            self.setup_bad_zero_byte(timestamp\u003dtime_)"},{"line_number":754,"context_line":"            self.disk_file.delete(timestamp + 1)"},{"line_number":755,"context_line":"            files \u003d os.listdir(self.disk_file._datadir)"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_9615d14a","line":752,"updated":"2016-07-25 16:58:09.000000000","message":"I think we generally use ts or timestamp to name timestamp variables\n\nalso, you could consider using make_timestamp_iter() in test/unit/__init__.py (or search for it under test)","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"a3f006babe88adf54f6867485ba72f48c8dc4bbf","unresolved":false,"context_lines":[{"line_number":749,"context_line":"    def test_with_tombstone_delete(self):"},{"line_number":750,"context_line":""},{"line_number":751,"context_line":"        def do_audit(self, timestamp):"},{"line_number":752,"context_line":"            time_ \u003d Timestamp(timestamp)"},{"line_number":753,"context_line":"            self.setup_bad_zero_byte(timestamp\u003dtime_)"},{"line_number":754,"context_line":"            self.disk_file.delete(timestamp + 1)"},{"line_number":755,"context_line":"            files \u003d os.listdir(self.disk_file._datadir)"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_28d18bfc","line":752,"in_reply_to":"dada55a8_9615d14a","updated":"2016-07-27 14:12:44.000000000","message":"Done","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":750,"context_line":""},{"line_number":751,"context_line":"        def do_audit(self, timestamp):"},{"line_number":752,"context_line":"            time_ \u003d Timestamp(timestamp)"},{"line_number":753,"context_line":"            self.setup_bad_zero_byte(timestamp\u003dtime_)"},{"line_number":754,"context_line":"            self.disk_file.delete(timestamp + 1)"},{"line_number":755,"context_line":"            files \u003d os.listdir(self.disk_file._datadir)"},{"line_number":756,"context_line":"            self.assertEqual(1, len(files))"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_b9e8c67d","line":753,"updated":"2016-07-25 16:58:09.000000000","message":"what is setup_bad_zero_byte doing? It doesn\u0027t *sound* like it should be applied here.","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"a3f006babe88adf54f6867485ba72f48c8dc4bbf","unresolved":false,"context_lines":[{"line_number":750,"context_line":""},{"line_number":751,"context_line":"        def do_audit(self, timestamp):"},{"line_number":752,"context_line":"            time_ \u003d Timestamp(timestamp)"},{"line_number":753,"context_line":"            self.setup_bad_zero_byte(timestamp\u003dtime_)"},{"line_number":754,"context_line":"            self.disk_file.delete(timestamp + 1)"},{"line_number":755,"context_line":"            files \u003d os.listdir(self.disk_file._datadir)"},{"line_number":756,"context_line":"            self.assertEqual(1, len(files))"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_8864f749","line":753,"in_reply_to":"dada55a8_b9e8c67d","updated":"2016-07-27 14:12:44.000000000","message":"yes it doesn\u0027t *sound* correct. I think it should be renamed. It creates a zero byte file, not necessarily a bad one unless such a timestamp is given. I was using it to create a .data file and then do a delete to have a tombstone (.ts). But in my new patchset I have changed the way it\u0027s done, please have a look.","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":757,"context_line":"            self.assertTrue(files[0].endswith(\u0027ts\u0027))"},{"line_number":758,"context_line":"            ts_file_path \u003d os.path.join(self.disk_file._datadir,"},{"line_number":759,"context_line":"                                        \u0027%s.ts\u0027 % Timestamp"},{"line_number":760,"context_line":"                                        (timestamp + 1).internal)"},{"line_number":761,"context_line":"            kwargs \u003d {\u0027mode\u0027: \u0027once\u0027}"},{"line_number":762,"context_line":"            self.auditor.run_audit(**kwargs)"},{"line_number":763,"context_line":"            return ts_file_path"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_192bd295","line":760,"updated":"2016-07-25 16:58:09.000000000","message":"is ts_file_path different from\n\n  os.path.join(self.disk_file._datadir, os.listdir[0])\n\n?","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"a3f006babe88adf54f6867485ba72f48c8dc4bbf","unresolved":false,"context_lines":[{"line_number":757,"context_line":"            self.assertTrue(files[0].endswith(\u0027ts\u0027))"},{"line_number":758,"context_line":"            ts_file_path \u003d os.path.join(self.disk_file._datadir,"},{"line_number":759,"context_line":"                                        \u0027%s.ts\u0027 % Timestamp"},{"line_number":760,"context_line":"                                        (timestamp + 1).internal)"},{"line_number":761,"context_line":"            kwargs \u003d {\u0027mode\u0027: \u0027once\u0027}"},{"line_number":762,"context_line":"            self.auditor.run_audit(**kwargs)"},{"line_number":763,"context_line":"            return ts_file_path"}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_682f03a2","line":760,"in_reply_to":"dada55a8_192bd295","updated":"2016-07-27 14:12:44.000000000","message":"yes it is. But this question made me find an anomaly in the test, thanks! The test was not doing what I thought it was. I modified it now.","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"ced124603724e43c527a382e37f2748c842a4e72","unresolved":false,"context_lines":[{"line_number":773,"context_line":""},{"line_number":774,"context_line":"        # now, audit with an ancient tombstone, trigger delete"},{"line_number":775,"context_line":"        ts_file_path \u003d do_audit(self, now - (10 * 24 * 60 * 60))"},{"line_number":776,"context_line":"        self.assertFalse(os.path.exists(ts_file_path))"},{"line_number":777,"context_line":""},{"line_number":778,"context_line":"    def test_sleeper(self):"},{"line_number":779,"context_line":"        with mock.patch("}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_8c5ade28","line":776,"updated":"2016-07-25 16:58:09.000000000","message":"also test with non-default reclaim_age in config","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"a3f006babe88adf54f6867485ba72f48c8dc4bbf","unresolved":false,"context_lines":[{"line_number":773,"context_line":""},{"line_number":774,"context_line":"        # now, audit with an ancient tombstone, trigger delete"},{"line_number":775,"context_line":"        ts_file_path \u003d do_audit(self, now - (10 * 24 * 60 * 60))"},{"line_number":776,"context_line":"        self.assertFalse(os.path.exists(ts_file_path))"},{"line_number":777,"context_line":""},{"line_number":778,"context_line":"    def test_sleeper(self):"},{"line_number":779,"context_line":"        with mock.patch("}],"source_content_type":"text/x-python","patch_set":2,"id":"bacf61ea_881cf778","line":776,"in_reply_to":"dada55a8_8c5ade28","updated":"2016-07-27 14:12:44.000000000","message":"Done","commit_id":"cb7969b0f112f1f40eb1ce72974209072923a08e"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"fd940c3eb1d57e52c8fbbeb7b645dac24f632cfc","unresolved":false,"context_lines":[{"line_number":780,"context_line":"                return hash_invalid_exists, hash_val, suffix"},{"line_number":781,"context_line":"            return hash_invalid_exists, ts_file"},{"line_number":782,"context_line":""},{"line_number":783,"context_line":"        # verify reclaim_age is of auditor config value"},{"line_number":784,"context_line":"        config_path \u003d os.path.join(self.testdir, \u0027objserver.conf\u0027)"},{"line_number":785,"context_line":"        stub_config \u003d \"\"\""},{"line_number":786,"context_line":"            [object-replicator]"},{"line_number":787,"context_line":"            [object-auditor]"},{"line_number":788,"context_line":"            reclaim_age \u003d 60"},{"line_number":789,"context_line":"            \"\"\""},{"line_number":790,"context_line":"        with open(config_path, \u0027w\u0027) as f:"},{"line_number":791,"context_line":"            f.write(textwrap.dedent(stub_config))"},{"line_number":792,"context_line":"        conf \u003d readconf(config_path, \u0027object-auditor\u0027)"},{"line_number":793,"context_line":"        auditor_worker \u003d auditor.AuditorWorker(conf, self.logger,"},{"line_number":794,"context_line":"                                               self.rcache, self.devices)"},{"line_number":795,"context_line":"        self.assertEqual(auditor_worker.reclaim_age, 60)"},{"line_number":796,"context_line":""},{"line_number":797,"context_line":"        # verify reclaim_age falls back to replicator config value"},{"line_number":798,"context_line":"        # if there is no auditor config value"},{"line_number":799,"context_line":"        config_path \u003d os.path.join(self.testdir, \u0027objserver.conf\u0027)"},{"line_number":800,"context_line":"        stub_config \u003d \"\"\""},{"line_number":801,"context_line":"            [object-replicator]"},{"line_number":802,"context_line":"            reclaim_age \u003d 60"},{"line_number":803,"context_line":"            [object-auditor]"},{"line_number":804,"context_line":"            \"\"\""},{"line_number":805,"context_line":"        with open(config_path, \u0027w\u0027) as f:"},{"line_number":806,"context_line":"            f.write(textwrap.dedent(stub_config))"},{"line_number":807,"context_line":"        conf \u003d readconf(config_path, \u0027object-auditor\u0027)"},{"line_number":808,"context_line":"        auditor_worker \u003d auditor.AuditorWorker(conf, self.logger,"},{"line_number":809,"context_line":"                                               self.rcache, self.devices)"},{"line_number":810,"context_line":"        self.assertEqual(auditor_worker.reclaim_age, 60)"},{"line_number":811,"context_line":""},{"line_number":812,"context_line":"        self.auditor \u003d auditor.ObjectAuditor(self.conf)"},{"line_number":813,"context_line":"        self.auditor.log_time \u003d 0"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a8ec9b2_b3404337","line":810,"range":{"start_line":783,"start_character":8,"end_line":810,"end_character":56},"updated":"2016-09-20 07:04:41.000000000","message":"I feel this is a good test, but outside of scope of what this test is trying to test. It would be good if this was moved to its own test method. Something like:\n\n def test_reclaim_age_config(self):\n     ...","commit_id":"521ca004e44239ea179894f32398f4c5fb8823ba"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"2c84b73b0e4a2178a840236aa79074215dc62792","unresolved":false,"context_lines":[{"line_number":780,"context_line":"                return hash_invalid_exists, hash_val, suffix"},{"line_number":781,"context_line":"            return hash_invalid_exists, ts_file"},{"line_number":782,"context_line":""},{"line_number":783,"context_line":"        # verify reclaim_age is of auditor config value"},{"line_number":784,"context_line":"        config_path \u003d os.path.join(self.testdir, \u0027objserver.conf\u0027)"},{"line_number":785,"context_line":"        stub_config \u003d \"\"\""},{"line_number":786,"context_line":"            [object-replicator]"},{"line_number":787,"context_line":"            [object-auditor]"},{"line_number":788,"context_line":"            reclaim_age \u003d 60"},{"line_number":789,"context_line":"            \"\"\""},{"line_number":790,"context_line":"        with open(config_path, \u0027w\u0027) as f:"},{"line_number":791,"context_line":"            f.write(textwrap.dedent(stub_config))"},{"line_number":792,"context_line":"        conf \u003d readconf(config_path, \u0027object-auditor\u0027)"},{"line_number":793,"context_line":"        auditor_worker \u003d auditor.AuditorWorker(conf, self.logger,"},{"line_number":794,"context_line":"                                               self.rcache, self.devices)"},{"line_number":795,"context_line":"        self.assertEqual(auditor_worker.reclaim_age, 60)"},{"line_number":796,"context_line":""},{"line_number":797,"context_line":"        # verify reclaim_age falls back to replicator config value"},{"line_number":798,"context_line":"        # if there is no auditor config value"},{"line_number":799,"context_line":"        config_path \u003d os.path.join(self.testdir, \u0027objserver.conf\u0027)"},{"line_number":800,"context_line":"        stub_config \u003d \"\"\""},{"line_number":801,"context_line":"            [object-replicator]"},{"line_number":802,"context_line":"            reclaim_age \u003d 60"},{"line_number":803,"context_line":"            [object-auditor]"},{"line_number":804,"context_line":"            \"\"\""},{"line_number":805,"context_line":"        with open(config_path, \u0027w\u0027) as f:"},{"line_number":806,"context_line":"            f.write(textwrap.dedent(stub_config))"},{"line_number":807,"context_line":"        conf \u003d readconf(config_path, \u0027object-auditor\u0027)"},{"line_number":808,"context_line":"        auditor_worker \u003d auditor.AuditorWorker(conf, self.logger,"},{"line_number":809,"context_line":"                                               self.rcache, self.devices)"},{"line_number":810,"context_line":"        self.assertEqual(auditor_worker.reclaim_age, 60)"},{"line_number":811,"context_line":""},{"line_number":812,"context_line":"        self.auditor \u003d auditor.ObjectAuditor(self.conf)"},{"line_number":813,"context_line":"        self.auditor.log_time \u003d 0"}],"source_content_type":"text/x-python","patch_set":10,"id":"7a8ec9b2_c29317f9","line":810,"range":{"start_line":783,"start_character":8,"end_line":810,"end_character":56},"in_reply_to":"7a8ec9b2_b3404337","updated":"2016-09-20 14:53:40.000000000","message":"Sure, makes sense.","commit_id":"521ca004e44239ea179894f32398f4c5fb8823ba"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":751,"context_line":"    def test_with_tombstone_delete(self):"},{"line_number":752,"context_line":"        test_md5 \u003d \u0027098f6bcd4621d373cade4e832627b4f6\u0027"},{"line_number":753,"context_line":""},{"line_number":754,"context_line":"        def do_audit(self, timestamp, invalidate\u003dFalse):"},{"line_number":755,"context_line":"            dir_path \u003d self.disk_file._datadir"},{"line_number":756,"context_line":"            ts_file \u003d os.path.join(dir_path, \u0027%d.ts\u0027 % timestamp)"},{"line_number":757,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_0b355c45","line":754,"updated":"2016-09-22 15:14:32.000000000","message":"no need to pass self into the function, its available from the outer context\n\nnit: don\u0027t use \u0027timestamp\u0027 for a param that is not a Timestamp","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":760,"context_line":"                mkdirs(dir_path)"},{"line_number":761,"context_line":"            fp \u003d open(ts_file, \u0027w\u0027)"},{"line_number":762,"context_line":"            write_metadata(fp, {\u0027X-Timestamp\u0027: \u0027%d\u0027 % timestamp})"},{"line_number":763,"context_line":"            fp.close()"},{"line_number":764,"context_line":"            # Create hashes.pkl"},{"line_number":765,"context_line":"            hash \u003d dirname(dirname(ts_file))  # hash value of ts file"},{"line_number":766,"context_line":"            suffix \u003d basename(hash)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_9186feb7","line":763,"updated":"2016-09-22 15:14:32.000000000","message":"use with open(...) context manager to have the close done automagically","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":762,"context_line":"            write_metadata(fp, {\u0027X-Timestamp\u0027: \u0027%d\u0027 % timestamp})"},{"line_number":763,"context_line":"            fp.close()"},{"line_number":764,"context_line":"            # Create hashes.pkl"},{"line_number":765,"context_line":"            hash \u003d dirname(dirname(ts_file))  # hash value of ts file"},{"line_number":766,"context_line":"            suffix \u003d basename(hash)"},{"line_number":767,"context_line":"            hashes_pkl \u003d join(os.path.dirname(hash), HASH_FILE)"},{"line_number":768,"context_line":"            with open(hashes_pkl, \u0027wb\u0027) as fp:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_91ffde33","line":765,"updated":"2016-09-22 15:14:32.000000000","message":"^^ comment and var name are misleading, this is the suffix dir not the hash","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":766,"context_line":"            suffix \u003d basename(hash)"},{"line_number":767,"context_line":"            hashes_pkl \u003d join(os.path.dirname(hash), HASH_FILE)"},{"line_number":768,"context_line":"            with open(hashes_pkl, \u0027wb\u0027) as fp:"},{"line_number":769,"context_line":"                pickle.dump({suffix: test_md5}, fp, 0)"},{"line_number":770,"context_line":"            # Run auditor"},{"line_number":771,"context_line":"            kwargs \u003d {\u0027mode\u0027: \u0027once\u0027}"},{"line_number":772,"context_line":"            self.auditor.run_audit(**kwargs)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_31444a73","line":769,"updated":"2016-09-22 15:14:32.000000000","message":"^^ there is a lot of intricate setup here that could be done by deleting the diskfile and running get_hashes, without the risk of the test getting something wrong or getting out of sync with any future changes to the diskfile implementation","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":778,"context_line":"                with open(hash_invalid, \u0027rb\u0027) as fp:"},{"line_number":779,"context_line":"                    hash_val \u003d fp.read()"},{"line_number":780,"context_line":"                return hash_invalid_exists, hash_val, suffix"},{"line_number":781,"context_line":"            return hash_invalid_exists, ts_file"},{"line_number":782,"context_line":""},{"line_number":783,"context_line":"        self.auditor \u003d auditor.ObjectAuditor(self.conf)"},{"line_number":784,"context_line":"        self.auditor.log_time \u003d 0"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_eb366fb3","line":781,"updated":"2016-09-22 15:14:32.000000000","message":"hmmm, different number of return values depending on invalidate. It works but seems error-prone. I\u0027d prefer to split the different validation steps for each scenario from the common setup plus audit run.","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":788,"context_line":"        # audit with a recent tombstone"},{"line_number":789,"context_line":"        hash_invalid_exists, ts_file \u003d do_audit(self, now - 55)"},{"line_number":790,"context_line":"        self.assertFalse(hash_invalid_exists)"},{"line_number":791,"context_line":"        os.unlink(ts_file)"},{"line_number":792,"context_line":""},{"line_number":793,"context_line":"        # audit with a tombstone that is beyond default reclaim_age"},{"line_number":794,"context_line":"        hash_invalid_exists, hash_val, suffix \u003d do_audit(self, now - (604800),"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_d473144c","line":791,"updated":"2016-09-22 15:14:32.000000000","message":"ok, the unlink is important, but maybe a sign that a separate test with fresh setup() is needed","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":794,"context_line":"        hash_invalid_exists, hash_val, suffix \u003d do_audit(self, now - (604800),"},{"line_number":795,"context_line":"                                                         True)"},{"line_number":796,"context_line":"        self.assertTrue(hash_invalid_exists)"},{"line_number":797,"context_line":"        self.assertEqual(hash_val.strip(\u0027\\n\u0027), suffix)"},{"line_number":798,"context_line":""},{"line_number":799,"context_line":"    def test_auditor_reclaim_age(self):"},{"line_number":800,"context_line":"        # if we don\u0027t have access to the replicator config section we\u0027ll"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_94a0ac92","line":797,"updated":"2016-09-22 15:14:32.000000000","message":"add a scenario for non-default conf of reclaim_age to check it gets used by the auditor","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":12261,"name":"Mahati","email":"mahati.chamarthy@gmail.com","username":"Mahati"},"change_message_id":"610b190a2dc91effdafba8a4a76e0935d8cadca4","unresolved":false,"context_lines":[{"line_number":797,"context_line":"        self.assertEqual(hash_val.strip(\u0027\\n\u0027), suffix)"},{"line_number":798,"context_line":""},{"line_number":799,"context_line":"    def test_auditor_reclaim_age(self):"},{"line_number":800,"context_line":"        # if we don\u0027t have access to the replicator config section we\u0027ll"},{"line_number":801,"context_line":"        # diskfile\u0027s default"},{"line_number":802,"context_line":"        auditor_worker \u003d auditor.AuditorWorker(self.conf, self.logger,"},{"line_number":803,"context_line":"                                               self.rcache, self.devices)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_0011c8ac","line":800,"updated":"2016-09-22 05:23:13.000000000","message":"nit: s/we\u0027ll/we\u0027ll use","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":860,"context_line":"                                               self.rcache, self.devices)"},{"line_number":861,"context_line":"        router \u003d auditor_worker.diskfile_router"},{"line_number":862,"context_line":"        for policy in POLICIES:"},{"line_number":863,"context_line":"            self.assertEqual(router[policy].reclaim_age, 60)"},{"line_number":864,"context_line":""},{"line_number":865,"context_line":"        # we\u0027ll prefer our own DEFAULT section to the replicator though"},{"line_number":866,"context_line":"        self.assertEqual(auditor_worker.rsync_tempfile_timeout,"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_d036c5b3","line":863,"updated":"2016-09-22 15:14:32.000000000","message":"yes. this fails is i revert change in auditor conf setup","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":862,"context_line":"        for policy in POLICIES:"},{"line_number":863,"context_line":"            self.assertEqual(router[policy].reclaim_age, 60)"},{"line_number":864,"context_line":""},{"line_number":865,"context_line":"        # we\u0027ll prefer our own DEFAULT section to the replicator though"},{"line_number":866,"context_line":"        self.assertEqual(auditor_worker.rsync_tempfile_timeout,"},{"line_number":867,"context_line":"                         replicator.DEFAULT_RSYNC_TIMEOUT + 900)"},{"line_number":868,"context_line":"        stub_config \u003d \"\"\""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_eb03d00b","line":865,"updated":"2016-09-22 15:14:32.000000000","message":"also need to test that we prefer object-auditor section over replicator section","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"23ee99d09a0076df8b5a0071a8e146110614bfa5","unresolved":false,"context_lines":[{"line_number":871,"context_line":"        [object-replicator]"},{"line_number":872,"context_line":"        reclaim_age \u003d 1800"},{"line_number":873,"context_line":"        [object-auditor]"},{"line_number":874,"context_line":"        \"\"\""},{"line_number":875,"context_line":"        with open(config_path, \u0027w\u0027) as f:"},{"line_number":876,"context_line":"            f.write(textwrap.dedent(stub_config))"},{"line_number":877,"context_line":"        conf \u003d readconf(config_path, \u0027object-auditor\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_ebd4b048","line":874,"updated":"2016-09-22 15:14:32.000000000","message":"should we log a warning if auditor reclaim_age is less than the replicator value, since that will cause unnecessary replication work?\n\ninvalid suffix -\u003e rehash -\u003e not past replication reclaim_age -\u003e hash takes previous value","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"7d39e4e6470314464df1e0497e17d2afccd8f501","unresolved":false,"context_lines":[{"line_number":871,"context_line":"        [object-replicator]"},{"line_number":872,"context_line":"        reclaim_age \u003d 1800"},{"line_number":873,"context_line":"        [object-auditor]"},{"line_number":874,"context_line":"        \"\"\""},{"line_number":875,"context_line":"        with open(config_path, \u0027w\u0027) as f:"},{"line_number":876,"context_line":"            f.write(textwrap.dedent(stub_config))"},{"line_number":877,"context_line":"        conf \u003d readconf(config_path, \u0027object-auditor\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a8bc5a2_d43cd931","line":874,"in_reply_to":"5a8bc5a2_ebd4b048","updated":"2016-09-22 18:29:14.000000000","message":"I\u0027d rather just get move reclaim_age to a global system configured value for the entire object storage layer [1]\n\n1. https://review.openstack.org/#/c/374419/","commit_id":"81d4673674febdbe0bba6e27a6d24739456eb3c4"}],"test/unit/obj/test_diskfile.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1198d0c06c24e3bb0edf782e6a196e3193faff92","unresolved":false,"context_lines":[{"line_number":5052,"context_line":"            hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [], policy)"},{"line_number":5053,"context_line":"            self.assertEqual(hashes, {})"},{"line_number":5054,"context_line":""},{"line_number":5055,"context_line":"    def test_hash_suffix_one_reclaim_tombstone_with_hash_pkl(self):"},{"line_number":5056,"context_line":"        for policy in self.iter_policies():"},{"line_number":5057,"context_line":"            df_mgr \u003d self.df_router[policy]"},{"line_number":5058,"context_line":"            df \u003d df_mgr.get_diskfile("}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_7bd8df1e","line":5055,"updated":"2016-09-21 17:55:56.000000000","message":"this is quite a large addition for an unrelated drive-by fix","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1198d0c06c24e3bb0edf782e6a196e3193faff92","unresolved":false,"context_lines":[{"line_number":5090,"context_line":"            self.assertIn(tombstone, os.listdir(df._datadir))"},{"line_number":5091,"context_line":""},{"line_number":5092,"context_line":"            # However if we call invalidate_hash for the suffix dir,"},{"line_number":5093,"context_line":"            # get_hashes can reclaim the tombstone"},{"line_number":5094,"context_line":"            with mock.patch(\u0027swift.obj.diskfile.lock_path\u0027):"},{"line_number":5095,"context_line":"                df_mgr.invalidate_hash(suffix_dir)"},{"line_number":5096,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_db1733a1","line":5093,"updated":"2016-09-21 17:55:56.000000000","message":"ah ok, its not completely unrelated","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1198d0c06c24e3bb0edf782e6a196e3193faff92","unresolved":false,"context_lines":[{"line_number":5103,"context_line":"            self.assertIn(tombstone, os.listdir(df._datadir))"},{"line_number":5104,"context_line":"            self.assertEqual(hash_timestamp, os.stat(hash_file).st_mtime)"},{"line_number":5105,"context_line":""},{"line_number":5106,"context_line":"            # *BUT* if suffix value is given to recalc, it can force to recaim!"},{"line_number":5107,"context_line":"            suffix \u003d os.path.dirname(suffix_dir)"},{"line_number":5108,"context_line":"            hashes \u003d df_mgr.get_hashes(\u0027sda1\u0027, \u00270\u0027, [suffix], policy)"},{"line_number":5109,"context_line":"            self.assertFalse(os.path.exists(df._datadir))"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a8ec9b2_1b0efb8b","line":5106,"updated":"2016-09-21 17:55:56.000000000","message":"typo: \"recaim\"","commit_id":"507debaa24a8ec257e80af411dd8836a5213ea18"}]}